Messages in this thread | | | Date | Tue, 19 Nov 2013 00:56:48 +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 |
| |
Hi Dave
Did you have a chance to look at this? Let me know how you want me to fix this.
//govind
On Mon, 11 Nov 2013, Govindarajulu Varadarajan wrote:
> > > 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 > >
| |