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 | Fri, 13 Jan 2023 13:06:52 -0800 |
| |
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 >>
| |