[PATCH] net: Google gve: Remove dma_wmb() before ringing doorbell

[PATCH] net: Google gve: Remove dma_wmb() before ringing doorbell

[Liran Alon]

Current code use dma_wmb() to ensure Tx descriptors are visible
to device before writing to doorbell.

However, these dma_wmb() are wrong and unnecessary. Therefore,
they should be removed.

iowrite32be() called from gve_tx_put_doorbell() internally executes
dma_wmb()/wmb() on relevant architectures. E.g. On ARM, iowrite32be()
calls __iowmb() which translates to wmb() and only then executes
__raw_writel(). However on x86, iowrite32be() will call writel()
which just writes to memory because writes to UC memory is guaranteed
to be globally visible only after previous writes to WB memory are
globally visible.

Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 drivers/net/ethernet/google/gve/gve_tx.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_tx.c b/drivers/net/ethernet/google/gve/gve_tx.c
index f4889431f9b7..d0244feb0301 100644
--- a/drivers/net/ethernet/google/gve/gve_tx.c
+++ b/drivers/net/ethernet/google/gve/gve_tx.c
@@ -487,10 +487,6 @@ netdev_tx_t gve_tx(struct sk_buff *skb, struct net_device *dev)
 		 * may have added descriptors without ringing the doorbell.
 		 */
 
-		/* Ensure tx descs from a prior gve_tx are visible before
-		 * ringing doorbell.
-		 */
-		dma_wmb();
 		gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
 		return NETDEV_TX_BUSY;
 	}
@@ -505,8 +501,6 @@ netdev_tx_t gve_tx(struct sk_buff *skb, struct net_device *dev)
 	if (!netif_xmit_stopped(tx->netdev_txq) && netdev_xmit_more())
 		return NETDEV_TX_OK;
 
-	/* Ensure tx descs are visible before ringing doorbell */
-	dma_wmb();
 	gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
 	return NETDEV_TX_OK;
 }
-- 
2.20.1

Re: [PATCH] net: Google gve: Remove dma_wmb() before ringing doorbell

[Eric Dumazet]

On 1/3/20 5:01 AM, Liran Alon wrote:
> Current code use dma_wmb() to ensure Tx descriptors are visible
> to device before writing to doorbell.
> 
> However, these dma_wmb() are wrong and unnecessary. Therefore,
> they should be removed.
> 
> iowrite32be() called from gve_tx_put_doorbell() internally executes
> dma_wmb()/wmb() on relevant architectures. E.g. On ARM, iowrite32be()
> calls __iowmb() which translates to wmb() and only then executes
> __raw_writel(). However on x86, iowrite32be() will call writel()
> which just writes to memory because writes to UC memory is guaranteed
> to be globally visible only after previous writes to WB memory are
> globally visible.

This part about x86 is confusing.

writel() has the needed barrier (compared to writel_relaxed() which has no such barrier)

The UC vs WB memory sounds irrelevant.

> 
> Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  drivers/net/ethernet/google/gve/gve_tx.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_tx.c b/drivers/net/ethernet/google/gve/gve_tx.c
> index f4889431f9b7..d0244feb0301 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx.c
> @@ -487,10 +487,6 @@ netdev_tx_t gve_tx(struct sk_buff *skb, struct net_device *dev)
>  		 * may have added descriptors without ringing the doorbell.
>  		 */
>  
> -		/* Ensure tx descs from a prior gve_tx are visible before
> -		 * ringing doorbell.
> -		 */
> -		dma_wmb();
>  		gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
>  		return NETDEV_TX_BUSY;
>  	}
> @@ -505,8 +501,6 @@ netdev_tx_t gve_tx(struct sk_buff *skb, struct net_device *dev)
>  	if (!netif_xmit_stopped(tx->netdev_txq) && netdev_xmit_more())
>  		return NETDEV_TX_OK;
>  
> -	/* Ensure tx descs are visible before ringing doorbell */
> -	dma_wmb();
>  	gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
>  	return NETDEV_TX_OK;
>  }
> 

This seems strange to care about TX but not RX ?

gve_rx_write_doorbell() also uses iowrite32be()

Re: [PATCH] net: Google gve: Remove dma_wmb() before ringing doorbell

[Liran Alon]

> On 3 Jan 2020, at 15:36, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> 
> 
> On 1/3/20 5:01 AM, Liran Alon wrote:
>> Current code use dma_wmb() to ensure Tx descriptors are visible
>> to device before writing to doorbell.
>> 
>> However, these dma_wmb() are wrong and unnecessary. Therefore,
>> they should be removed.
>> 
>> iowrite32be() called from gve_tx_put_doorbell() internally executes
>> dma_wmb()/wmb() on relevant architectures. E.g. On ARM, iowrite32be()
>> calls __iowmb() which translates to wmb() and only then executes
>> __raw_writel(). However on x86, iowrite32be() will call writel()
>> which just writes to memory because writes to UC memory is guaranteed
>> to be globally visible only after previous writes to WB memory are
>> globally visible.
> 
> This part about x86 is confusing.

I disagree but I don’t mind removing it from commit message if you think so.
I think it emphasise that writel()/iowrite32be() does the right thing for the given architecture.

> 
> writel() has the needed barrier (compared to writel_relaxed() which has no such barrier)

Right.

> 
> The UC vs WB memory sounds irrelevant.

It is relevant. For example, writel()/iowrite32be() wasn’t sufficient in case Tx descriptors was in WC memory instead of WB memory.
(As for example done in AWS ENA LLQ or Mellanox mlx4 BlueFlame technologies).

i.e. writel() guarantees that all previous writes to UC/WB memory are globally visible before the write done by writel().
In our case, the Tx descriptors are in WB memory.

> 
>> 
>> Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>> drivers/net/ethernet/google/gve/gve_tx.c | 6 ------
>> 1 file changed, 6 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/google/gve/gve_tx.c b/drivers/net/ethernet/google/gve/gve_tx.c
>> index f4889431f9b7..d0244feb0301 100644
>> --- a/drivers/net/ethernet/google/gve/gve_tx.c
>> +++ b/drivers/net/ethernet/google/gve/gve_tx.c
>> @@ -487,10 +487,6 @@ netdev_tx_t gve_tx(struct sk_buff *skb, struct net_device *dev)
>> 		 * may have added descriptors without ringing the doorbell.
>> 		 */
>> 
>> -		/* Ensure tx descs from a prior gve_tx are visible before
>> -		 * ringing doorbell.
>> -		 */
>> -		dma_wmb();
>> 		gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
>> 		return NETDEV_TX_BUSY;
>> 	}
>> @@ -505,8 +501,6 @@ netdev_tx_t gve_tx(struct sk_buff *skb, struct net_device *dev)
>> 	if (!netif_xmit_stopped(tx->netdev_txq) && netdev_xmit_more())
>> 		return NETDEV_TX_OK;
>> 
>> -	/* Ensure tx descs are visible before ringing doorbell */
>> -	dma_wmb();
>> 	gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
>> 	return NETDEV_TX_OK;
>> }
>> 
> 
> This seems strange to care about TX but not RX ?
> 
> gve_rx_write_doorbell() also uses iowrite32be()
> 

You are right.
I missed that I should also remove dma_wmb() in gve_clean_rx_done() before calling gve_rx_write_doorbell().
Will fix it in v2.

Thanks,
-Liran