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]

Date:   Sat, 11 Jul 2020 01:03:02 +0800
From:   "YU, Xiangning" <xiangning.yu@...baba-inc.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] net: sched: Lockless Token Bucket (LTB)
 Qdisc



On 7/9/20 11:09 PM, Cong Wang wrote:
> On Thu, Jul 9, 2020 at 10:49 PM YU, Xiangning
> <xiangning.yu@...baba-inc.com> wrote:
>>
>> Well, we do ask packets from a flow to be classified to a single class, not multiple ones. It doesn't have to be socket priority, it could be five tuple hash, or even container classid.
> 
> I don't see how it is so in your code, without skb priority your code
> simply falls back to default class:
> 
> +       /* Allow to select a class by setting skb->priority */
> +       if (likely(skb->priority != 0)) {
> +               cl = ltb_find_class(sch, skb->priority);
> +               if (cl)
> +                       return cl;
> +       }
> +       return rcu_dereference_bh(ltb->default_cls);
> 
> Mind to be more specific here?
> 

The application will call setsockopt() to set priority. If no match if found, it will use the default class.

In real deployment we have another classifier. Please feel free to suggest what is best choice for it. 

> BTW, your qdisc does not even support TC filters, does it?
> At least I don't see that tcf_classify() is called.
>

You are correct we don't use generic tc filters. I leave it unsupported in this patch too.
 
> 
>>
>> I think it's ok to have this requirement, even if we use htb, I would suggest the same. Why do you think this is a problem?
> 
> Because HTB does not have a per-cpu queue for each class,
> yours does, cl->aggr_queues[cpu], if your point here is why we
> don't blame HTB.
>

My point is: it's OK to ask a flow to be classified to a single class.

Thanks,
- Xiangning

> Thanks.
>