Re: [PATCH RFC] act_cpu: packet distributing

[Changli Gao <xiaosuo@gmail.com>]

On Wed, Jul 14, 2010 at 11:41 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mercredi 14 juillet 2010 à 11:17 +0800, Changli Gao a écrit :
>> I want to know if I can assign the sk to skb as nf_tproxy_core does to avoid
>> the duplicate search later. Thanks.
>>
>
> I suggest we first correct bugs before adding new features.
>
> For me, tproxy is a high suspect at this moment, I would not use it on
> production machine.
>
>> act_cpu: packet distributing
>>
>> This TC action can be used to redirect packets to the CPU:
>>  * specified by the cpuid option
>>  * specified by the class minor ID obtained previously
>>  * on which the corresponding application runs
>>
>> It supports the similar functions of RPS and RFS, but is more flexible.
>>
>
> Thins kind of claims is disgusting.
>
> No documentation,  I have no idea how you setup the thing.

for example:

redirect packets to the CPU on which the corresponding application runs

tc qdisc add dev eth0 ingress
tc filter add dev eth0 parent ffff:0 protocol ip basic action cpu socket

redirect packets to special CPU.

tc filter add dev eth0 parent ffff:0 protocol ip basic action cpu cpuid 1

sport hash packet distributing among CPU 0-1.

tc filter add dev eth0 parent ffff:0 protocol ip handle 1 flow hash
keys proto-src divisor 2 action cpu map 1

>> +static inline void sock_save_cpu(struct sock *sk)
>> +{
>> +#if defined(CONFIG_NET_ACT_CPU) || defined(CONFIG_NET_ACT_CPU_MODULE)
>> +     int cpu = get_cpu();
>> +     if (sk->sk_cpu != cpu)
>> +             sk->sk_cpu = cpu;
>> +     put_cpu();
>
> sk->sk_cpu = raw_smp_processor_id();

Thanks.

>
> Why doing the search again, in case skb->sk already set by another
> module before you, like tproxy ?
>

Although it is unlikely skb->sk is non null, as tc is before
netfilter, I will handle this case. Thanks.

>> +static int tcf_cpu(struct sk_buff *skb, struct tc_action *a,
>> +                struct tcf_result *res)
>> +{
>> +     struct tcf_cpu *p = a->priv;
>> +     u32 type;
>> +     u32 value;
>> +     int cpu, action;
>> +     struct sk_buff *nskb;
>> +     unsigned int qtail;
>> +
>> +     spin_lock(&p->tcf_lock);
>
> Ok, the big lock...
>
> We have a lockless TCP/UDP input path, and this modules adds a lock
> again.
>
>> +     p->tcf_tm.lastuse = jiffies;
>> +     p->tcf_bstats.bytes += qdisc_pkt_len(skb);
>> +     p->tcf_bstats.packets++;
>> +     type = p->type;
>> +     value = p->value;
>> +     action = p->tcf_action;
>> +     spin_unlock(&p->tcf_lock);
>> +
>
> Please change all this crap  (legacy crap, copied from other tc
> modules), by modern one, using RCU and no locking in hot path.
>

Thanks, I'll try. It is a write critical section, and for me it is
difficult to convert this lock to RCU. Could you show me some
examples?
 Thanks.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)