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

From: Richard Henderson <richard.henderson@linaro.org>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: qemu-devel@nongnu.org, laurent@vivier.eu, pbonzini@redhat.com,
	imp@bsdimp.com, f4bug@amsat.org
Subject: Re: [PATCH 20/24] accel/tcg: Use interval tree for TARGET_PAGE_DATA_SIZE
Date: Wed, 26 Oct 2022 07:29:05 +1000	[thread overview]
Message-ID: <ba06b907-4fec-e21d-7bc4-64f92ef8d909@linaro.org> (raw)
In-Reply-To: <87bkpz66h7.fsf@linaro.org>

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~