netdev - Re: [PATCH 4/4] bnx2x: merge common code when stopping queue

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]

Date:	Wed, 10 Mar 2010 18:02:06 +0200
From:	"Eilon Greenstein" <eilong@...adcom.com>
To:	"Stanislaw Gruszka" <sgruszka@...hat.com>
cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Vladislav Zolotarov" <vladz@...adcom.com>,
	"Michael Chan" <mchan@...adcom.com>
Subject: Re: [PATCH 4/4] bnx2x: merge common code when stopping queue

On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote:
> Main reason for merge code is willingness to use memory barrier to avoid
> races with bnx2x_tx_int(). There is no real situation possible to
> trigger the races. But if we assume that bug of not stopped and full
> queue can happen, we should also stop queue then in the safe way.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@...hat.com>
> ---
> I have no strong fillings that patch should be applied, if Vladislav
> do not want the patch, I'll will not argue.
> 
Stanislaw,

As you wrote, there is no real situation possible to trigger this race.
So basically, you are changing the code from “return” on error to “go
to”  and in the “go to” branch you add some harmless but useless code in
the form of smp_mb and testing for a possible wake again. I find the
code to be less readable after this change and harder to maintain so I
prefer not to include this code change.

Thanks for all the changes and you participant in improving the bnx2x!

Eilon

>  drivers/net/bnx2x_main.c |   37 ++++++++++++++++++++-----------------
>  1 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> index 6c042a7..6751ca2 100644
> --- a/drivers/net/bnx2x_main.c
> +++ b/drivers/net/bnx2x_main.c
> @@ -11176,10 +11176,9 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	struct eth_tx_bd *tx_data_bd, *total_pkt_bd = NULL;
>  	struct eth_tx_parse_bd *pbd = NULL;
>  	u16 pkt_prod, bd_prod;
> -	int nbd, fp_index;
> +	int i, ret, nbd, fp_index;
>  	dma_addr_t mapping;
>  	u32 xmit_type = bnx2x_xmit_type(bp, skb);
> -	int i;
>  	u8 hlen = 0;
>  	__le16 pkt_size = 0;
>  
> @@ -11194,10 +11193,9 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	fp = &bp->fp[fp_index];
>  
>  	if (unlikely(bnx2x_tx_avail(fp) < (skb_shinfo(skb)->nr_frags + 3))) {
> -		fp->eth_q_stats.driver_xoff++;
> -		netif_tx_stop_queue(txq);
>  		BNX2X_ERR("BUG! Tx ring full when queue awake!\n");
> -		return NETDEV_TX_BUSY;
> +		ret = NETDEV_TX_BUSY;
> +		goto stop_queue;
>  	}
>  
>  	DP(NETIF_MSG_TX_QUEUED, "SKB: summed %x  protocol %x  protocol(%x,%x)"
> @@ -11425,22 +11423,27 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	mmiowb();
>  
>  	fp->tx_bd_prod += nbd;
> +	fp->tx_pkt++;
>  
> -	if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3)) {
> -		netif_tx_stop_queue(txq);
> +	ret = NETDEV_TX_OK;
> +	if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3))
> +		goto stop_queue;
>  
> -		/* paired memory barrier is in bnx2x_tx_int(), we have to keep
> -		 * ordering of set_bit() in netif_tx_stop_queue() and read of
> -		 * fp->bd_tx_cons */
> -		smp_mb();
> +	return ret;
>  
> -		fp->eth_q_stats.driver_xoff++;
> -		if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3)
> -			netif_tx_wake_queue(txq);
> -	}
> -	fp->tx_pkt++;
> +stop_queue:
> +	netif_tx_stop_queue(txq);
> +
> +	/* paired memory barrier is in bnx2x_tx_int(), we have to keep
> +	 * ordering of set_bit() in netif_tx_stop_queue() and read of
> +	 * fp->bd_tx_cons */
> +	smp_mb();
>  
> -	return NETDEV_TX_OK;
> +	fp->eth_q_stats.driver_xoff++;
> +	if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3)
> +		netif_tx_wake_queue(txq);
> +
> +	return ret;
>  }
>  
>  /* called with rtnl_lock */



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html