lkml.org 
[lkml]   [2013]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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



    \
     
     \ /
      Last update: 2013-11-11 11:41    [W:2.261 / U:0.436 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site