Re: [PATCH 20/24] accel/tcg: Use interval tree for TARGET_PAGE_DATA_SIZE

[Richard Henderson]

On 10/26/22 05:30, Alex Bennée wrote:
> >   void *page_get_target_data(target_ulong address)
> >   {
> > -    PageDesc *p = page_find(address >> TARGET_PAGE_BITS);
> > -    void *ret = p->target_data;
> > +    IntervalTreeNode *n;
> > +    TargetPageDataNode *t;
> > +    target_ulong page, region;
> > +    bool locked;
> >   
> > -    if (!ret) {
> > -        ret = g_malloc0(TARGET_PAGE_DATA_SIZE);
> > -        p->target_data = ret;
> > +    page = address & TARGET_PAGE_MASK;
> > +    region = address & TBD_MASK;
> > +
> > +    n = interval_tree_iter_first(&targetdata_root, page, page);
> > +    if (n) {
> > +        goto found;
> >       }
> > -    return ret;
> > +
> > +    /*
> > +     * See util/interval-tree.c re lockless lookups: no false positives but
> > +     * there are false negatives.  If we find nothing, retry with the mmap
> > +     * lock acquired.  We also need the lock for the allocation + insert.
> > +     */
> > +    locked = have_mmap_lock();
>
> Are we expecting to already hold the lock here?

No.  This is called in the middle of a normal load/store instruction, in response to MTE 
being enabled.  We really want to avoid taking a lock if we can.

> > +    if (!locked) {
> > +        mmap_lock();
> > +        n = interval_tree_iter_first(&targetdata_root, page, page);
>
> So we only search if we haven't got a lock already.

We only search *again* if we haven't got a lock already.  If we had the lock, then the 
first search above produced golden results.  And we must have the lock, acquired just now...

> > +        if (n) {
> > +            mmap_unlock();
> > +            goto found;
> > +        }
> > +    }
> > +
> > +    t = g_new0(TargetPageDataNode, 1);
> > +    n = &t->itree;
> > +    n->start = region;
> > +    n->last = region | ~TBD_MASK;
> > +    interval_tree_insert(n, &targetdata_root);

... for the modification to the data structure.

> > +    if (!locked) {
> > +        mmap_unlock();
> > +    }
>
> To be honest the mmap_lock safe to use recursively and wrapping the
> locked portion in a LOCK_GUARD would make me happier that we didn't cock
> up unwinding.

Fair.  I was trying to avoid a second interval tree walk if we *did* happen to have the 
lock, but as I said myself above, that's extremely unlikely given the only user.


r~