netdev - Re: [PATCH net-next 02/13] driver: net: remove unnecessary skb NULL check before calling dev_kfree_skb

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:	Mon, 04 Nov 2013 15:12:30 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	govindarajulu90@...il.com
Cc:	gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, schwidefsky@...ibm.com,
	linville@...driver.com, linux-wireless@...r.kernel.org,
	netdev@...r.kernel.org, IvDoorn@...il.com, sbhatewara@...are.com,
	samuel@...tiz.org, chas@....nrl.navy.mil, roland@...nel.org,
	isdn@...ux-pingi.de, jcliburn@...il.com, benve@...co.com,
	ssujith@...co.com, jeffrey.t.kirsher@...el.com,
	jesse.brandeburg@...el.com, shahed.shaikh@...gic.com,
	joe@...ches.com, apw@...onical.com
Subject: Re: [PATCH net-next 02/13] driver: net: remove unnecessary skb
 NULL check before calling dev_kfree_skb_irq

From: Govindarajulu Varadarajan <govindarajulu90@...il.com>
Date: Sat,  2 Nov 2013 19:17:43 +0530

> @@ -1030,10 +1030,8 @@ static void ni65_xmit_intr(struct net_device *dev,int csr0)
>  		}
>  
>  #ifdef XMT_VIA_SKB
> -		if(p->tmd_skb[p->tmdlast]) {
> -			 dev_kfree_skb_irq(p->tmd_skb[p->tmdlast]);
> -			 p->tmd_skb[p->tmdlast] = NULL;
> -		}
> +		 dev_kfree_skb_irq(p->tmd_skb[p->tmdlast]);
> +		 p->tmd_skb[p->tmdlast] = NULL;
>  #endif

I absolutely disagree with this kind of change.

There is a non-trivial cost for NULL'ing out that array entry
unconditionally.  It's a dirtied cache line and this is in the
fast path of TX SKB reclaim of this driver.

You've made several changes of this kind.

And it sort-of shows that the places that do check for NULL,
are getting something in return for that test, namely avoidance
of an unnecessary cpu store in the fast path of the driver.

I'm throwing away this series, sorry.
--
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