* [PATCH] netfilter: remove redundant skb check in nf_conntrack_put_reasm
@ 2013-06-19 14:27 Phil Oester
2013-06-20 9:22 ` Florian Westphal
2013-06-20 9:49 ` Pablo Neira Ayuso
0 siblings, 2 replies; 6+ messages in thread
From: Phil Oester @ 2013-06-19 14:27 UTC (permalink / raw)
To: netfilter-devel; +Cc: pablo
[-- Attachment #1: Type: text/plain, Size: 155 bytes --]
kfree_skb already checks for skb existence - remove redundant check from
nf_conntrack_put_reasm.
Phil
Signed-off-by: Phil Oester <kernel@linuxace.com>
[-- Attachment #2: patch-put_reasm --]
[-- Type: text/plain, Size: 395 bytes --]
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a7393ad..3da70ba 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2604,8 +2604,7 @@ static inline void nf_conntrack_get_reasm(struct sk_buff *skb)
}
static inline void nf_conntrack_put_reasm(struct sk_buff *skb)
{
- if (skb)
- kfree_skb(skb);
+ kfree_skb(skb);
}
#endif
#ifdef CONFIG_BRIDGE_NETFILTER
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: remove redundant skb check in nf_conntrack_put_reasm
2013-06-20 9:58 ` Pablo Neira Ayuso
@ 2013-06-20 0:01 ` Phil Oester
2013-06-20 14:15 ` Florian Westphal
0 siblings, 1 reply; 6+ messages in thread
From: Phil Oester @ 2013-06-20 0:01 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel
On Thu, Jun 20, 2013 at 11:58:46AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jun 20, 2013 at 11:22:59AM +0200, Florian Westphal wrote:
> > Phil Oester <kernel@linuxace.com> wrote:
> > > kfree_skb already checks for skb existence - remove redundant check from
> > > nf_conntrack_put_reasm.
> >
> > This is called for every free'd skb, in almost all cases
> > the condition will be false.
> >
> > So I think its ok to keep this check.
>
> I see, we'll hit branch mispredictions most of the time. I have kept
> this back. Thanks Florian.
If we're concerned about branch mispredictions, we should change the check to
match the one at the top of kfree_skb:
if (unlikely(!skb))
return;
However nf_conntrack_put_reasm is only used in 3 places in the entire tree.
Unclear why we don't just call kfree_skb directly in these 3 places.
Phil
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: remove redundant skb check in nf_conntrack_put_reasm
2013-06-19 14:27 [PATCH] netfilter: remove redundant skb check in nf_conntrack_put_reasm Phil Oester
@ 2013-06-20 9:22 ` Florian Westphal
2013-06-20 9:58 ` Pablo Neira Ayuso
2013-06-20 9:49 ` Pablo Neira Ayuso
1 sibling, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2013-06-20 9:22 UTC (permalink / raw)
To: Phil Oester; +Cc: netfilter-devel, pablo
Phil Oester <kernel@linuxace.com> wrote:
> kfree_skb already checks for skb existence - remove redundant check from
> nf_conntrack_put_reasm.
This is called for every free'd skb, in almost all cases
the condition will be false.
So I think its ok to keep this check.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: remove redundant skb check in nf_conntrack_put_reasm
2013-06-19 14:27 [PATCH] netfilter: remove redundant skb check in nf_conntrack_put_reasm Phil Oester
2013-06-20 9:22 ` Florian Westphal
@ 2013-06-20 9:49 ` Pablo Neira Ayuso
1 sibling, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2013-06-20 9:49 UTC (permalink / raw)
To: Phil Oester; +Cc: netfilter-devel
On Wed, Jun 19, 2013 at 10:27:35AM -0400, Phil Oester wrote:
> kfree_skb already checks for skb existence - remove redundant check from
> nf_conntrack_put_reasm.
Applied with changes. I have added this chunk to this patch.
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -154,8 +154,7 @@ static unsigned int nf_hashfn(struct
inet_frag_queue *q)
static void nf_skb_free(struct sk_buff *skb)
{
- if (NFCT_FRAG6_CB(skb)->orig)
- kfree_skb(NFCT_FRAG6_CB(skb)->orig);
+ kfree_skb(NFCT_FRAG6_CB(skb)->orig);
}
static void nf_ct_frag6_expire(unsigned long data)
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: remove redundant skb check in nf_conntrack_put_reasm
2013-06-20 9:22 ` Florian Westphal
@ 2013-06-20 9:58 ` Pablo Neira Ayuso
2013-06-20 0:01 ` Phil Oester
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2013-06-20 9:58 UTC (permalink / raw)
To: Florian Westphal; +Cc: Phil Oester, netfilter-devel
On Thu, Jun 20, 2013 at 11:22:59AM +0200, Florian Westphal wrote:
> Phil Oester <kernel@linuxace.com> wrote:
> > kfree_skb already checks for skb existence - remove redundant check from
> > nf_conntrack_put_reasm.
>
> This is called for every free'd skb, in almost all cases
> the condition will be false.
>
> So I think its ok to keep this check.
I see, we'll hit branch mispredictions most of the time. I have kept
this back. Thanks Florian.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: remove redundant skb check in nf_conntrack_put_reasm
2013-06-20 0:01 ` Phil Oester
@ 2013-06-20 14:15 ` Florian Westphal
0 siblings, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2013-06-20 14:15 UTC (permalink / raw)
To: Phil Oester; +Cc: Pablo Neira Ayuso, Florian Westphal, netfilter-devel
Phil Oester <kernel@linuxace.com> wrote:
> On Thu, Jun 20, 2013 at 11:58:46AM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Jun 20, 2013 at 11:22:59AM +0200, Florian Westphal wrote:
> > > Phil Oester <kernel@linuxace.com> wrote:
> > > > kfree_skb already checks for skb existence - remove redundant check from
> > > > nf_conntrack_put_reasm.
> > >
> > > This is called for every free'd skb, in almost all cases
> > > the condition will be false.
> > >
> > > So I think its ok to keep this check.
> >
> > I see, we'll hit branch mispredictions most of the time. I have kept
> > this back. Thanks Florian.
>
> If we're concerned about branch mispredictions, we should change the check to
> match the one at the top of kfree_skb:
> if (unlikely(!skb))
> return;
Its the opposite. For kfree_skb skb is (virtually) never NULL.
> However nf_conntrack_put_reasm is only used in 3 places in the entire tree.
> Unclear why we don't just call kfree_skb directly in these 3 places.
I guess what you could do is s/nf_conntrack_put_reasm/kfree_skb/g and
also change skb_release_head_state() to use
#ifdef NET_SKBUFF_NF_DEFRAG_NEEDED
if (skb->nfct) /* usually NULL */
kfree_skb(skb->nfct);
#endif
[ or show that there is no performance degradation for workloads where
we end up calling kfree_skb(NULL) in hot path ]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-06-20 14:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-19 14:27 [PATCH] netfilter: remove redundant skb check in nf_conntrack_put_reasm Phil Oester
2013-06-20 9:22 ` Florian Westphal
2013-06-20 9:58 ` Pablo Neira Ayuso
2013-06-20 0:01 ` Phil Oester
2013-06-20 14:15 ` Florian Westphal
2013-06-20 9:49 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).