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;
From:   Rahul Rameshbabu <rrameshbabu@nvidia.com>
To:     Maxim Mikityanskiy <maxtram95@gmail.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
References: <20230112173120.23312-1-hkelam@marvell.com>
        <20230112173120.23312-2-hkelam@marvell.com>
        <Y8FMWs3XKuI+t0zW@mail.gmail.com>
Date:   Fri, 13 Jan 2023 13:06:52 -0800
In-Reply-To: <Y8FMWs3XKuI+t0zW@mail.gmail.com> (Maxim Mikityanskiy's message
        of "Fri, 13 Jan 2023 14:19:38 +0200")
Message-ID: <87k01q400j.fsf@nvidia.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.1 (gnu/linux)
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?82YKmyRJCNJXY0Cu3BxhcXBJGqoejSXCScb7ja41MPTdEGKGZwHCmYLiSMTR?=
 =?us-ascii?Q?gStDG4f09CCTCw+G3VHDQ/W5Mk2MhAYaHhz/c9JEo4V4jX73XsGPJPR8sqz/?=
 =?us-ascii?Q?7VdWt4U4Ho62P6j1l3DV6m4oODEdX5MZNz/4morbCCVot02W/j4ub/LCn8TK?=
 =?us-ascii?Q?Zal51U6osyleX+a9ByY32/74rMlPtmg3R2ujOEynTudkFzH18xAP7Yf690d2?=
 =?us-ascii?Q?YFb1gUsMofAmK6OJvvHAdUlunbG90I8JdIi5DpHC3siKEaRvLGCFcGLu+IHX?=
 =?us-ascii?Q?CCFaswNrdZfXUgWOLL4CfXQFgtrYhXWZYJqArb+dTumINUr26weFP6Qi62TL?=
 =?us-ascii?Q?seOvGCAZ3yn6hVtoSKjHaPhQYszYZoi5O5kgDnYX03gYvFBubxI2SfWeUhPN?=
 =?us-ascii?Q?9xqnNh1EVf+GPXe14+giGNewOkzH/DkfWpV3Faj5XBu+YWgng3AZKTmkWG8z?=
 =?us-ascii?Q?6gl4v5WqjhawDx6KW0wpRCE5WYNwSxS6dpRRv+9MV9RLOuIQDH1+5etXVmnZ?=
 =?us-ascii?Q?dMmtJmqhP9guO/0+bHIspuc8dH0Vgb6f7ZQie4MZlWVG7e5WTKTB8ysJZfdm?=
 =?us-ascii?Q?PSYPh3XzH3w4vL0BXqSAkBa7FKOfQleEtFUeIrxDzvCXT3qnQZdkX+EVXP+a?=
 =?us-ascii?Q?EYUcod1kU09pfamHKi+XMF2Cw1rS0rhYzRMOYh8/beemKSzpYDptP8PtYfyU?=
 =?us-ascii?Q?nBqHrHrgHe0ZTlsemA/gxiiNvah0ryEcufkmq6CEQQI1Ti4aK2qkQbm25hnR?=
 =?us-ascii?Q?qYDIRlRgeUhnKtRol/8Zw7f8bzPyHiTodo7oWjMHZ29TAdoggoCrTpeaZz8i?=
 =?us-ascii?Q?Z43XsGtCUEhmT+2F91zDgzkzfrvm0dSWBsftJlNkO9aMIRe7D2I9VLYcY0UT?=
 =?us-ascii?Q?FOKo14JNisl3qp5Xv9/3YFvE+zbj8x9eYeSDLEVz1JzpoohmayFmt2dmf+XI?=
 =?us-ascii?Q?ChgFx/d8G7onwchzlb9PXxmdopMCywS5kSBEn/LHcOjDts1sUxsjAj/DUkfP?=
 =?us-ascii?Q?z9pO1ZgprMYqLnJs0ScVFvtgQzc3AVq09yoZeNqBnBsbj4IzMkRH7ni7NN2q?=
 =?us-ascii?Q?G0yD5Cyd0n1h80WdbEm6LfEjQEhM51vwFWiO0uT8RsGrmvrE/BhKwlB+M5Ss?=
 =?us-ascii?Q?7ci6dy0MfxjA9t+9UQh0HV3jejChdziprYGQA5Ac8ZgUd7zStZ6wDxuyWYGV?=
 =?us-ascii?Q?wwVUfCrd9bZ0JSwA2tHdngSdxS5jBG4D31rizPIlcVodAUHGZ2OQWYW2M+Gy?=
 =?us-ascii?Q?4CZlZavao+7UBLzJv4c/TQAKxRk2+n++bJduIjr3bXJ0pSbaQcBw/Q9gGtl0?=
 =?us-ascii?Q?s0jMxFnNehjNU8SlYPul8jCqsG0y7m5GeasbkZOnzONtVX3ngOiG8hwc1hnS?=
 =?us-ascii?Q?LGsO0u3UKP1ISU+vl0gF2QsQAo0+DrqC+C5mIAAb+BDaWTgeDINBxxR8zvdw?=
 =?us-ascii?Q?PCnVso1ib39P+8hRBHwWRvWdqXfamNAHdEi922ctEbJFN/yXuVGuGS4leoE5?=
 =?us-ascii?Q?zkySTRUZNZWmyubaekcIg9y61wH5BRgNr2gZWQpaSSC2vjYziH26vvCqhVC0?=
 =?us-ascii?Q?JtsjIcHxDbYK4mxBMAVITr4tMY1IgrMwhBTMW11GXchRhL+OZVrlSptC6w73?=
 =?us-ascii?Q?cemMV08MUf8uhK5zSPD3VF+vthWcoesP/UmK+poq+dK4TwkaaKxcVCyN6m+1?=
 =?us-ascii?Q?3+Ca8w=3D=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 961dfa00-007f-4a96-8288-08daf5aa1c5f
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB2743.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jan 2023 21:06:59.7800
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Qs8R3T866K31zi/9T6wDz6n9eZPgKi7JnOcYcrsrS1NjuwuevMod4pe1OW3FxqzLx1+Pm+YuaQIh9UqRdNblYQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB5643
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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