netdev - Re: [PATCH net-next v3 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]

Message-ID: <04607f76-3548-f6d1-061c-8d5d918f4017@nvidia.com>
Date:   Mon, 4 Jan 2021 23:55:13 +0200
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Maxim Mikityanskiy <maximmi@...lanox.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>, David Ahern <dsahern@...il.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Tariq Toukan <tariqt@...dia.com>,
        Yossi Kuperman <yossiku@...dia.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v3 2/4] sch_htb: Hierarchical QoS hardware
 offload

On 2020-12-22 03:17, Jakub Kicinski wrote:
> On Tue, 15 Dec 2020 09:42:11 +0200 Maxim Mikityanskiy wrote:
>> +	q->offload = nla_get_flag(tb[TCA_HTB_OFFLOAD]);
>> +
>> +	if (q->offload) {
>> +		if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
>> +			return -EOPNOTSUPP;
> 
> Is there a check somewhere making sure this is the root?

No, I should add something like:
         if (sch->parent != TC_H_ROOT)
                 return -EOPNOTSUPP;

Thanks.

>> +		q->num_direct_qdiscs = dev->real_num_tx_queues;
> 
> Why real_num_tx_queues? How do you handle queue count changes?

When HTB gets attached, we query the number of regular TX queues and use 
them for HTB direct (unclassified) traffic. When HTB is already 
attached, changing the number of channels is blocked by the driver, so 
the amount of direct queues doesn't change (otherwise it would be hard 
to maintain correspondence between queues and classes). However, new 
queues are added (i.e. real_num_tx_queues increases) when HTB classes 
are added.

>> +		q->direct_qdiscs = kcalloc(q->num_direct_qdiscs,
>> +					   sizeof(*q->direct_qdiscs),
>> +					   GFP_KERNEL);
>> +		if (!q->direct_qdiscs)
>> +			return -ENOMEM;
>> +	}
> 
> I can't quite parse after 20 minutes of staring at this code what the
> relationship between the device queues and classes is. Is there any
> relationship between real_num_tx_queues and classes?
> 

Let's say we had N TX queues before attaching HTB (i.e. 
real_num_tx_queues was N). Then queues 0..N-1 correspond to direct mode 
of HTB, and queues starting from N are used for leaf classes. So, when 
some classes are added, real_num_tx_queues will be bigger than N (say, 
it would be M), and queues N..M-1 will correspond to leaf classes. Inner 
classes don't have a backing queue.

In the current implementation, one leaf class is backed by one queue, 
but it can be changed in the future to allow multiple queues to back a 
class.

The mapping is stored in the driver and used in ndo_select_queue.

I hope it answers your question, but I'm ready to provide more details 
if you ask.

Thanks,
Max