| Message ID | 20210206104932.29064-8-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/events: bug fixes and some diagnostic aids | expand |
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? Jan
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
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? 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. And of course there's also our beloved topic of access tearing here: READ_ONCE() also excludes that (at least as per its intentions aiui). Jan
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. > And of course there's also our beloved topic of access > tearing here: READ_ONCE() also excludes that (at least as > per its intentions aiui). Yes, but I don't see an urgent need to fix that, as there would be thousands of accesses in the kernel needing a fix. A compiler tearing a naturally aligned access into multiple memory accesses would be rejected as buggy from the kernel community IMO. Juergen
On 06/02/2021 10: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); For consistency, don't you also need the write side in evtchn_interrupt() to use WRITE_ONCE()? > if (c != p) > break; > >
On 08.02.21 12:40, Julien Grall wrote: > > > On 06/02/2021 10: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); > For consistency, don't you also need the write side in > evtchn_interrupt() to use WRITE_ONCE()? Only in case I'd consider the compiler needing multiple memory accesses for doing the update (see my reply to Jan's comment on this patch). Juergen
On 08/02/2021 10:59, Jürgen Groß wrote: > On 08.02.21 11:51, Jan Beulich wrote: > Yes, but I don't see an urgent need to fix that, as there would > be thousands of accesses in the kernel needing a fix. A compiler > tearing a naturally aligned access into multiple memory accesses > would be rejected as buggy from the kernel community IMO. I would not be so sure. From lwn [1]: "In the Linux kernel, tearing of plain C-language loads has been observed even given properly aligned and machine-word-sized loads.)" And for store tearing: "Note that this tearing can happen even on properly aligned and machine-word-sized accesses, and in this particular case, even for volatile stores. Some might argue that this behavior constitutes a bug in the compiler, but either way it illustrates the perceived value of store tearing from a compiler-writer viewpoint. [...] But for properly aligned machine-sized stores, WRITE_ONCE() will prevent store tearing." Cheers, [1] https://lwn.net/Articles/793253/#Load%20Tearing > > > Juergen
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. Jan
Hi Juergen, On 08/02/2021 11:48, Jürgen Groß wrote: > On 08.02.21 12:40, Julien Grall wrote: >> >> >> On 06/02/2021 10: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); >> For consistency, don't you also need the write side in >> evtchn_interrupt() to use WRITE_ONCE()? > > Only in case I'd consider the compiler needing multiple memory > accesses for doing the update (see my reply to Jan's comment on this > patch). Right, I have just answered there :). AFAICT, without using WRITE_ONCE()/READ_ONCE() there is no guarantee that load/store tearing will not happen. We can continue the conversation there. Cheers, > > Juergen
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
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. Jan
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
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;
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(-)