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

[Cong Wang]

On Mon, Jul 6, 2020 at 11:11 AM YU, Xiangning
<xiangning...@alibaba-inc.com> wrote:
> +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_class *cl;
> +       struct ltb_pcpu_data *pcpu = this_cpu_ptr(ltb->pcpu_data);
> +       int cpu;
> +
> +       cpu = smp_processor_id();
> +       pcpu_q = qdisc_priv(pcpu->qdisc);
> +       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);
> +               return NET_XMIT_DROP;
> +       }


How do you prevent out-of-order packets?


> +static int ltb_init(struct Qdisc *sch, struct nlattr *opt,
...
> +       ltb->default_cls = ltb->shadow_cls; /* Default hasn't been created */
> +       tasklet_init(&ltb->fanout_tasklet, ltb_fanout_tasklet,
> +                    (unsigned long)ltb);
> +
> +       /* Bandwidth balancer, this logic can be implemented in user-land. */
> +       init_waitqueue_head(&ltb->bwbalancer_wq);
> +       ltb->bwbalancer_task =
> +           kthread_create(ltb_bw_balancer_kthread, ltb, "ltb-balancer");
> +       wake_up_process(ltb->bwbalancer_task);

Creating a kthread for each qdisc doesn't look good. Why do you
need a per-qdisc kthread or even a kernel thread at all?

Thanks.