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~