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

[Eric Dumazet <eric.dumazet@gmail.com>]

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.

> +	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)