Redundant calls to nf_ct_tuplehash_to_ctrack

Redundant calls to nf_ct_tuplehash_to_ctrack

[Anand Raj Manickam]

This my observation ..

There is a redundant call to nf_ct_tuplehash_to_ctrack() after every
nf_conntrack_find_get() call .

Why is ct not returned from nf_conntrack_find_get() , since in all the
occurances of nf_conntrack_find_get() there is immediate call to
nf_ct_tuplehash_to_ctrack() . Although  nf_ct_tuplehash_to_ctrack() is
invoked to check the ct after we get a valid hash .

Following the code snippet of nf_conntrack_find_get


struct nf_conntrack_tuple_hash *
nf_conntrack_find_get(struct net *net, u16 zone,
                      const struct nf_conntrack_tuple *tuple)
{
        struct nf_conntrack_tuple_hash *h;
        struct nf_conn *ct;

        rcu_read_lock();
begin:
        h = __nf_conntrack_find(net, zone, tuple);
        if (h) {
                ct = nf_ct_tuplehash_to_ctrack(h);
                if (unlikely(nf_ct_is_dying(ct) ||
                             !atomic_inc_not_zero(&ct->ct_general.use)))
                        h = NULL;
                else {
                        if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple) ||
                                     nf_ct_zone(ct) != zone)) {
                                nf_ct_put(ct);
                                goto begin;
                        }
                }
        }
        rcu_read_unlock();

        return h;
}

Re: Redundant calls to nf_ct_tuplehash_to_ctrack

[Eric Dumazet]

Le mardi 08 juin 2010 à 19:48 +0530, Anand Raj Manickam a écrit :
> This my observation ..
> 
> There is a redundant call to nf_ct_tuplehash_to_ctrack() after every
> nf_conntrack_find_get() call .
> 
> Why is ct not returned from nf_conntrack_find_get() , since in all the
> occurances of nf_conntrack_find_get() there is immediate call to
> nf_ct_tuplehash_to_ctrack() . Although  nf_ct_tuplehash_to_ctrack() is
> invoked to check the ct after we get a valid hash .
> 

One random sample :

net/ipv4/netfilter/nf_conntrack_proto_icmp.c

        h = nf_conntrack_find_get(net, zone, &innertuple);
        if (!h) {
                pr_debug("icmp_error_message: no match\n");
                return -NF_ACCEPT;
        }

        if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
                *ctinfo += IP_CT_IS_REPLY;


As you can see, we need 'h' here (to get NF_CT_DIRECTION(h)), not 'ct'



--
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

Re: Redundant calls to nf_ct_tuplehash_to_ctrack

[Anand Raj Manickam]

On Tue, Jun 8, 2010 at 8:46 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 08 juin 2010 à 19:48 +0530, Anand Raj Manickam a écrit :
>> This my observation ..
>>
>> There is a redundant call to nf_ct_tuplehash_to_ctrack() after every
>> nf_conntrack_find_get() call .
>>
>> Why is ct not returned from nf_conntrack_find_get() , since in all the
>> occurances of nf_conntrack_find_get() there is immediate call to
>> nf_ct_tuplehash_to_ctrack() . Although  nf_ct_tuplehash_to_ctrack() is
>> invoked to check the ct after we get a valid hash .
>>
>
> One random sample :
>
> net/ipv4/netfilter/nf_conntrack_proto_icmp.c
>
>        h = nf_conntrack_find_get(net, zone, &innertuple);
>        if (!h) {
>                pr_debug("icmp_error_message: no match\n");
>                return -NF_ACCEPT;
>        }
>
>        if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
>                *ctinfo += IP_CT_IS_REPLY;
>
>
> As you can see, we need 'h' here (to get NF_CT_DIRECTION(h)), not 'ct'

I did think about it,
Can we derive the direction from conntrack  mabbe we replace
NF_CT_DIRECTION with the following  .

/* If we're the first tuple, it's the original dir. */
 #define NF_CT_DIRECTION(h)      ((enum ip_conntrack_dir)(h)->tuple.dst.dir)

if (ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.dir == IP_CT_DIR_REPLY)
--
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

Re: Redundant calls to nf_ct_tuplehash_to_ctrack

[Eric Dumazet]

Le mardi 08 juin 2010 à 21:26 +0530, Anand Raj Manickam a écrit :
> On Tue, Jun 8, 2010 at 8:46 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Le mardi 08 juin 2010 à 19:48 +0530, Anand Raj Manickam a écrit :
> >> This my observation ..
> >>
> >> There is a redundant call to nf_ct_tuplehash_to_ctrack() after every
> >> nf_conntrack_find_get() call .
> >>
> >> Why is ct not returned from nf_conntrack_find_get() , since in all the
> >> occurances of nf_conntrack_find_get() there is immediate call to
> >> nf_ct_tuplehash_to_ctrack() . Although  nf_ct_tuplehash_to_ctrack() is
> >> invoked to check the ct after we get a valid hash .
> >>
> >
> > One random sample :
> >
> > net/ipv4/netfilter/nf_conntrack_proto_icmp.c
> >
> >        h = nf_conntrack_find_get(net, zone, &innertuple);
> >        if (!h) {
> >                pr_debug("icmp_error_message: no match\n");
> >                return -NF_ACCEPT;
> >        }
> >
> >        if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
> >                *ctinfo += IP_CT_IS_REPLY;
> >
> >
> > As you can see, we need 'h' here (to get NF_CT_DIRECTION(h)), not 'ct'
> 
> I did think about it,
> Can we derive the direction from conntrack  mabbe we replace
> NF_CT_DIRECTION with the following  .
> 
> /* If we're the first tuple, it's the original dir. */
>  #define NF_CT_DIRECTION(h)      ((enum ip_conntrack_dir)(h)->tuple.dst.dir)
> 
> if (ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.dir == IP_CT_DIR_REPLY)

Well, that would be a big problem, if you find a ct where this test is
true ;)

By definition :

ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.dir == IP_CT_DIR_ORIGINAL

I dont know why you care, since nf_ct_tuplehash_to_ctrack() is _very_
cheap, its a few addition/subtraction.



--
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