lkml.org 
[lkml]   [2013]   [Nov]   [18]   [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
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
>
>


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