Re: [PATCH net-next 02/13] driver: net: remove unnecessary skb NULLcheck before calling dev_kfree_skb_irq

[Govindarajulu Varadarajan]

On Mon, 4 Nov 2013, David Miller wrote:

> From: Govindarajulu Varadarajan <govindarajulu90@xxxxxxxxx>
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/