| Message ID | 20210206104932.29064-7-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: > The ring buffer for user events is used in the local system only, so > smp barriers are fine for ensuring consistency. > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Albeit I think "local system" is at least ambiguous (physical machine? VM?). How about something like "is local to the given kernel instance"? Jan
On 08.02.21 10:38, Jan Beulich wrote: > On 06.02.2021 11:49, Juergen Gross wrote: >> The ring buffer for user events is used in the local system only, so >> smp barriers are fine for ensuring consistency. >> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Albeit I think "local system" is at least ambiguous (physical > machine? VM?). How about something like "is local to the given > kernel instance"? Yes. Juergen
On 06/02/2021 10:49, Juergen Gross wrote: > The ring buffer for user events is used in the local system only, so > smp barriers are fine for ensuring consistency. > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Juergen Gross <jgross@suse.com> These need to be virt_* to not break in UP builds (on non-x86). ~Andrew
On 08.02.2021 10:44, Andrew Cooper wrote: > On 06/02/2021 10:49, Juergen Gross wrote: >> The ring buffer for user events is used in the local system only, so >> smp barriers are fine for ensuring consistency. >> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > These need to be virt_* to not break in UP builds (on non-x86). Initially I though so, too, but isn't the sole vCPU of such a VM getting re-scheduled to a different pCPU in the hypervisor an implied barrier anyway? Jan
On 08/02/2021 09:50, Jan Beulich wrote: > On 08.02.2021 10:44, Andrew Cooper wrote: >> On 06/02/2021 10:49, Juergen Gross wrote: >>> The ring buffer for user events is used in the local system only, so >>> smp barriers are fine for ensuring consistency. >>> >>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >> These need to be virt_* to not break in UP builds (on non-x86). > Initially I though so, too, but isn't the sole vCPU of such a > VM getting re-scheduled to a different pCPU in the hypervisor > an implied barrier anyway? Yes, but that isn't relevant to why UP builds break. smp_*() degrade to compiler barriers in UP builds, and while that's mostly fine for x86 read/write, its not fine for ARM barriers. virt_*() exist specifically to be smp_*() which don't degrade to broken in UP builds. ~Andrew
On 08.02.21 11:23, Andrew Cooper wrote: > On 08/02/2021 09:50, Jan Beulich wrote: >> On 08.02.2021 10:44, Andrew Cooper wrote: >>> On 06/02/2021 10:49, Juergen Gross wrote: >>>> The ring buffer for user events is used in the local system only, so >>>> smp barriers are fine for ensuring consistency. >>>> >>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> These need to be virt_* to not break in UP builds (on non-x86). >> Initially I though so, too, but isn't the sole vCPU of such a >> VM getting re-scheduled to a different pCPU in the hypervisor >> an implied barrier anyway? > > Yes, but that isn't relevant to why UP builds break. > > smp_*() degrade to compiler barriers in UP builds, and while that's > mostly fine for x86 read/write, its not fine for ARM barriers. > > virt_*() exist specifically to be smp_*() which don't degrade to broken > in UP builds. But the barrier is really only necessary to serialize accesses within the guest against each other. There is no guest outside party involved. In case you are right this would mean that UP guests are all broken on Arm. Juergen
On 08/02/2021 10:25, Jürgen Groß wrote: > On 08.02.21 11:23, Andrew Cooper wrote: >> On 08/02/2021 09:50, Jan Beulich wrote: >>> On 08.02.2021 10:44, Andrew Cooper wrote: >>>> On 06/02/2021 10:49, Juergen Gross wrote: >>>>> The ring buffer for user events is used in the local system only, so >>>>> smp barriers are fine for ensuring consistency. >>>>> >>>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> These need to be virt_* to not break in UP builds (on non-x86). >>> Initially I though so, too, but isn't the sole vCPU of such a >>> VM getting re-scheduled to a different pCPU in the hypervisor >>> an implied barrier anyway? >> >> Yes, but that isn't relevant to why UP builds break. >> >> smp_*() degrade to compiler barriers in UP builds, and while that's >> mostly fine for x86 read/write, its not fine for ARM barriers. >> >> virt_*() exist specifically to be smp_*() which don't degrade to broken >> in UP builds. > > But the barrier is really only necessary to serialize accesses within > the guest against each other. There is no guest outside party involved. > > In case you are right this would mean that UP guests are all broken on > Arm. Oh - right. This is a ring between the interrupt handler and a task. Not a ring between the guest and something else. In which case smp_*() are correct. Sorry for the noise. ~Andrew
On 08.02.2021 11:23, Andrew Cooper wrote: > On 08/02/2021 09:50, Jan Beulich wrote: >> On 08.02.2021 10:44, Andrew Cooper wrote: >>> On 06/02/2021 10:49, Juergen Gross wrote: >>>> The ring buffer for user events is used in the local system only, so >>>> smp barriers are fine for ensuring consistency. >>>> >>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> These need to be virt_* to not break in UP builds (on non-x86). >> Initially I though so, too, but isn't the sole vCPU of such a >> VM getting re-scheduled to a different pCPU in the hypervisor >> an implied barrier anyway? > > Yes, but that isn't relevant to why UP builds break. > > smp_*() degrade to compiler barriers in UP builds, and while that's > mostly fine for x86 read/write, its not fine for ARM barriers. Hmm, I may not know enough of Arm's memory model - are you saying Arm CPUs aren't even self-coherent, i.e. later operations (e.g. the consuming of ring contents) won't observe earlier ones (the updating of ring contents) when only a single physical CPU is involved in all of this? (I did mention the hypervisor level context switch simply because that's the only way multiple CPUs can get involved.) Jan
On 08/02/2021 10:36, Jan Beulich wrote: > On 08.02.2021 11:23, Andrew Cooper wrote: >> On 08/02/2021 09:50, Jan Beulich wrote: >>> On 08.02.2021 10:44, Andrew Cooper wrote: >>>> On 06/02/2021 10:49, Juergen Gross wrote: >>>>> The ring buffer for user events is used in the local system only, so >>>>> smp barriers are fine for ensuring consistency. >>>>> >>>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> These need to be virt_* to not break in UP builds (on non-x86). >>> Initially I though so, too, but isn't the sole vCPU of such a >>> VM getting re-scheduled to a different pCPU in the hypervisor >>> an implied barrier anyway? >> Yes, but that isn't relevant to why UP builds break. >> >> smp_*() degrade to compiler barriers in UP builds, and while that's >> mostly fine for x86 read/write, its not fine for ARM barriers. > Hmm, I may not know enough of Arm's memory model - are you saying > Arm CPUs aren't even self-coherent, i.e. later operations (e.g. > the consuming of ring contents) won't observe earlier ones (the > updating of ring contents) when only a single physical CPU is > involved in all of this? (I did mention the hypervisor level > context switch simply because that's the only way multiple CPUs > can get involved.) In this case, no - see my later reply. I'd mistaken the two ends of this ring. As they're both inside the same guest, its fine. For cases such as the xenstore/console ring, the semantics required really are SMP, even if the guest is built UP. These cases really will break when smp_rmb() etc degrade to just a compiler barrier on architectures with weaker semantics than x86. ~Andrew
diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c index a7a85719a8c8..421382c73d88 100644 --- a/drivers/xen/evtchn.c +++ b/drivers/xen/evtchn.c @@ -173,7 +173,7 @@ static irqreturn_t evtchn_interrupt(int irq, void *data) if ((u->ring_prod - u->ring_cons) < u->ring_size) { *evtchn_ring_entry(u, u->ring_prod) = evtchn->port; - wmb(); /* Ensure ring contents visible */ + smp_wmb(); /* Ensure ring contents visible */ if (u->ring_cons == u->ring_prod++) { wake_up_interruptible(&u->evtchn_wait); kill_fasync(&u->evtchn_async_queue, @@ -245,7 +245,7 @@ static ssize_t evtchn_read(struct file *file, char __user *buf, } rc = -EFAULT; - rmb(); /* Ensure that we see the port before we copy it. */ + smp_rmb(); /* Ensure that we see the port before we copy it. */ if (copy_to_user(buf, evtchn_ring_entry(u, c), bytes1) || ((bytes2 != 0) && copy_to_user(&buf[bytes1], &u->ring[0], bytes2)))
The ring buffer for user events is used in the local system only, so smp barriers are fine for ensuring consistency. Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/evtchn.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)