netdev - Re: [PATCH net-next v2 2/4] sch_htb: Hierarchical QoS hardware offload

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] [day] [month] [year] [list]

Date:   Fri, 18 Dec 2020 12:48:02 +0200
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>
CC:     Maxim Mikityanskiy <maximmi@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jiri Pirko <jiri@...nulli.us>,
        Saeed Mahameed <saeedm@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Tariq Toukan <tariqt@...lanox.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        "Linux Kernel Network Developers" <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...dia.com>,
        Yossi Kuperman <yossiku@...dia.com>
Subject: Re: [PATCH net-next v2 2/4] sch_htb: Hierarchical QoS hardware
 offload

On 2020-12-17 17:09, Jamal Hadi Salim wrote:
> On 2020-12-16 6:47 a.m., Maxim Mikityanskiy wrote:
>> On 2020-12-15 18:37, Jamal Hadi Salim wrote:
> 
> [..]
> 
>>>
>>> Same question above:
>>> Is there a limit to the number of classes that can be created?
>>
>> Yes, the commit message of the mlx5 patch lists the limitations of our 
>> NICs. Basically, it's 256 leaf classes and 3 levels of hierarchy.
>>
> 
> Ok, thats what i was looking for.
> 
> 
>>> IOW, if someone just created an arbitrary number of queues do they
>>> get errored-out if it doesnt make sense for the hardware?
>>
>> The current implementation starts failing gracefully if the limits are 
>> exceeded. The tc command won't succeed, and everything will roll back 
>> to the stable state, which was just before the tc command.
>>
> 
> Does the user gets notified somehow or it fails silently?
> An extack message would help.
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1;
	t=1608288498; bh=RMhOdIb4utXHB89dvxKO9kM+jQNSV+kIKsJ0O6KNJWs=;
	h=Subject:To:CC:References:From:Message-ID:Date:User-Agent:
	 MIME-Version:In-Reply-To:Content-Type:Content-Language:
	 Content-Transfer-Encoding:X-Originating-IP:X-ClientProxiedBy;
	b=pThe8Bqxykn4jFcf5ZYZMSgEPZZBiId+lReLZZ9KFCdbROoDwWPTGDWFNhOADAvRA
	 7bjihDOFAvu2+MXP1m9/IS0lQgKXa2iOKAk5S1QwZj+aF5m9bZ1ya7uRSZiK7k32Tp
	 cPyfic9k+SXTN0zyJovtWc5i4QlwnHWCQ7zPvMYwB4Kmvmk/YiRwIQvEooMDQaq92Z
	 7+scnlRaNVAEd8ZERye9Fxa7htaGiUArPQR3aS7ol1QmrCXaN1hZA84lySCaLcJ6JA
	 6qFKvjj0XWP0TsYJqpZzkS+F0yfGOAphtaIMWq/o8m5Julk0e90Yj6maRvGG3kFOaY
	 c4zEilz1uAHKA==

The current implementation doesn't use extack, just returns an error 
code, because many callbacks to the qdisc don't get extack as a 
parameter. However, I agree with you, these messages would be helpful 
for the user when an operation fails due to hardware limitations - it 
will be easier than guessing what caused a EINVAL, so I'll add them. I 
will review which callbacks lacked an extack, and I might add it if it's 
meaningful for them.

> 
>>> If such limits exist, it may make sense to provide a knob to query
>>> (maybe ethtool)
>>
>> Sounds legit, but I'm not sure what would be the best interface for 
>> that. Ethtool is not involved at all in this implementation, and AFAIK 
>> it doesn't contain any existing command for similar stuff. We could 
>> hook into set-channels and add new type of channels for HTB, but the 
>> semantics isn't very clear, because HTB queues != HTB leaf classes, 
>> and I don't know if it's allowed to extend this interface (if so, I 
>> have more thoughts of extending it for other purposes).
>>
> 
> More looking to make sure no suprise to the user. Either the user can
> discover what the constraints are or when they provision they get a
> a message like "cannot offload more than 3 hierarchies" or "use devlink
> if you want to use more than 256 classes", etc.

Yes, it makes perfect sense. Messages are even more user-friendly, as 
for me. So, I'll add such messages to extack, and as the limitations are 
driver-specific, I'll pass extack to the driver.

I will respin when net-next reopens, in the meanwhile comments are welcome.

Thanks,
Max

> 
> cheers,
> jamal