Messages in this thread |  | | From | Jürgen Groß <> | Subject | Re: [PATCH 7/7] xen/evtchn: read producer index only once | Date | Mon, 8 Feb 2021 11:41:08 +0100 |
| |
On 08.02.21 10:48, Jan Beulich wrote: > On 06.02.2021 11:49, Juergen Gross wrote: >> In evtchn_read() use READ_ONCE() for reading the producer index in >> order to avoid the compiler generating multiple accesses. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> drivers/xen/evtchn.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c >> index 421382c73d88..f6b199b597bf 100644 >> --- a/drivers/xen/evtchn.c >> +++ b/drivers/xen/evtchn.c >> @@ -211,7 +211,7 @@ static ssize_t evtchn_read(struct file *file, char __user *buf, >> goto unlock_out; >> >> c = u->ring_cons; >> - p = u->ring_prod; >> + p = READ_ONCE(u->ring_prod); >> if (c != p) >> break; > > Why only here and not also in > > rc = wait_event_interruptible(u->evtchn_wait, > u->ring_cons != u->ring_prod); > > or in evtchn_poll()? I understand it's not needed when > ring_prod_lock is held, but that's not the case in the two > afaics named places. Plus isn't the same then true for > ring_cons and ring_cons_mutex, i.e. aren't the two named > places plus evtchn_interrupt() also in need of READ_ONCE() > for ring_cons?
The problem solved here is the further processing using "p" multiple times. p must not be silently replaced with u->ring_prod by the compiler, so I probably should reword the commit message to say:
... in order to not allow the compiler to refetch p.
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] |  |