Re: [PATCH iproute2-next] tc/htb: Hierarchical QoS hardware offload

[Maxim Mikityanskiy <maximmi@nvidia.com>]

On 2021-01-29 18:05, David Ahern wrote:
> On 12/15/20 12:42 AM, Maxim Mikityanskiy wrote:
>> This commit adds support for configuring HTB in offload mode. HTB
>> offload eliminates the single qdisc lock in the datapath and offloads
>> the algorithm to the NIC. The new 'offload' parameter is added to
>> enable this mode:
>>
>>      # tc qdisc replace dev eth0 root handle 1: htb offload
>>
>> Classes are created as usual, but filters should be moved to clsact for
>> lock-free classification (filters attached to HTB itself are not
>> supported in the offload mode):
>>
>>      # tc filter add dev eth0 egress protocol ip flower dst_port 80
>>      action skbedit priority 1:10
> 
> please add the dump in both stdout and json here.

Do you mean to add example output to the commit message?

>>
>> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
>> ---
>>   include/uapi/linux/pkt_sched.h | 1 +
>>   tc/q_htb.c                     | 9 ++++++++-
> 
> missing an update to man/man8/tc-htb.8

OK, I'll add.

>>   2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
>> index 9e7c2c60..79a699f1 100644
>> --- a/include/uapi/linux/pkt_sched.h
>> +++ b/include/uapi/linux/pkt_sched.h
>> @@ -434,6 +434,7 @@ enum {
>>   	TCA_HTB_RATE64,
>>   	TCA_HTB_CEIL64,
>>   	TCA_HTB_PAD,
>> +	TCA_HTB_OFFLOAD,
>>   	__TCA_HTB_MAX,
>>   };
>>   
>> diff --git a/tc/q_htb.c b/tc/q_htb.c
>> index c609e974..fd11dad6 100644
>> --- a/tc/q_htb.c
>> +++ b/tc/q_htb.c
>> @@ -30,11 +30,12 @@
>>   static void explain(void)
>>   {
>>   	fprintf(stderr, "Usage: ... qdisc add ... htb [default N] [r2q N]\n"
>> -		"                      [direct_qlen P]\n"
>> +		"                      [direct_qlen P] [offload]\n"
>>   		" default  minor id of class to which unclassified packets are sent {0}\n"
>>   		" r2q      DRR quantums are computed as rate in Bps/r2q {10}\n"
>>   		" debug    string of 16 numbers each 0-3 {0}\n\n"
>>   		" direct_qlen  Limit of the direct queue {in packets}\n"
>> +		" offload  hardware offload\n"
> 
> why 'offload hardware offload'? does not make sense to me and

"offload" is a new parameter, and "hardware offload" is the description, 
just like the other parameters above.

> you don't
> reference hardware below.

Where should I reference it?

> 
> 
> 
>>   		"... class add ... htb rate R1 [burst B1] [mpu B] [overhead O]\n"
>>   		"                      [prio P] [slot S] [pslot PS]\n"
>>   		"                      [ceil R2] [cburst B2] [mtu MTU] [quantum Q]\n"
>> @@ -68,6 +69,7 @@ static int htb_parse_opt(struct qdisc_util *qu, int argc,
>>   	};
>>   	struct rtattr *tail;
>>   	unsigned int i; char *p;
>> +	bool offload = false;
>>   
>>   	while (argc > 0) {
>>   		if (matches(*argv, "r2q") == 0) {
>> @@ -91,6 +93,8 @@ static int htb_parse_opt(struct qdisc_util *qu, int argc,
>>   			if (get_u32(&direct_qlen, *argv, 10)) {
>>   				explain1("direct_qlen"); return -1;
>>   			}
>> +		} else if (matches(*argv, "offload") == 0) {
>> +			offload = true;
>>   		} else {
>>   			fprintf(stderr, "What is \"%s\"?\n", *argv);
>>   			explain();
>> @@ -103,6 +107,8 @@ static int htb_parse_opt(struct qdisc_util *qu, int argc,
>>   	if (direct_qlen != ~0U)
>>   		addattr_l(n, 2024, TCA_HTB_DIRECT_QLEN,
>>   			  &direct_qlen, sizeof(direct_qlen));
>> +	if (offload)
>> +		addattr(n, 2024, TCA_HTB_OFFLOAD);
>>   	addattr_nest_end(n, tail);
>>   	return 0;
>>   }
>> @@ -344,6 +350,7 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>>   		print_uint(PRINT_ANY, "direct_qlen", " direct_qlen %u",
>>   			   direct_qlen);
>>   	}
>> +	print_uint(PRINT_ANY, "offload", " offload %d", !!tb[TCA_HTB_OFFLOAD]);
>>   	return 0;
>>   }
>>   
>>
>