On 7/6/20 1:29 PM, David Miller wrote:
> From: "YU, Xiangning" <xiangning...@alibaba-inc.com>
> Date: Tue, 07 Jul 2020 02:08:13 +0800
>
>> 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.
>>
>> Signed-off-by: Xiangning Yu <xiangning...@alibaba-inc.com>
>
> I'm very skeptical about having a kthread for each qdisc, that doesn't
> sound like a good idea at all.
>
> Also:
>
Hi David,
I can change the kthread to a timer, like what sfq does for perturbation. Hope
that would be OK.
Will take care of the other comments too.
Thanks,
- Xiangning
>> +static inline struct ltb_skb_cb *ltb_skb_cb(const struct sk_buff *skb)
>
> No inline functions in foo.c files please.
>
>> +static inline s64 get_linkspeed(struct net_device *dev)
>
> Likewise.
>
>> +static inline int ltb_drain(struct ltb_class *cl)
>> +{
>> + typeof(&cl->drain_queue) queue;
>
> Use the actual type not typeof().
>
>> + struct sk_buff *skb;
>> + int npkts, bytes;
>> + unsigned long now = NOW();
>> + int cpu;
>> + struct ltb_sched *ltb = qdisc_priv(cl->root_qdisc);
>> + struct ltb_pcpu_sched *pcpu_q;
>> + s64 timestamp;
>> + bool need_watchdog = false;
>> + struct cpumask cpumask;
>
> Please order local variables in reverse christmas tree order.
>
>> +static void ltb_aggregate(struct ltb_class *cl)
>> +{
>> + s64 timestamp = ktime_get_ns();
>> + struct ltb_sched *ltb = qdisc_priv(cl->root_qdisc);
>> + int num_cpus = ltb->num_cpus;
>> + int i;
>
> Likewise.
>
>> +static inline void ltb_fanout(struct ltb_sched *ltb)
>> +{
>
> No inline please.
>
>> +/* How many classes within the same group want more bandwidth */
>> +static inline int bw_class_want_more_count(struct list_head *head)
>> +{
>> + int n = 0;
>> + struct ltb_class *cl;
>
> No inline, and reverse christmas tree ordering for local variables.
>
>> +/* Redistribute bandwidth among classes with the same priority */
>> +static int bw_redistribute_prio(struct list_head *lhead, int bw_available,
>> + int n, bool *all_reached_ceil)
>> +{
>> + struct ltb_class *cl;
>> + int avg = 0;
>> + int orig_bw_allocated;
>> + int safe_loop = 0;
>> +
>
> Likewise.
>
>> +static int bw_redistribute(struct ltb_sched *ltb, int bw_available)
>> +{
>> + int prio = 0;
>> + int n;
>> + int highest_non_saturated_prio = TC_LTB_NUMPRIO;
>> + bool all_reached_ceil;
>
> Likewise.
>
>> +static void bw_balance(struct ltb_sched *ltb)
>> +{
>> + struct ltb_class *cl;
>> + s64 link_speed = ltb->link_speed;
>> + int bw_available = link_speed;
>> + s64 total = 0;
>> + int high = TC_LTB_NUMPRIO;
>> + int is_light_traffic = 1;
>> + int i;
>
> Likewise.
>
> And so on and so forth. This code needs a lot of style fixes
> and removal of the per-qdisc kthread design.
>
> Thank you.
>
