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-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
