Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 14 Jan 2023 03:50:04 +0200
From:   Maxim Mikityanskiy <maxtram95@gmail.com>
To:     Rahul Rameshbabu <rrameshbabu@nvidia.com>
Cc:     Hariprasad Kelam <hkelam@marvell.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, kuba@kernel.org, davem@davemloft.net,
        pabeni@redhat.com, edumazet@google.com, sgoutham@marvell.com,
        lcherian@marvell.com, gakula@marvell.com, jerinj@marvell.com,
        sbhatta@marvell.com, Naveen Mamindlapalli <naveenm@marvell.com>,
        Tariq Toukan <tariqt@nvidia.com>,
        Saeed Mahameed <saeedm@nvidia.com>,
        Gal Pressman <gal@nvidia.com>
Subject: Re: [net-next PATCH 1/5] sch_htb: Allow HTB priority parameter in
 offload mode
Message-ID: <Y8IKTP1hf21oLYvL@mail.gmail.com>
References: <20230112173120.23312-1-hkelam@marvell.com>
 <20230112173120.23312-2-hkelam@marvell.com>
 <Y8FMWs3XKuI+t0zW@mail.gmail.com>
 <87k01q400j.fsf@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87k01q400j.fsf@nvidia.com>
Precedence: bulk

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
> >>