Re: [net-next PATCH 1/5] sch_htb: Allow HTB priority parameter in offload mode

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Fri, 13 Jan, 2023 14:19:38 +0200 Maxim Mikityanskiy <maxtram95@gmail.com> wrote:
> On Thu, Jan 12, 2023 at 11:01:16PM +0530, Hariprasad Kelam wrote:
>> From: Naveen Mamindlapalli <naveenm@marvell.com>
>> 
>> The current implementation of HTB offload returns the EINVAL error
>> for unsupported parameters like prio and quantum. This patch removes
>> the error returning checks for 'prio' parameter and populates its
>> value to tc_htb_qopt_offload structure such that driver can use the
>> same.
>> 
>> Signed-off-by: Naveen Mamindlapalli <naveenm@marvell.com>
>> Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
>> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
>> ---
>>  include/net/pkt_cls.h | 1 +
>>  net/sched/sch_htb.c   | 7 +++----
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>> index 4cabb32a2ad9..02afb1baf39d 100644
>> --- a/include/net/pkt_cls.h
>> +++ b/include/net/pkt_cls.h
>> @@ -864,6 +864,7 @@ struct tc_htb_qopt_offload {
>>  	u16 qid;
>>  	u64 rate;
>>  	u64 ceil;
>> +	u8 prio;
>>  };
>>  
>>  #define TC_HTB_CLASSID_ROOT U32_MAX
>> diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
>> index 2238edece1a4..f2d034cdd7bd 100644
>> --- a/net/sched/sch_htb.c
>> +++ b/net/sched/sch_htb.c
>> @@ -1806,10 +1806,6 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>>  			NL_SET_ERR_MSG(extack, "HTB offload doesn't support the quantum parameter");
>>  			goto failure;
>>  		}
>> -		if (hopt->prio) {
>> -			NL_SET_ERR_MSG(extack, "HTB offload doesn't support the prio parameter");
>> -			goto failure;
>> -		}
>
> The check should go to mlx5e then.
>

Agreed. Also, I am wondering in general if its a good idea for the HTB
offload implementation to be dictating what parameters are and are not
supported.

	if (q->offload) {
		/* Options not supported by the offload. */
		if (hopt->rate.overhead || hopt->ceil.overhead) {
			NL_SET_ERR_MSG(extack, "HTB offload doesn't support the overhead parameter");
			goto failure;
		}
		if (hopt->rate.mpu || hopt->ceil.mpu) {
			NL_SET_ERR_MSG(extack, "HTB offload doesn't support the mpu parameter");
			goto failure;
		}
		if (hopt->quantum) {
			NL_SET_ERR_MSG(extack, "HTB offload doesn't support the quantum parameter");
			goto failure;
		}
	}

Every time a vendor introduces support for a new offload parameter,
netdevs that cannot support said parameter are affected. I think it
would be better to remove this block and expect each driver to check
what parameters are and are not supported for their offload flow.

>>  	}
>>  
>>  	/* Keeping backward compatible with rate_table based iproute2 tc */
>> @@ -1905,6 +1901,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>>  					TC_HTB_CLASSID_ROOT,
>>  				.rate = max_t(u64, hopt->rate.rate, rate64),
>>  				.ceil = max_t(u64, hopt->ceil.rate, ceil64),
>> +				.prio = hopt->prio,
>>  				.extack = extack,
>>  			};
>>  			err = htb_offload(dev, &offload_opt);
>> @@ -1925,6 +1922,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>>  					TC_H_MIN(parent->common.classid),
>>  				.rate = max_t(u64, hopt->rate.rate, rate64),
>>  				.ceil = max_t(u64, hopt->ceil.rate, ceil64),
>> +				.prio = hopt->prio,
>>  				.extack = extack,
>>  			};
>>  			err = htb_offload(dev, &offload_opt);
>> @@ -2010,6 +2008,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>>  				.classid = cl->common.classid,
>>  				.rate = max_t(u64, hopt->rate.rate, rate64),
>>  				.ceil = max_t(u64, hopt->ceil.rate, ceil64),
>> +				.prio = hopt->prio,
>>  				.extack = extack,
>>  			};
>>  			err = htb_offload(dev, &offload_opt);
>> -- 
>> 2.17.1
>>