Message-ID: <Y8U0h/TVw5dCpPEq@mail.gmail.com>
Date:   Mon, 16 Jan 2023 13:27:03 +0200
From:   Maxim Mikityanskiy <maxtram95@...il.com>
To:     Rahul Rameshbabu <rrameshbabu@...dia.com>
Cc:     Hariprasad Kelam <hkelam@...vell.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kuba@...nel.org, davem@...emloft.net,
        pabeni@...hat.com, edumazet@...gle.com, sgoutham@...vell.com,
        lcherian@...vell.com, gakula@...vell.com, jerinj@...vell.com,
        sbhatta@...vell.com, Naveen Mamindlapalli <naveenm@...vell.com>,
        Tariq Toukan <tariqt@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Gal Pressman <gal@...dia.com>
Subject: Re: [net-next PATCH 1/5] sch_htb: Allow HTB priority parameter in
 offload mode

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@...il.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@...il.com> wrote:
> >> > On Thu, Jan 12, 2023 at 11:01:16PM +0530, Hariprasad Kelam wrote:
> >> >> From: Naveen Mamindlapalli <naveenm@...vell.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@...vell.com>
> >> >> Signed-off-by: Hariprasad Kelam <hkelam@...vell.com>
> >> >> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@...vell.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
> >> >> 

Powered by blists - more mailing lists