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:26:25 +0100 |
| |
On 08.02.21 13:23, Jan Beulich wrote: > On 08.02.2021 13:15, Jürgen Groß wrote: >> 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? > > Not any particular active one. Yet enhancing some accesses > but not others seems to me like a recipe for new problems > down the road.
I already reasoned that the usage of READ_ONCE() is due to storing the value in a local variable which needs to be kept constant during the following processing (no refetches by the compiler). This reasoning very clearly doesn't apply to the other accesses.
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] |  |