netdev - Re: [PATCH] net: Google gve: Remove dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 3 Jan 2020 18:19:16 +0200
From:   Liran Alon <liran.alon@...cle.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     csully@...gle.com, davem@...emloft.net, netdev@...r.kernel.org,
        sagis@...gle.com, jonolson@...gle.com, yangchun@...gle.com,
        lrizzo@...gle.com, adisuresh@...gle.com,
        Si-Wei Liu <si-wei.liu@...cle.com>
Subject: Re: [PATCH] net: Google gve: Remove dma_wmb() before ringing doorbell



> On 3 Jan 2020, at 15:36, Eric Dumazet <eric.dumazet@...il.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@...cle.com>
>> Signed-off-by: Liran Alon <liran.alon@...cle.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