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

[Maxim Mikityanskiy <maximmi@nvidia.com>]

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