Its not exactly pointless.
Its a tradeoff between kernel code size, and ability for cpu to predict
a branch in kfree_skb()
This test is in hot path, and therefore this patch can potentially have
a performance impact.
I really think most kfree_skb() calls are done with a non NULL param,
so the branch prediction is good.
But after this patch, things are totally different.
Therefore, I am against it.
Its not exactly pointless.
Its a tradeoff between kernel code size, and ability for cpu to predict
a branch in kfree_skb()
This test is in hot path, and therefore this patch can potentially have
a performance impact.
I really think most kfree_skb() calls are done with a non NULL param,
so the branch prediction is good.
But after this patch, things are totally different.
But then the kfree_skb() is somewhat misleading because it does
check for NULL argument. One would have to remember if it's in
hot path or not. So, what about a new macro to pair with
kfree_skb()? That would document the code and would also
make easier to remember about the performance issue.
For instance:
/* kfree_skb() version to be used in hot code path
* as the branch prediction can improve performance
*/
#define kfree_skb_hot(skb) \
if (skb) \
kfree_skb(skb) \
fbl
Its not exactly pointless.
Its a tradeoff between kernel code size, and ability for cpu to predict
a branch in kfree_skb()
This test is in hot path, and therefore this patch can potentially have
a performance impact.
I really think most kfree_skb() calls are done with a non NULL param,
so the branch prediction is good.
But after this patch, things are totally different.
But then the kfree_skb() is somewhat misleading because it does
check for NULL argument. One would have to remember if it's in
hot path or not. So, what about a new macro to pair with
kfree_skb()? That would document the code and would also
make easier to remember about the performance issue.
For instance:
/* kfree_skb() version to be used in hot code path
* as the branch prediction can improve performance
*/
#define kfree_skb_hot(skb) \
if (skb) \
kfree_skb(skb) \
Really kfree_skb() is not misleading at all :
if (unlikely(!skb))
return;
So while its _perfectly_ valid to call kfree_skb(NULL), this code
expect callers to not abuse this facility.
And nf_conntrack_put_reasm() is called from skb_release_head_state()
We know in this code that most of the time, skb will be NULL.
I dont think we need to add another API for this case and see one
hundred patches coming _trying_ to use this new API.
Just review patches and shout if something bad happens.
Its not exactly pointless.
Its a tradeoff between kernel code size, and ability for cpu to predict
a branch in kfree_skb()
This test is in hot path, and therefore this patch can potentially have
a performance impact.
I really think most kfree_skb() calls are done with a non NULL param,
so the branch prediction is good.
But after this patch, things are totally different.
But then the kfree_skb() is somewhat misleading because it does
check for NULL argument. One would have to remember if it's in
hot path or not. So, what about a new macro to pair with
kfree_skb()? That would document the code and would also
make easier to remember about the performance issue.
For instance:
/* kfree_skb() version to be used in hot code path
* as the branch prediction can improve performance
*/
#define kfree_skb_hot(skb) \
if (skb) \
kfree_skb(skb) \
Really kfree_skb() is not misleading at all :
if (unlikely(!skb))
return;
So while its _perfectly_ valid to call kfree_skb(NULL), this code
expect callers to not abuse this facility.
Well, I don't think that is obvious. Neither the patch's author.
And nf_conntrack_put_reasm() is called from skb_release_head_state()
We know in this code that most of the time, skb will be NULL.
yeah, but it looks pointless to check the same thing twice.
I dont think we need to add another API for this case and see one
hundred patches coming _trying_ to use this new API.
Ok, and what if kfree_skb() becomes a macro that first checks
if the skb is NULL and if not, call the _kfree_skb() to
continue as before?
#define kfree_skb(skb) \
if (skb) \
_kfree_skb(skb) \
void _kfree_skb(struct sk_buff *skb)
{
if (likely(atomic_read(&skb->users) == 1))
smp_rmb();
else if (likely(!atomic_dec_and_test(&skb->users)))
return;
trace_kfree_skb(skb, __builtin_return_address(0));
__kfree_skb(skb);
}
Same API which would work for either use-cases. At the cost of
additional size in the binary.
Just review patches and shout if something bad happens.
I hope we always have you around to catch these cases :)
fbl
From: Eric Dumazet <hidden> Date: 2012-08-29 03:38:50
On Tue, 2012-08-28 at 17:39 -0300, Flavio Leitner wrote:
Ok, and what if kfree_skb() becomes a macro that first checks
if the skb is NULL and if not, call the _kfree_skb() to
continue as before?
#define kfree_skb(skb) \
if (skb) \
_kfree_skb(skb) \
Then its adding a conditional test on each call site and increase
kernel code size.
So if you plan submitting such patch, please keep the whole thing out of
line.
From: David Miller <davem@davemloft.net> Date: 2012-08-30 17:39:31
From: Eric Dumazet <redacted>
Date: Tue, 28 Aug 2012 20:38:48 -0700
On Tue, 2012-08-28 at 17:39 -0300, Flavio Leitner wrote:
quoted
Ok, and what if kfree_skb() becomes a macro that first checks
if the skb is NULL and if not, call the _kfree_skb() to
continue as before?
#define kfree_skb(skb) \
if (skb) \
_kfree_skb(skb) \
Then its adding a conditional test on each call site and increase
kernel code size.
So if you plan submitting such patch, please keep the whole thing out of
line.
I'm tossing this entire series.
Each and every case must be investigated individually and:
1) If the check is kept, a big comment explaining why is added
to the code.
2) If the check is removed, a big piece of explanatory text is
added to the commit log message explaining everything in
full detail.