Re: [PATCH] netfilter: remove redundant skb check in nf_conntrack_put_reasm

[Florian Westphal <fw@strlen.de>]

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 ]