Skip to contentSkip to conversation

netfilter: remove redundant skb check in nf_conntrack_put_reasm

PATCH In review Phil Oester 3 participants 6 messages Jun 20, 2013

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Phil Oester

author
#0 lore raw

PATCH 1/1 netfilter: remove redundant skb check in nf_conntrack_put_reasm

kfree_skb already checks for skb existence - remove redundant check from nf_conntrack_put_reasm.

Phil

Signed-off-by Phil Oester <[email protected]>
include/linux/skbuff.h +1 -2
index a7393ad..3da70ba 100644
@@ -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
maintainer
#1 lore raw
Quoted 2 lines · Phil Oester

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.

maintainer
#5 lore raw
In reply to Phil Oester
Quoted 2 lines · Phil Oester

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.

net/ipv6/netfilter/nf_conntrack_reasm.c +0 -0
@@ -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.

maintainer
#2 lore raw
In reply to Florian Westphal
Quoted 8 lines · Florian Westphal
Quoted 2 lines · Phil Oester

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.

author
#3 lore raw
Quoted 5 lines · Pablo Neira Ayuso
Quoted 4 lines · Florian Westphal
Quoted 2 lines · Phil Oester

kfree_skb already checks for skb existence - remove redundant check from nf_conntrack_put_reasm.

Quoted 7 lines
Quoted 4 lines

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

maintainer
#4 lore raw
Quoted 11 lines · Phil Oester
Quoted 10 lines · Pablo Neira Ayuso
Quoted 8 lines · Florian Westphal
Quoted 2 lines · Phil Oester

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.

Quoted 7 lines
Quoted 2 lines

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.

Quoted 2 lines

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 ]