netdev - Re: [PATCH net-next 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: <26fa9e62-22c7-649e-f7f8-a5b7e678fd99@alibaba-inc.com>
Date:   Tue, 07 Jul 2020 06:08:44 +0800
From:   "YU, Xiangning" <xiangning.yu@...baba-inc.com>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] net: sched: Lockless Token Bucket (LTB)
 Qdisc



On 7/6/20 1:29 PM, David Miller wrote:
> From: "YU, Xiangning" <xiangning.yu@...baba-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.yu@...baba-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.
>