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

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Mon, 16 Jan, 2023 05:18:32 +0000 Hariprasad Kelam <hkelam@marvell.com> wrote:
> Thanks for the review,
>
>>> If you extend the API (for example, with a new parameter), you have to make sure existing drivers are not broken.
>    Sure, we will add checks in existing drivers for the new parameter.

Just sent a patch for mlx5e that should be applied in a v2 patch series.

>
> Thanks,
> Hariprasad k
>
> On Fri, Jan 13, 2023 at 01:06:52PM -0800, Rahul Rameshbabu wrote:
>> 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;
>> 		}
>> 	}
>
> Jakub asked for that [1], I implemented it [2].
>
> [1]:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20220113110801.7c1a6347-40kicinski-2Dfedora-2DPC1C0HJN.hsd1.ca.comcast.net_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=2bd4kP44ECYFgf-KoNSJWqEipEtpxXnNBKy0vyoJJ8A&m=BHYls0vs10PjYQd-g7Lv51bPiN5Ay-x1lca_mGg_S_tH2pfwR7uADDykRTMmtVcU&s=FQPgPEhy6I2JRBqOmbyX8xAU69oNnUrl33ZR8QY8ZuM&e=
> [2]:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20220125100654.424570-2D1-2Dmaximmi-40nvidia.com_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=2bd4kP44ECYFgf-KoNSJWqEipEtpxXnNBKy0vyoJJ8A&m=BHYls0vs10PjYQd-g7Lv51bPiN5Ay-x1lca_mGg_S_tH2pfwR7uADDykRTMmtVcU&s=wHguR00zCQGIop1-2XwsXa_PWXD-J8hMRKhtIuWXjOE&e=
>
> I think it's a good idea, unless you want to change the API to pass all HTB parameters to drivers, see the next paragraph.
>
>> 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.
>
> How can netdevs check unsupported parameters if they don't even receive them
> from HTB? The checks in HTB block parameters that aren't even part of the API.
> If you extend the API (for example, with a new parameter), you have to make sure
> existing drivers are not broken.
>
>> 
>> >>  	}
>> >>  
>> >>  	/* 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
>> >>