| Message ID | 20230112173120.23312-2-hkelam@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | octeontx2-pf: HTB offload support | expand |
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. > } > > /* 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 >
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 >>
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://lore.kernel.org/all/20220113110801.7c1a6347@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net/ [2]: https://lore.kernel.org/all/20220125100654.424570-1-maximmi@nvidia.com/ 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 > >>
On Sat, 14 Jan, 2023 03:50:04 +0200 Maxim Mikityanskiy <maxtram95@gmail.com> wrote: > 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://lore.kernel.org/all/20220113110801.7c1a6347@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net/ > [2]: https://lore.kernel.org/all/20220125100654.424570-1-maximmi@nvidia.com/ > > 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. They can if tc_htb_qopt_offload contains placeholders for every unsupported parameter. Then, the responsibility of indicating whether those parameters are supported in an offload or not moves to the drivers that should look at those parameters and see if they are set with non-default values. My concern is that, if another driver implementer decides to introduce support for one of the other parameters, this becomes a growing chain of maintainers that need to review since every driver that chooses to adopt htb offload is subject to being impacted by future parameters supported. I agree this is unavoidable, if a new parameter is added for htb altogether, but if netdev drivers adopt a practice of checking these parameters, it becomes very easy for a feature author to just add another check in those drivers that already make use of htb offload. In the patch you authored and linked, you mention the following. If future drivers support more offload parameters, the checks can be moved to the driver side. I think this is good opportunity to make that move. Btw, as you previously pointed, this is the change in behavior seen but the mlx5 netdevs do not support the prio parameter, so we will need to add a driver side check. /opt/mellanox/iproute2/sbin/tc class add dev eth2 classid 1:3 parent 1: htb rate 3596mbit burst 89900kbit cburst 89900kbit prio 8 Error: HTB offload doesn't support the prio parameter. /opt/mellanox/iproute2/sbin/tc class add dev eth2 classid 1:3 parent 1: htb rate 3596mbit burst 89900kbit cburst 89900kbit prio 8 # No error since support is being added in the htb offload API > >> >> >> } >> >> >> >> /* 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 >> >>
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. 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 > >>
On Fri, Jan 13, 2023 at 06:18:37PM -0800, Rahul Rameshbabu wrote: > On Sat, 14 Jan, 2023 03:50:04 +0200 Maxim Mikityanskiy <maxtram95@gmail.com> wrote: > > 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://lore.kernel.org/all/20220113110801.7c1a6347@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net/ > > [2]: https://lore.kernel.org/all/20220125100654.424570-1-maximmi@nvidia.com/ > > > > 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. > > They can if tc_htb_qopt_offload contains placeholders for every > unsupported parameter. Then, the responsibility of indicating whether > those parameters are supported in an offload or not moves to the drivers > that should look at those parameters and see if they are set with > non-default values. Yes, but why add fields and pass them to the drivers if they are not used, and some of them probably will never be used? This approach is also not a role model of robustness, because: 1. The need to add these checks to each driver is not obvious, and a new driver may easily miss them. As a reviewer, I will not even notice it, because I don't know hardware specifics of each and every NIC. 2. Let's say, a new parameter gets added to HTB. In this case, a check will have to be added to every driver, or HTB will end up having "supported for offload" and "unsupported for offload" fields anyway, just as it has now. > My concern is that, if another driver implementer decides to introduce > support for one of the other parameters, this becomes a growing chain of > maintainers that need to review since every driver that chooses to adopt > htb offload is subject to being impacted by future parameters supported. > I agree this is unavoidable, if a new parameter is added for htb > altogether, but if netdev drivers adopt a practice of checking these > parameters, it becomes very easy for a feature author to just add > another check in those drivers that already make use of htb offload. Yes, your suggestion simplifies life for authors of existing drivers who want to add a new feature, but it's more risky for new drivers, and it doesn't work when HTB itself wants to add a new parameter. Status quo is something that I would expect to be more natural for kernel developers: 1. If you are an author of a new driver, implement the API in whole, or it's your responsibility to check for unsupported parts. 2. If you are extending the API, make sure to fix the existing users. > In the patch you authored and linked, you mention the following. > > If future drivers support more offload parameters, the checks can be moved > to the driver side. > > I think this is good opportunity to make that move. I didn't mean "if a future driver supports one more parameter, let's make all drivers check all parameters". The meaning is: the list of unsupported parameters is not carved in stone; if a new driver supports more parameters, the checks for those parameters can be moved to the drivers. In our case, it's one new parameter. > > Btw, as you previously pointed, this is the change in behavior seen but > the mlx5 netdevs do not support the prio parameter, so we will need to > add a driver side check. I would expect the patch authors to do it :) BTW, discussing where to do the checks is all fun until you realize not all parameters can even be checked on HTB level, for example, we can't detect whether the user has set buffer and cbuffer, because tc assigns some defaults if they are omitted from the command line. If you have good ideas how it can be fixed, please share. > > /opt/mellanox/iproute2/sbin/tc class add dev eth2 classid 1:3 parent 1: htb rate 3596mbit burst 89900kbit cburst 89900kbit prio 8 > Error: HTB offload doesn't support the prio parameter. > > /opt/mellanox/iproute2/sbin/tc class add dev eth2 classid 1:3 parent 1: htb rate 3596mbit burst 89900kbit cburst 89900kbit prio 8 > # No error since support is being added in the htb offload API > > > > >> > >> >> } > >> >> > >> >> /* 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 > >> >>
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 >> >>
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; - } } /* 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);