linux-kernel - Re: [PATCH 1/3] net: fs_enet: merge NAPI RX and NAPI TX

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: <1472044021.14381.71.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Wed, 24 Aug 2016 06:07:01 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Christophe Leroy <christophe.leroy@....fr>
Cc:     Pantelis Antoniou <pantelis.antoniou@...il.com>,
        Vitaly Bordug <vbordug@...mvista.com>, davem@...emloft.net,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH 1/3] net: fs_enet: merge NAPI RX and NAPI TX

On Wed, 2016-08-24 at 12:36 +0200, Christophe Leroy wrote:
> Initially, a NAPI TX routine has been implemented separately from
> NAPI RX, as done on the freescale/gianfar driver.
> 
> By merging NAPI RX and NAPI TX, we reduce the amount of TX completion
> interrupts.
> 
> Handling of the budget in association with TX interrupts is based on
> indications provided at https://wiki.linuxfoundation.org/networking/napi
> 
> At the same time, we fix an issue in the handling of fep->tx_free:
> 
> It is only when fep->tx_free goes up to MAX_SKB_FRAGS that
> we need to wake up the queue. There is no need to call
> netif_wake_queue() at every packet successfully transmitted.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@....fr>
> ---
>  .../net/ethernet/freescale/fs_enet/fs_enet-main.c  | 259 +++++++++------------
>  drivers/net/ethernet/freescale/fs_enet/fs_enet.h   |  16 +-
>  drivers/net/ethernet/freescale/fs_enet/mac-fcc.c   |  57 ++---
>  drivers/net/ethernet/freescale/fs_enet/mac-fec.c   |  57 ++---
>  drivers/net/ethernet/freescale/fs_enet/mac-scc.c   |  57 ++---
>  5 files changed, 153 insertions(+), 293 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> index 61fd486..7cd3ef9 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> @@ -79,8 +79,8 @@ static void skb_align(struct sk_buff *skb, int align)
>  		skb_reserve(skb, align - off);
>  }
>  
> -/* NAPI receive function */
> -static int fs_enet_rx_napi(struct napi_struct *napi, int budget)
> +/* NAPI function */
> +static int fs_enet_napi(struct napi_struct *napi, int budget)
>  {
>  	struct fs_enet_private *fep = container_of(napi, struct fs_enet_private, napi);
>  	struct net_device *dev = fep->ndev;
> @@ -90,9 +90,100 @@ static int fs_enet_rx_napi(struct napi_struct *napi, int budget)
>  	int received = 0;
>  	u16 pkt_len, sc;
>  	int curidx;
> +	int dirtyidx, do_wake, do_restart;
>  
> -	if (budget <= 0)
> -		return received;
> +	spin_lock(&fep->tx_lock);
> +	bdp = fep->dirty_tx;
> +
> +	/* clear status bits for napi*/
> +	(*fep->ops->napi_clear_event)(dev);
> +
> +	do_wake = do_restart = 0;
> +	while (((sc = CBDR_SC(bdp)) & BD_ENET_TX_READY) == 0) {

I am afraid you could live lock here on SMP.

You should make sure you do not loop forever, not assuming cpu is faster
than NIC.



> +		dirtyidx = bdp - fep->tx_bd_base;
> +
> +		if (fep->tx_free == fep->tx_ring)
> +			break;
> +
> +		skb = fep->tx_skbuff[dirtyidx];
> +
> +		/*
> +		 * Check for errors.
> +		 */
> +		if (sc & (BD_ENET_TX_HB | BD_ENET_TX_LC |
> +			  BD_ENET_TX_RL | BD_ENET_TX_UN | BD_ENET_TX_CSL)) {
> +
> +			if (sc & BD_ENET_TX_HB)	/* No heartbeat */
> +				fep->stats.tx_heartbeat_errors++;
> +			if (sc & BD_ENET_TX_LC)	/* Late collision */
> +				fep->stats.tx_window_errors++;
> +			if (sc & BD_ENET_TX_RL)	/* Retrans limit */
> +				fep->stats.tx_aborted_errors++;
> +			if (sc & BD_ENET_TX_UN)	/* Underrun */
> +				fep->stats.tx_fifo_errors++;
> +			if (sc & BD_ENET_TX_CSL)	/* Carrier lost */
> +				fep->stats.tx_carrier_errors++;
> +
> +			if (sc & (BD_ENET_TX_LC | BD_ENET_TX_RL | BD_ENET_TX_UN)) {
> +				fep->stats.tx_errors++;
> +				do_restart = 1;
> +			}
> +		} else
> +			fep->stats.tx_packets++;
> +
> +		if (sc & BD_ENET_TX_READY) {
> +			dev_warn(fep->dev,
> +				 "HEY! Enet xmit interrupt and TX_READY.\n");
> +		}
> +
> +		/*
> +		 * Deferred means some collisions occurred during transmit,
> +		 * but we eventually sent the packet OK.
> +		 */
> +		if (sc & BD_ENET_TX_DEF)
> +			fep->stats.collisions++;
> +
> +		/* unmap */
> +		if (fep->mapped_as_page[dirtyidx])
> +			dma_unmap_page(fep->dev, CBDR_BUFADDR(bdp),
> +				       CBDR_DATLEN(bdp), DMA_TO_DEVICE);
> +		else
> +			dma_unmap_single(fep->dev, CBDR_BUFADDR(bdp),
> +					 CBDR_DATLEN(bdp), DMA_TO_DEVICE);
> +
> +		/*
> +		 * Free the sk buffer associated with this last transmit.
> +		 */
> +		if (skb) {
> +			dev_kfree_skb(skb);
> +			fep->tx_skbuff[dirtyidx] = NULL;
> +		}
> +
> +		/*
> +		 * Update pointer to next buffer descriptor to be transmitted.
> +		 */
> +		if ((sc & BD_ENET_TX_WRAP) == 0)
> +			bdp++;
> +		else
> +			bdp = fep->tx_bd_base;
> +
> +		/*
> +		 * Since we have freed up a buffer, the ring is no longer
> +		 * full.
> +		 */
> +		if (++fep->tx_free == MAX_SKB_FRAGS)
> +			do_wake = 1;
> +	}
> +
> +	fep->dirty_tx = bdp;
> +
> +	if (do_restart)
> +		(*fep->ops->tx_restart)(dev);
> +
> +	spin_unlock(&fep->tx_lock);
> +
> +	if (do_wake)
> +		netif_wake_queue(dev);
>