Messages in this thread | | | Date | Mon, 11 Nov 2013 16:01:22 +0530 (IST) | From | Govindarajulu Varadarajan <> | Subject | Re: [PATCH net-next 02/13] driver: net: remove unnecessary skb NULL check before calling dev_kfree_skb_irq |
| |
On Mon, 4 Nov 2013, David Miller wrote:
> From: Govindarajulu Varadarajan <govindarajulu90@gmail.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. >
True, in case of dev_kfree_skb_irq. If you look at patch 06-12, at many places we do
if (s->skb) { dev_kfree_skb_any(s->skb); s->skb = NULL) }
This is in fast path. If the code is not running in hardirq, dev_kfree_skb_any calls dev_kfree_skb. Which again check if skb is NULL. So we are checking if skb is null twice. That is what this patch is trying to fix. (sorry I should have mentioned this in cover letter).
I am not sure if you have read my previous mail. I am pasting it below.
>> On Sun, 3 Nov 2013, Brandeburg, Jesse wrote: >> Thanks for this work, I'm a little concerned that there is a >> non-trivial >> overhead to this patch. >> >> when doing (for example in the Intel drivers): >> if (s->skb) { >> dev_kfree_skb(s->skb); >> s->skb = NULL; >> } >> > >In current code, dev_kfree_skb is NULL safe. Which means skb is >checked for NULL inside dev_kfree_skb. dev_kfree_skb_any is also NULL safe >when the code is running in non-hardirq. > >Lets consider two cases > >1. skb is not NULL: > * Without my patch: > In the code above, we check for skb!=NULL twice. (once > before calling dev_kfree_skb, once by dev_kfree_skb). And > then we do assignment. > * With this patch: > we check for skb!=NULL once, And then we do assignment. > > To fix the twice NULL check, we either have to remove the check > which is inside dev_kfree_skb (1). Or do whats done in this > patch. > > (1) is not an option because a lot of kernel code already > assumes that dev_kfree_skb is NULL safe. > >2. skb is NULL: > * Without this patch: > One if statement is executed. > * With this patch: > One if statement and one assignment is executed. > > From my observation most of the dev_kfree_skb calls are from > e1000_unmap_and_free_tx_resource, e1000_put_txbuf, > atl1_clean_tx_ring, alx_free_txbuf etc. in clean up functions. > > Is is quite unlikely thats skb is NULL. So it comes down to one extra > if-branching statement or one extra assignment. I would prefer extra > assignment to branching statement. In my opinion extra assignment is > very little price we pay. > > //govind
Another way to solve the double NULL check is to define a new function something like this
dev_kfree_skb_NULL(struct sk_buff **skb) { if(*skb) { free_skb(*skb); *skb=NULL; } }
and use this if you want to free a skb and make it NULL. Is this approach better?
//govind
| |