Patchwork netfilter: remove redundant skb check in nf_conntrack_put_reasm

login
register
mail settings
Submitter Phil Oester
Date June 19, 2013, 2:27 p.m.
Message ID <20130619142735.GA2758@gmail.com>
Download mbox | patch
Permalink /patch/252705/
State Not Applicable
Headers show

Comments

Phil Oester - June 19, 2013, 2:27 p.m.
kfree_skb already checks for skb existence - remove redundant check from
nf_conntrack_put_reasm.

Phil

Signed-off-by: Phil Oester <kernel@linuxace.com>
Phil Oester - June 20, 2013, 12:01 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal - June 20, 2013, 9:22 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - June 20, 2013, 9:58 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal - June 20, 2013, 2:15 p.m.
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 ]
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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