Messages in this thread |  | | Subject | Re: [PATCH 7/7] xen/evtchn: read producer index only once | From | Jürgen Groß <> | Date | Mon, 8 Feb 2021 13:15:29 +0100 |
| |
On 08.02.21 12:54, Jan Beulich wrote: > On 08.02.2021 11:59, Jürgen Groß wrote: >> On 08.02.21 11:51, Jan Beulich wrote: >>> On 08.02.2021 11:41, Jürgen Groß wrote: >>>> 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. >>> >>> I still wouldn't understand the change (and the lack of >>> further changes) then: The first further use of p is >>> outside the loop, alongside one of c. IOW why would c >>> then not need treating the same as p? >> >> Its value wouldn't change, as ring_cons is being modified only at >> the bottom of this function, and nowhere else (apart from the reset >> case, but this can't run concurrently due to ring_cons_mutex). >> >>> I also still don't see the difference between latching a >>> value into a local variable vs a "freestanding" access - >>> neither are guaranteed to result in exactly one memory >>> access afaict. >> >> READ_ONCE() is using a pointer to volatile, so any refetching by >> the compiler would be a bug. > > Of course, but this wasn't my point. I was contrasting > > c = u->ring_cons; > p = u->ring_prod; > > which you change with > > rc = wait_event_interruptible(u->evtchn_wait, > u->ring_cons != u->ring_prod); > > which you leave alone.
Can you point out which problem might arise from that?
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] |  |