[PATCH] xen: Use wmb instead of rmb in xen_evtchn_do_upcall().

[PATCH] xen: Use wmb instead of rmb in xen_evtchn_do_upcall().

[Isaku Yamahata]

This patch is ported one from 534:77db69c38249 of linux-2.6.18-xen.hg.
Use wmb instead of rmb to enforce ordering between
evtchn_upcall_pending and evtchn_pending_sel stores
in xen_evtchn_do_upcall().

Cc: Samuel Thibault <samuel.thibault@eu.citrix.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 drivers/xen/events.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 73d78dc..332dd63 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -529,7 +529,7 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
 
 #ifndef CONFIG_X86 /* No need for a barrier -- XCHG is a barrier on x86. */
 		/* Clear master flag /before/ clearing selector flag. */
-		rmb();
+		wmb();
 #endif
 		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
 		while (pending_words != 0) {
-- 
1.5.3

Re: [PATCH] xen: Use wmb instead of rmb in xen_evtchn_do_upcall().

[Nick Piggin]

On Tuesday 10 June 2008 17:35, Isaku Yamahata wrote:
> This patch is ported one from 534:77db69c38249 of linux-2.6.18-xen.hg.
> Use wmb instead of rmb to enforce ordering between
> evtchn_upcall_pending and evtchn_pending_sel stores
> in xen_evtchn_do_upcall().

There are a whole load of places in the kernel that should be using
smp_ variants of memory barriers. This seemed to me like one of them,
but I could be wrong.

Also, if you do that can you get rid of the ifdef? If it really *really*
mattered, we could introduce smp_mb before/after xchg... but if you
use smp_wmb anyway then it definitely does not matter because that is a
noop on x86.

Thanks,
Nick

>
 Cc: Samuel Thibault <samuel.thibault@eu.citrix.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  drivers/xen/events.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 73d78dc..332dd63 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -529,7 +529,7 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
>
>  #ifndef CONFIG_X86 /* No need for a barrier -- XCHG is a barrier on x86.
> */ /* Clear master flag /before/ clearing selector flag. */
> -		rmb();
> +		wmb();
>  #endif
>  		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
>  		while (pending_words != 0) {

Re: [PATCH] xen: Use wmb instead of rmb in xen_evtchn_do_upcall().

[Jeremy Fitzhardinge]

Nick Piggin wrote:
> On Tuesday 10 June 2008 17:35, Isaku Yamahata wrote:
>   
>> This patch is ported one from 534:77db69c38249 of linux-2.6.18-xen.hg.
>> Use wmb instead of rmb to enforce ordering between
>> evtchn_upcall_pending and evtchn_pending_sel stores
>> in xen_evtchn_do_upcall().
>>     
>
> There are a whole load of places in the kernel that should be using
> smp_ variants of memory barriers. This seemed to me like one of them,
> but I could be wrong.
>   

No, it needs to be an unconditional barrier.  This is synchronizing with 
the hypervisor - even if the kernel is compiled UP, the SMP hypervisor 
may be testing/setting the events pending bits from another (physical) cpu.

> Also, if you do that can you get rid of the ifdef? If it really *really*
> mattered, we could introduce smp_mb before/after xchg... but if you
> use smp_wmb anyway then it definitely does not matter because that is a
> noop on x86.
>   

Yes, I'd like to lose the #ifdef.  Unfortunately I think putting a 
"lock; addl $0,0(%%esp)" style barrier had a measurable negative 
performance impact, but I may be thinking of something else.  I don't 
know how expensive sfence is.

The alternative is to make ia64's xchg a barrier (or to add a barrier 
variant of it).  It seems like a wart to have a cross-architecture 
function like xchg(), but then have different architectures differ in 
important details like barrier-ness.

    J

> Thanks,
> Nick
>
>   
>  Cc: Samuel Thibault <samuel.thibault@eu.citrix.com>
>   
>> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>> ---
>>  drivers/xen/events.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
>> index 73d78dc..332dd63 100644
>> --- a/drivers/xen/events.c
>> +++ b/drivers/xen/events.c
>> @@ -529,7 +529,7 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
>>
>>  #ifndef CONFIG_X86 /* No need for a barrier -- XCHG is a barrier on x86.
>> */ /* Clear master flag /before/ clearing selector flag. */
>> -		rmb();
>> +		wmb();
>>  #endif
>>  		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
>>  		while (pending_words != 0) {
>>     

Re: [PATCH] xen: Use wmb instead of rmb in xen_evtchn_do_upcall().

[Isaku Yamahata]

On Tue, Jun 10, 2008 at 05:41:39PM +1000, Nick Piggin wrote:
> On Tuesday 10 June 2008 17:35, Isaku Yamahata wrote:
> > This patch is ported one from 534:77db69c38249 of linux-2.6.18-xen.hg.
> > Use wmb instead of rmb to enforce ordering between
> > evtchn_upcall_pending and evtchn_pending_sel stores
> > in xen_evtchn_do_upcall().
> 
> There are a whole load of places in the kernel that should be using
> smp_ variants of memory barriers. This seemed to me like one of them,
> but I could be wrong.

Thank you for your comment.
Non smp_ version is intentionally used.
Because here the synchronization is done between other guest kernel
which might be running on another physical cpu. So non smp version is
really necessary even if this kernel is build for UP.


> Also, if you do that can you get rid of the ifdef? If it really *really*
> mattered, we could introduce smp_mb before/after xchg... but if you
> use smp_wmb anyway then it definitely does not matter because that is a
> noop on x86.

To be honest, I don't know whether the overhead of mb here matters
on x86 or not. I'm wondering Jeremy might have an idea.


>
> 
> Thanks,
> Nick
> 
> >
>  Cc: Samuel Thibault <samuel.thibault@eu.citrix.com>
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  drivers/xen/events.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index 73d78dc..332dd63 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -529,7 +529,7 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
> >
> >  #ifndef CONFIG_X86 /* No need for a barrier -- XCHG is a barrier on x86.
> > */ /* Clear master flag /before/ clearing selector flag. */
> > -		rmb();
> > +		wmb();
> >  #endif
> >  		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
> >  		while (pending_words != 0) {
> 

-- 
yamahata

Re: [PATCH] xen: Use wmb instead of rmb in xen_evtchn_do_upcall().

[Nick Piggin]

On Tuesday 10 June 2008 17:57, Jeremy Fitzhardinge wrote:
> Nick Piggin wrote:
> > On Tuesday 10 June 2008 17:35, Isaku Yamahata wrote:
> >> This patch is ported one from 534:77db69c38249 of linux-2.6.18-xen.hg.
> >> Use wmb instead of rmb to enforce ordering between
> >> evtchn_upcall_pending and evtchn_pending_sel stores
> >> in xen_evtchn_do_upcall().
> >
> > There are a whole load of places in the kernel that should be using
> > smp_ variants of memory barriers. This seemed to me like one of them,
> > but I could be wrong.
>
> No, it needs to be an unconditional barrier.  This is synchronizing with
> the hypervisor - even if the kernel is compiled UP, the SMP hypervisor
> may be testing/setting the events pending bits from another (physical) cpu.

OK. What you *really* want is smp_*mb_even_if_compiled_for_UP() ;)
That is, a small set of primitives that are compiled with CONFIG_SMP
(and given some xxx_ prefix to distinguish).

IO barriers are probably the best thing you can use for the moment.


> > Also, if you do that can you get rid of the ifdef? If it really *really*
> > mattered, we could introduce smp_mb before/after xchg... but if you
> > use smp_wmb anyway then it definitely does not matter because that is a
> > noop on x86.
>
> Yes, I'd like to lose the #ifdef.  Unfortunately I think putting a
> "lock; addl $0,0(%%esp)" style barrier had a measurable negative
> performance impact, but I may be thinking of something else.  I don't
> know how expensive sfence is.
>
> The alternative is to make ia64's xchg a barrier (or to add a barrier
> variant of it).  It seems like a wart to have a cross-architecture
> function like xchg(), but then have different architectures differ in
> important details like barrier-ness.

Well, no you have to be careful. Because even if we did ask for ia64's
xchg to be a full barrier, you wouldn't get the right behaviour on UP
because it would be free to optimise that away -- those kinds of barriers
referred to in all those primitives are defined for cacheable memory and
CPU-to-CPU only...

I guess under the circumstances, leaving the ifdef there is probably
reasonable.

Re: [PATCH] xen: Use wmb instead of rmb in xen_evtchn_do_upcall().

[Jeremy Fitzhardinge]

Nick Piggin wrote:
> On Tuesday 10 June 2008 17:57, Jeremy Fitzhardinge wrote:
>   
>> Nick Piggin wrote:
>>     
>>> On Tuesday 10 June 2008 17:35, Isaku Yamahata wrote:
>>>       
>>>> This patch is ported one from 534:77db69c38249 of linux-2.6.18-xen.hg.
>>>> Use wmb instead of rmb to enforce ordering between
>>>> evtchn_upcall_pending and evtchn_pending_sel stores
>>>> in xen_evtchn_do_upcall().
>>>>         
>>> There are a whole load of places in the kernel that should be using
>>> smp_ variants of memory barriers. This seemed to me like one of them,
>>> but I could be wrong.
>>>       
>> No, it needs to be an unconditional barrier.  This is synchronizing with
>> the hypervisor - even if the kernel is compiled UP, the SMP hypervisor
>> may be testing/setting the events pending bits from another (physical) cpu.
>>     
>
> OK. What you *really* want is smp_*mb_even_if_compiled_for_UP() ;)
> That is, a small set of primitives that are compiled with CONFIG_SMP
> (and given some xxx_ prefix to distinguish).
>   

We already have a set of sync_* for atomic ops which are always locked.

> IO barriers are probably the best thing you can use for the moment.
>   

It is conceptually similar, I suppose.

       J

[PATCH] xen: Use wmb instead of rmb in xen_evtchn_do_upcall().

[Jeremy Fitzhardinge]

From: Isaku Yamahata <yamahata@valinux.co.jp>

This patch is ported one from 534:77db69c38249 of linux-2.6.18-xen.hg.
Use wmb instead of rmb to enforce ordering between
evtchn_upcall_pending and evtchn_pending_sel stores
in xen_evtchn_do_upcall().

Cc: Samuel Thibault <samuel.thibault@eu.citrix.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 drivers/xen/events.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

===================================================================
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -529,7 +529,7 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
 
 #ifndef CONFIG_X86 /* No need for a barrier -- XCHG is a barrier on x86. */
 		/* Clear master flag /before/ clearing selector flag. */
-		rmb();
+		wmb();
 #endif
 		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
 		while (pending_words != 0) {
-- 
1.5.3