Re: [PATCH v2] netfilter: save the hash of the tuple in the original direction for latter use

[yao zhao <dragonlinux@gmail.com>]

On Thu, Aug 19, 2010 at 11:35 AM, Changli Gao <xiaosuo@gmail.com> wrote:
> On Thu, Aug 19, 2010 at 11:21 PM, Mathieu Desnoyers
> <mathieu.desnoyers@polymtl.ca> wrote:
>> * Changli Gao (xiaosuo@gmail.com) wrote:
>>> Since we don't change the tuple in the original direction, we can save it
>>> in ct->tuplehash[IP_CT_DIR_REPLY].hnode.pprev for __nf_conntrack_confirm()
>>> use.
>>>
>>> __hash_conntrack() is split into two steps: ____hash_conntrack() is used
>>> to get the raw hash, and __hash_bucket() is used to get the bucket id.
>>>
>>> In SYN-flood case, early_drop() doesn't need to recompute the hash again.
>>>
>>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>>> ---
>>> v2: use cmpxchg() to save 2 variables.
>>>  net/netfilter/nf_conntrack_core.c |  114 ++++++++++++++++++++++++++------------
>>>  1 file changed, 79 insertions(+), 35 deletions(-)
>>> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
>>> index df3eedb..0e02205 100644
>>> --- a/net/netfilter/nf_conntrack_core.c
>>> +++ b/net/netfilter/nf_conntrack_core.c
>>> @@ -65,14 +65,20 @@ EXPORT_SYMBOL_GPL(nf_conntrack_max);
>>>  DEFINE_PER_CPU(struct nf_conn, nf_conntrack_untracked);
>>>  EXPORT_PER_CPU_SYMBOL(nf_conntrack_untracked);
>>>
>>> -static int nf_conntrack_hash_rnd_initted;
>>> -static unsigned int nf_conntrack_hash_rnd;
>>> -
>>> -static u_int32_t __hash_conntrack(const struct nf_conntrack_tuple *tuple,
>>> -                               u16 zone, unsigned int size, unsigned int rnd)
>>> +static u32 ____hash_conntrack(const struct nf_conntrack_tuple *tuple, u16 zone)
>>>  {
>>>       unsigned int n;
>>>       u_int32_t h;
>>> +     static unsigned int rnd;
>>
>> Why are you putting a hidden static variable here ? It should probably
>> be declared outside of the function scope.
>
> It is only used in this function.

Looks like it is only initialized once to rnd then it should go a "init".

>
>>
>>> +
>>> +     if (unlikely(!rnd)) {
>>> +             unsigned int rand;
>>> +
>>> +             get_random_bytes(&rand, sizeof(rand));
>>> +             if (!rand)
>>> +                     rand = 1;
>>> +             cmpxchg(&rnd, 0, rand);
>>
>> This really belongs to a "init()" function, not in a dynamic check in
>> the __hash_conntrack hot path. If you do that a init time, then you
>> don't even need the cmpxchg.
>>
>> Or maybe I completely misunderstand you goal here; maybe you are really
>> trying to re-calculate random bytes. But more explanation is called for,
>> because I really don't see where the value is brought back to 0.
>>
>
> In fact, I don't know the reason clearly. This code is derived from
> the older one. Maybe there isn't enough entropy when initializing.
>
> --
> Regards,
> Changli Gao(xiaosuo@gmail.com)
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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