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
