linux-kernel - Re: [PATCH net-next 2/4] atl1c: improve performance by avoiding unnecessary pcie writes on xmit

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <217ba8c9-ab09-46be-3e49-149f810e72fd@gmail.com>
Date:   Tue, 11 May 2021 23:39:37 +0200
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Gatis Peisenieks <gatis@...rotik.com>, chris.snook@...il.com,
        davem@...emloft.net, kuba@...nel.org, hkallweit1@...il.com,
        jesse.brandeburg@...el.com, dchickles@...vell.com,
        tully@...rotik.com, eric.dumazet@...il.com
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/4] atl1c: improve performance by avoiding
 unnecessary pcie writes on xmit



On 5/11/21 9:05 PM, Gatis Peisenieks wrote:
> The kernel has xmit_more facility that hints the networking driver xmit
> path about whether more packets are coming soon. This information can be
> used to avoid unnecessary expensive PCIe transaction per tx packet at a
> slight increase in latency.
> 
> Max TX pps on Mikrotik 10/25G NIC in a Threadripper 3960X system
> improved from 1150Kpps to 1700Kpps.
> 
> Signed-off-by: Gatis Peisenieks <gatis@...rotik.com>
> ---
>  drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 28c30d5288e4..2a8ab51b0ed9 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -2211,8 +2211,8 @@ static int atl1c_tx_map(struct atl1c_adapter *adapter,
>  	return -1;
>  }
>  
> -static void atl1c_tx_queue(struct atl1c_adapter *adapter, struct sk_buff *skb,
> -			   struct atl1c_tpd_desc *tpd, enum atl1c_trans_queue type)
> +static void atl1c_tx_queue(struct atl1c_adapter *adapter,
> +			   enum atl1c_trans_queue type)
>  {
>  	struct atl1c_tpd_ring *tpd_ring = &adapter->tpd_ring[type];
>  	u16 reg;
> @@ -2238,6 +2238,7 @@ static netdev_tx_t atl1c_xmit_frame(struct sk_buff *skb,
>  
>  	if (atl1c_tpd_avail(adapter, type) < tpd_req) {
>  		/* no enough descriptor, just stop queue */
> +		atl1c_tx_queue(adapter, type);
>  		netif_stop_queue(netdev);
>  		return NETDEV_TX_BUSY;
>  	}
> @@ -2246,6 +2247,7 @@ static netdev_tx_t atl1c_xmit_frame(struct sk_buff *skb,
>  
>  	/* do TSO and check sum */
>  	if (atl1c_tso_csum(adapter, skb, &tpd, type) != 0) {
> +		atl1c_tx_queue(adapter, type);
>  		dev_kfree_skb_any(skb);
>  		return NETDEV_TX_OK;
>  	}
> @@ -2270,8 +2272,11 @@ static netdev_tx_t atl1c_xmit_frame(struct sk_buff *skb,
>  		atl1c_tx_rollback(adapter, tpd, type);
>  		dev_kfree_skb_any(skb);
>  	} else {
> -		netdev_sent_queue(adapter->netdev, skb->len);
> -		atl1c_tx_queue(adapter, skb, tpd, type);
> +		bool more = netdev_xmit_more();
> +
> +		__netdev_sent_queue(adapter->netdev, skb->len, more);


This is probably buggy.

You must check and use the return code of this function,
as in :

	bool door_bell = __netdev_sent_queue(adapter->netdev, skb->len, netdev_xmit_more());

	if (door_bell)
		atl1c_tx_queue(adapter, type);


> +		if (!more)
> +			atl1c_tx_queue(adapter, type);
>  	}
>  
>  	return NETDEV_TX_OK;
>