[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] xen/grant-table: Simplify the update to the per-vCPU maptrack freelist

Hi Jan,

On 28/05/2021 14:29, Jan Beulich wrote:
On 26.05.2021 17:21, Julien Grall wrote:
From: Julien Grall <jgrall@xxxxxxxxxx>

Since XSA-288 (commit 02cbeeb62075 "gnttab: split maptrack lock to make

XSA-228 I suppose?

Yes, I will update in the next version.

it fulfill its purpose again"), v->maptrack_head and v->maptrack_tail
are with the lock v->maptrack_freelist_lock held.

Nit: missing "accessed" or alike?
I have added "accessed".

Therefore it is not necessary to update the fields using cmpxchg()
and also read them atomically.

Ah yes, very good observation. Should have noticed this back at the
time, for an immediate follow-up change.

Note that there are two cases where v->maptrack_tail is accessed without
the lock. They both happen _get_maptrack_handle() when the current vCPU
list is empty. Therefore there is no possible race.

I think you mean the other function here, without a leading underscore
in its name.

Hmmm... Yes. I will update it.

And if you want to explain the absence of a race, wouldn't
you then better also mention that the list can get initially filled
only on the local vCPU?

Sure. I will reword it.

I am not sure whether we should try to protect the remaining unprotected
access with the lock or maybe add a comment?

As per above I don't view adding locking as sensible. If you feel like
adding a helpful comment, perhaps. I will admit that it took me more
than just a moment to recall that "local vCPU only" argument.

I will try to come up with an helpful comment.

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -543,34 +543,26 @@ double_gt_unlock(struct grant_table *lgt, struct 
grant_table *rgt)
  static inline grant_handle_t
  _get_maptrack_handle(struct grant_table *t, struct vcpu *v)
-    unsigned int head, next, prev_head;
+    unsigned int head, next;
spin_lock(&v->maptrack_freelist_lock); - do {
-        /* No maptrack pages allocated for this VCPU yet? */
-        head = read_atomic(&v->maptrack_head);
-        if ( unlikely(head == MAPTRACK_TAIL) )
-        {
-            spin_unlock(&v->maptrack_freelist_lock);
-            return INVALID_MAPTRACK_HANDLE;

Where did this and ...

-        }
-        /*
-         * Always keep one entry in the free list to make it easier to
-         * add free entries to the tail.
-         */
-        next = read_atomic(&maptrack_entry(t, head).ref);
-        if ( unlikely(next == MAPTRACK_TAIL) )
-        {
-            spin_unlock(&v->maptrack_freelist_lock);
-            return INVALID_MAPTRACK_HANDLE;

... this use of INVALID_MAPTRACK_HANDLE go? It is at present merely
want to fold them, you will need to do so properly (by eliminating
one of the two constants). But I think they're separate on purpose.

Hmmm... Somehow I thought one was an alias to the other. But they are clearly not. I will update it on the next version.

-        }
+    /* No maptrack pages allocated for this VCPU yet? */
+    head = v->maptrack_head;
+    if ( unlikely(head == MAPTRACK_TAIL) )
+        goto out;
- prev_head = head;
-        head = cmpxchg(&v->maptrack_head, prev_head, next);
-    } while ( head != prev_head );
+    /*
+     * Always keep one entry in the free list to make it easier to
+     * add free entries to the tail.
+     */
+    next = read_atomic(&maptrack_entry(t, head).ref);

Since the lock protects the entire free list, why do you need to
keep read_atomic() here?

Because I wasn't sure whether dropping {write, read}_atomic() when accessing the freelist would be fine.

Anyway, I can drop it in the next version.

+    if ( unlikely(next == MAPTRACK_TAIL) )
+        head = MAPTRACK_TAIL;
+    else
+        v->maptrack_head = next;

Please indent labels by at least one blank, to avoid issues with
diff's -p option. In fact if you didn't introduce a goto here in
the first place, there'd be less code churn overall, as you'd
need to alter the indentation of fewer lines.

I will have a look.

@@ -623,7 +615,7 @@ put_maptrack_handle(
      struct domain *currd = current->domain;
      struct vcpu *v;
-    unsigned int prev_tail, cur_tail;
+    unsigned int prev_tail;
/* 1. Set entry to be a tail. */
      maptrack_entry(t, handle).ref = MAPTRACK_TAIL;
@@ -633,11 +625,8 @@ put_maptrack_handle(
spin_lock(&v->maptrack_freelist_lock); - cur_tail = read_atomic(&v->maptrack_tail);
-    do {
-        prev_tail = cur_tail;
-        cur_tail = cmpxchg(&v->maptrack_tail, prev_tail, handle);
-    } while ( cur_tail != prev_tail );
+    prev_tail = v->maptrack_tail;
+    v->maptrack_tail = handle;
/* 3. Update the old tail entry to point to the new entry. */
      write_atomic(&maptrack_entry(t, prev_tail).ref, handle);

Since the write_atomic() here can then also be converted, may I
ask that you then rename the local variable to just "tail" as


--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -255,7 +255,10 @@ struct vcpu
      /* VCPU paused by system controller. */
      int              controller_pause_count;
- /* Grant table map tracking. */
+    /*
+     * Grant table map tracking. The lock maptrack_freelist_lock protect

Nit: protects

I will fix it.

+     * the access to maptrack_head and maptrack_tail.
+     */

I'm inclined to suggest this doesn't need spelling out, considering ...

      spinlock_t       maptrack_freelist_lock;
      unsigned int     maptrack_head;
      unsigned int     maptrack_tail;

... both the name of the lock and its placement next to the two
fields it protects. Also as per the docs change of the XSA-228 change,
the lock protects more than just these two fields, so the comment may
be misleading the way you have it now.

So I think it would be good to document above the lock what it actually protects. I agree this is fairly clear that it protect maptrack_{head, tail} but this wasn't very clear to me that it would also protect the content of the freelist (so read_atomic()/write_atomic() could be dropped).

I will try to come up with a better comment.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.