Re: [PATCH] netfilter: save the hash of the tuple in the originaldirection for latter use

[Changli Gao]

On Tue, Aug 17, 2010 at 5:01 PM, Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote:
> Le mardi 17 août 2010 à 16:46 +0800, Changli Gao a écrit :
>> On Tue, Aug 17, 2010 at 4:30 PM, Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote:
>> >
>> > Three variables ?
>> >
>> > static atomic_t rnd __read_mostly;
>> >
>> > if (unlikely(!atomic_read(&rnd))) {
>> >        unsigned int val;
>> >
>> >        get_random_bytes(&val, sizeof(val));
>> >        if (!val)
>> >                val = 1;
>> >        atomic_cmpxchg(&rnd, 0, val);
>> > }
>> >
>>
>>
>> Good idea. However, atomic_t is a volatile variable, and it prevent
>> ILP. I think maybe it hurts the likely case. cmpxchg() is an atomic
>> operations, so is the bellow code better?
>>
>
> I am not sure what you mean by ILP.

I mean http://en.wikipedia.org/wiki/Instruction_level_parallelism

and volatile will suppress compiler optimization.

>
> On x86, reading twice same memory location is faster than
> reading it and store in temp variable (on stack)
> reading from stack

Thanks for this info. But compiler may store it in a register. In
fact, we can't control the behavior of compiler in C.

>
>>  static unsigned long rnd __read_mostly;
>>
>>  if (unlikely(!rnd)) {
>>         unsigned long val;
>>
>>         get_random_bytes(&val, sizeof(val));
>>         if (!val)
>>                 val = 1;
>>         cmpxchg(&rnd, 0, val);
>>  }
>>
>> Thanks.
>>
>
> I am not sure we must use a long (we really need 4 bytes only),

Yea, 4 bytes is enough.

> and last
> time I tried to use cmpxchg(), I was being told it was not available on
> all arches.
>
> But seeing it used in kernel/pid.c, maybe its not true anymore (that is,
> __HAVE_ARCH_CMPXCHG is always defined to 1)
>
> Since its a recent change (in kernel/pid.c), I would wait a bit and see
> if an arch maintainer complains ;)
>

Thanks. I saw it. These are two cmpxchg() in file:

include/asm-generic/cmpxchg.h

include/asm-generic/system.h

static inline unsigned long __cmpxchg(volatile unsigned long *m,
                                      unsigned long old, unsigned long new)
{
        unsigned long retval;
        unsigned long flags;

        local_irq_save(flags);
        retval = *m;
        if (retval == old)
                *m = new;
        local_irq_restore(flags);
        return retval;
}

#define cmpxchg(ptr, o, n)                                      \
        ((__typeof__(*(ptr))) __cmpxchg((unsigned long *)(ptr), \
                                        (unsigned long)(o),     \
                                        (unsigned long)(n)))

This one seems buggy when using it to non unsigned long variables. And
in kernel/pid.c, the last_pid variable is int.

-- 
Regards,
Changli Gao(xiaosuo@xxxxxxxxx)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/