From: Julien Grall <julien@xen.org>
To: xen-devel@lists.xenproject.org
Cc: julien@xen.org, Julien Grall <jgrall@amazon.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Ian Jackson <iwj@xenproject.org>, Jan Beulich <jbeulich@suse.com>,
Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>
Subject: [PATCH] xen/grant-table: Simplify the update to the per-vCPU maptrack freelist
Date: Wed, 26 May 2021 16:21:52 +0100 [thread overview]
Message-ID: <20210526152152.26251-1-julien@xen.org> (raw)
From: Julien Grall <jgrall@amazon.com>
Since XSA-288 (commit 02cbeeb62075 "gnttab: split maptrack lock to make
it fulfill its purpose again"), v->maptrack_head and v->maptrack_tail
are with the lock v->maptrack_freelist_lock held.
Therefore it is not necessary to update the fields using cmpxchg()
and also read them atomically.
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.
The code is now reworked to remove any use of cmpxch() and read_atomic()
when accessing the fields v->maptrack_{head, tail}.
Take the opportunity to add a comment on top of the lock definition
and explain what it protects.
Signed-off-by: Julien Grall <jgrall@amazon.com>
----
I am not sure whether we should try to protect the remaining unprotected
access with the lock or maybe add a comment?
---
xen/common/grant_table.c | 60 +++++++++++++++-------------------------
xen/include/xen/sched.h | 5 +++-
2 files changed, 27 insertions(+), 38 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index ab30e2e8cfb6..cac9d1d73446 100644
--- 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;
- }
-
- /*
- * 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;
- }
+ /* 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);
+ if ( unlikely(next == MAPTRACK_TAIL) )
+ head = MAPTRACK_TAIL;
+ else
+ v->maptrack_head = next;
+out:
spin_unlock(&v->maptrack_freelist_lock);
return head;
@@ -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);
@@ -650,7 +639,7 @@ get_maptrack_handle(
struct grant_table *lgt)
{
struct vcpu *curr = current;
- unsigned int i, head;
+ unsigned int i;
grant_handle_t handle;
struct grant_mapping *new_mt = NULL;
@@ -686,7 +675,7 @@ get_maptrack_handle(
maptrack_entry(lgt, handle).ref = MAPTRACK_TAIL;
curr->maptrack_tail = handle;
if ( curr->maptrack_head == MAPTRACK_TAIL )
- write_atomic(&curr->maptrack_head, handle);
+ curr->maptrack_head = handle;
spin_unlock(&curr->maptrack_freelist_lock);
}
return steal_maptrack_handle(lgt, curr);
@@ -716,13 +705,10 @@ get_maptrack_handle(
lgt->maptrack_limit += MAPTRACK_PER_PAGE;
spin_unlock(&lgt->maptrack_lock);
- spin_lock(&curr->maptrack_freelist_lock);
-
- do {
- new_mt[i - 1].ref = read_atomic(&curr->maptrack_head);
- head = cmpxchg(&curr->maptrack_head, new_mt[i - 1].ref, handle + 1);
- } while ( head != new_mt[i - 1].ref );
+ spin_lock(&curr->maptrack_freelist_lock);
+ new_mt[i - 1].ref = curr->maptrack_head;
+ curr->maptrack_head = handle + 1;
spin_unlock(&curr->maptrack_freelist_lock);
return handle;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 3982167144c6..bd1cb08266d8 100644
--- 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
+ * the access to maptrack_head and maptrack_tail.
+ */
spinlock_t maptrack_freelist_lock;
unsigned int maptrack_head;
unsigned int maptrack_tail;
--
2.17.1
next reply other threads:[~2021-05-26 15:22 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-26 15:21 Julien Grall [this message]
2021-05-28 13:29 ` [PATCH] xen/grant-table: Simplify the update to the per-vCPU maptrack freelist Jan Beulich
2021-06-08 9:01 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210526152152.26251-1-julien@xen.org \
--to=julien@xen.org \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=jgrall@amazon.com \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).