Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1573297 > unrolled thread

[PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable

Started byShannon Nelson <shannon.nelson@oracle.com>
First post2017-02-03 18:50 +0100
Last post2017-02-04 23:40 +0100
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable Shannon Nelson <shannon.nelson@oracle.com> - 2017-02-03 18:50 +0100
    Re: [PATCH net-next 5/9] sunvnet: add memory barrier before check  for tx enable Eric Dumazet <eric.dumazet@gmail.com> - 2017-02-03 19:00 +0100
      Re: [PATCH net-next 5/9] sunvnet: add memory barrier before check for  tx enable Shannon Nelson <shannon.nelson@oracle.com> - 2017-02-03 22:30 +0100
        Re: [PATCH net-next 5/9] sunvnet: add memory barrier before check  for tx enable David Miller <davem@davemloft.net> - 2017-02-03 23:00 +0100
        Re: [PATCH net-next 5/9] sunvnet: add memory barrier before check  for tx enable Eric Dumazet <eric.dumazet@gmail.com> - 2017-02-03 23:20 +0100
          Re: [PATCH net-next 5/9] sunvnet: add memory barrier before check for  tx enable Shannon Nelson <shannon.nelson@oracle.com> - 2017-02-04 23:40 +0100

#1573297 — [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable

FromShannon Nelson <shannon.nelson@oracle.com>
Date2017-02-03 18:50 +0100
Subject[PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable
Message-ID<t6Q9H-4Ln-15@gated-at.bofh.it>
In order to allow the underlying LDC and outstanding memory operations
to potentially catch up with the driver's Tx requests, add a memory
barrier before checking again for available tx descriptors.

Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
---
 drivers/net/ethernet/sun/sunvnet_common.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
index 5d0d386..98e758e 100644
--- a/drivers/net/ethernet/sun/sunvnet_common.c
+++ b/drivers/net/ethernet/sun/sunvnet_common.c
@@ -1467,6 +1467,7 @@ ldc_start_done:
 	dr->prod = (dr->prod + 1) & (VNET_TX_RING_SIZE - 1);
 	if (unlikely(vnet_tx_dring_avail(dr) < 1)) {
 		netif_tx_stop_queue(txq);
+		dma_wmb();
 		if (vnet_tx_dring_avail(dr) > VNET_TX_WAKEUP_THRESH(dr))
 			netif_tx_wake_queue(txq);
 	}
-- 
1.7.1

[toc] | [next] | [standalone]

#1573310 — Re: [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable

FromEric Dumazet <eric.dumazet@gmail.com>
Date2017-02-03 19:00 +0100
SubjectRe: [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable
Message-ID<t6Qjo-4OT-17@gated-at.bofh.it>
In reply to#1573297
On Fri, 2017-02-03 at 09:42 -0800, Shannon Nelson wrote:
> In order to allow the underlying LDC and outstanding memory operations
> to potentially catch up with the driver's Tx requests, add a memory
> barrier before checking again for available tx descriptors.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> ---
>  drivers/net/ethernet/sun/sunvnet_common.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
> index 5d0d386..98e758e 100644
> --- a/drivers/net/ethernet/sun/sunvnet_common.c
> +++ b/drivers/net/ethernet/sun/sunvnet_common.c
> @@ -1467,6 +1467,7 @@ ldc_start_done:
>  	dr->prod = (dr->prod + 1) & (VNET_TX_RING_SIZE - 1);
>  	if (unlikely(vnet_tx_dring_avail(dr) < 1)) {
>  		netif_tx_stop_queue(txq);
> +		dma_wmb();

This does not look right.

I believe you need smp_rmb() here.

>  		if (vnet_tx_dring_avail(dr) > VNET_TX_WAKEUP_THRESH(dr))
>  			netif_tx_wake_queue(txq);
>  	}

[toc] | [prev] | [next] | [standalone]

#1573452 — Re: [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable

FromShannon Nelson <shannon.nelson@oracle.com>
Date2017-02-03 22:30 +0100
SubjectRe: [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable
Message-ID<t6TAB-7il-13@gated-at.bofh.it>
In reply to#1573310
On 2/3/2017 9:56 AM, Eric Dumazet wrote:
> On Fri, 2017-02-03 at 09:42 -0800, Shannon Nelson wrote:
>> In order to allow the underlying LDC and outstanding memory operations
>> to potentially catch up with the driver's Tx requests, add a memory
>> barrier before checking again for available tx descriptors.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>> ---
>>  drivers/net/ethernet/sun/sunvnet_common.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
>> index 5d0d386..98e758e 100644
>> --- a/drivers/net/ethernet/sun/sunvnet_common.c
>> +++ b/drivers/net/ethernet/sun/sunvnet_common.c
>> @@ -1467,6 +1467,7 @@ ldc_start_done:
>>  	dr->prod = (dr->prod + 1) & (VNET_TX_RING_SIZE - 1);
>>  	if (unlikely(vnet_tx_dring_avail(dr) < 1)) {
>>  		netif_tx_stop_queue(txq);
>> +		dma_wmb();
>
> This does not look right.
>
> I believe you need smp_rmb() here.

Well, it probably should be dma_rmb(), since regardless of the number of 
cores we think we have, we're communicating with a peer ldom that has 
its own core(s).  Either way, on sparc they all seem to boil down to the 
same bit of asm, but using the "rmb" part makes more logical sense. 
I'll respin with dma_rmb().

Good catch, thanks,
sln


>
>>  		if (vnet_tx_dring_avail(dr) > VNET_TX_WAKEUP_THRESH(dr))
>>  			netif_tx_wake_queue(txq);
>>  	}
>
>

[toc] | [prev] | [next] | [standalone]

#1573472 — Re: [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable

FromDavid Miller <davem@davemloft.net>
Date2017-02-03 23:00 +0100
SubjectRe: [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable
Message-ID<t6U3E-7uQ-21@gated-at.bofh.it>
In reply to#1573452
From: Shannon Nelson <shannon.nelson@oracle.com>
Date: Fri, 3 Feb 2017 13:20:43 -0800

> On 2/3/2017 9:56 AM, Eric Dumazet wrote:
>> On Fri, 2017-02-03 at 09:42 -0800, Shannon Nelson wrote:
>>> In order to allow the underlying LDC and outstanding memory operations
>>> to potentially catch up with the driver's Tx requests, add a memory
>>> barrier before checking again for available tx descriptors.
>>>
>>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>>> ---
>>>  drivers/net/ethernet/sun/sunvnet_common.c |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/sun/sunvnet_common.c
>>> b/drivers/net/ethernet/sun/sunvnet_common.c
>>> index 5d0d386..98e758e 100644
>>> --- a/drivers/net/ethernet/sun/sunvnet_common.c
>>> +++ b/drivers/net/ethernet/sun/sunvnet_common.c
>>> @@ -1467,6 +1467,7 @@ ldc_start_done:
>>>  	dr->prod = (dr->prod + 1) & (VNET_TX_RING_SIZE - 1);
>>>  	if (unlikely(vnet_tx_dring_avail(dr) < 1)) {
>>>  		netif_tx_stop_queue(txq);
>>> +		dma_wmb();
>>
>> This does not look right.
>>
>> I believe you need smp_rmb() here.
> 
> Well, it probably should be dma_rmb(), since regardless of the number
> of cores we think we have, we're communicating with a peer ldom that
> has its own core(s).  Either way, on sparc they all seem to boil down
> to the same bit of asm, but using the "rmb" part makes more logical
> sense. I'll respin with dma_rmb().

DMA barriers are for ordering between CPUs and devices.

SMP barriers are for ordering between CPUs, which is your situation
here.

It is completely inappropriate to use DMA barriers in a virutalization
device driver.

[toc] | [prev] | [next] | [standalone]

#1573480 — Re: [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable

FromEric Dumazet <eric.dumazet@gmail.com>
Date2017-02-03 23:20 +0100
SubjectRe: [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable
Message-ID<t6UmZ-7Sg-9@gated-at.bofh.it>
In reply to#1573452
On Fri, 2017-02-03 at 13:20 -0800, Shannon Nelson wrote:
> On 2/3/2017 9:56 AM, Eric Dumazet wrote:
> > On Fri, 2017-02-03 at 09:42 -0800, Shannon Nelson wrote:
> >> In order to allow the underlying LDC and outstanding memory operations
> >> to potentially catch up with the driver's Tx requests, add a memory
> >> barrier before checking again for available tx descriptors.
> >>
> >> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> >> ---
> >>  drivers/net/ethernet/sun/sunvnet_common.c |    1 +
> >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/sun/sunvnet_common.c b/drivers/net/ethernet/sun/sunvnet_common.c
> >> index 5d0d386..98e758e 100644
> >> --- a/drivers/net/ethernet/sun/sunvnet_common.c
> >> +++ b/drivers/net/ethernet/sun/sunvnet_common.c
> >> @@ -1467,6 +1467,7 @@ ldc_start_done:
> >>  	dr->prod = (dr->prod + 1) & (VNET_TX_RING_SIZE - 1);
> >>  	if (unlikely(vnet_tx_dring_avail(dr) < 1)) {
> >>  		netif_tx_stop_queue(txq);
> >> +		dma_wmb();
> >
> > This does not look right.
> >
> > I believe you need smp_rmb() here.
> 
> Well, it probably should be dma_rmb(), since regardless of the number of 
> cores we think we have, we're communicating with a peer ldom that has 
> its own core(s).  Either way, on sparc they all seem to boil down to the 
> same bit of asm, but using the "rmb" part makes more logical sense. 
> I'll respin with dma_rmb().
> 

Transmit completion might happen on another cpu, regardless of ldom.

Therefore you need smp_rmb() here ( like mellanox/mlx4/en_tx.c) , or
even smp_mb() as bnx2x does.

dma_rmb() is never used in this context.

[toc] | [prev] | [next] | [standalone]

#1573766 — Re: [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable

FromShannon Nelson <shannon.nelson@oracle.com>
Date2017-02-04 23:40 +0100
SubjectRe: [PATCH net-next 5/9] sunvnet: add memory barrier before check for tx enable
Message-ID<t7h9T-75V-11@gated-at.bofh.it>
In reply to#1573480
On 2/3/2017 2:11 PM, Eric Dumazet wrote:
>
> Transmit completion might happen on another cpu, regardless of ldom.
>
> Therefore you need smp_rmb() here ( like mellanox/mlx4/en_tx.c) , or
> even smp_mb() as bnx2x does.
>
> dma_rmb() is never used in this context.
>

In that case, it looks like there are a couple other similar issues in 
this code that need attention, that cropped up when the new dma_*mb() 
interface was added.  I'll see what I can do with those as well.

The comments and code a few lines above, some of DaveM's original driver 
code, seem to dissuade us from the SMP version and originally used the 
bare wmb().  Perhaps the bare rmb() should be used here?  Again, I 
suppose it doesn't matter much as it looks like they all boil down to 
the same bit of asm, at least on sparc, which is all that matters for 
this driver.

Thanks,
sln

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web