[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 11 Nov 2013 16:01:22 +0530 (IST)
From: Govindarajulu Varadarajan <gvaradar@...co.com>
To: David Miller <davem@...emloft.net>
cc: govindarajulu90@...il.com, 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
On Mon, 4 Nov 2013, David Miller wrote:
> 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.
>
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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists