netdev - Re: [PATCH net-next v2 2/2] net: sched: Lockless Token Bucket (LTB) qdisc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d1716bc1-a975-54a3-8b7e-a3d3bcac69c5@alibaba-inc.com>
Date:   Thu, 09 Jul 2020 07:59:45 +0800
From:   "YU, Xiangning" <xiangning.yu@...baba-inc.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 2/2] net: sched: Lockless Token Bucket (LTB)
 qdisc



On 7/8/20 3:29 PM, Eric Dumazet wrote:
> 
> 
> On 7/8/20 9:38 AM, YU, Xiangning wrote:
>> Lockless Token Bucket (LTB) is a qdisc implementation that controls the
>> use of outbound bandwidth on a shared link. With the help of lockless
>> qdisc, and by decoupling rate limiting and bandwidth sharing, LTB is
>> designed to scale in the cloud data centers.
> 
>> +static int ltb_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>> +		       spinlock_t *root_lock, struct sk_buff **to_free)
>> +{
>> +	struct ltb_sched *ltb = qdisc_priv(sch);
>> +	struct ltb_pcpu_sched *pcpu_q;
>> +	struct ltb_pcpu_data *pcpu;
>> +	struct ltb_class *cl;
>> +	int cpu;
>> +
>> +	pcpu = this_cpu_ptr(ltb->pcpu_data);
>> +	pcpu_q = qdisc_priv(pcpu->qdisc);
>> +	cpu = smp_processor_id();
>> +	ltb_skb_cb(skb)->cpu = cpu;
>> +
>> +	cl = ltb_classify(sch, ltb, skb);
>> +	if (unlikely(!cl)) {
>> +		kfree_skb(skb);
>> +		return NET_XMIT_DROP;
>> +	}
>> +
>> +	pcpu->active = true;
>> +	if (unlikely(kfifo_put(&cl->aggr_queues[cpu], skb) == 0)) {
>> +		kfree_skb(skb);
>> +		atomic64_inc(&cl->stat_drops);
> 
>             qdisc drop counter should also be incremented.
> 
>> +		return NET_XMIT_DROP;
>> +	}
>> +
> 
>> +	sch->q.qlen = 1;
> So, this is touching a shared cache line, why is it needed ? This looks some hack to me.
> 

Somehow I had the impression that if qlen is zero the qdisc won't be scheduled. We need to fix it. Thank you for catching this!

>> +	pcpu_q->qdisc->q.qlen++;
> 
>> +	tasklet_schedule(&cl->aggr_tasklet);
> 
> This is also touching a cache line.
> 
> I really have doubts about scheduling a tasklet for every sent packet.
> 
> (Particularly if majority of packets should not be rate limited)
> 

Yes, we are touching a cache line here to make sure aggregation tasklet is scheduled immediately. In most cases it is a call to test_and_set_bit(). 

We might be able to do some inline processing without tasklet here, still we need to make sure the aggregation won't run simultaneously on multiple CPUs. 

Thanks,
- Xiangning