Messages in this thread Patch in this message | | | From | Rahul Rameshbabu <> | Subject | Re: [net-next PATCH 1/5] sch_htb: Allow HTB priority parameter in offload mode | Date | Tue, 17 Jan 2023 20:31:40 -0800 |
| |
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 >> >>
| |