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:   Tue, 16 May 2023 10:07:04 +0200
From:   Simon Horman <simon.horman@corigine.com>
To:     Hariprasad Kelam <hkelam@marvell.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        kuba@kernel.org, davem@davemloft.net,
        willemdebruijn.kernel@gmail.com, andrew@lunn.ch,
        sgoutham@marvell.com, lcherian@marvell.com, gakula@marvell.com,
        jerinj@marvell.com, sbhatta@marvell.com, naveenm@marvell.com,
        edumazet@google.com, pabeni@redhat.com, jhs@mojatatu.com,
        xiyou.wangcong@gmail.com, jiri@resnulli.us, maxtram95@gmail.com,
        corbet@lwn.net, linux-doc@vger.kernel.org
Subject: Re: [net-next Patch v10 4/8] octeontx2-pf: Refactor schedular queue
 alloc/free calls
Message-ID: <ZGM5qO+Uhea7eGGt@corigine.com>
References: <20230513085143.3289-1-hkelam@marvell.com>
 <20230513085143.3289-5-hkelam@marvell.com>
 <ZGM5VmGcuEFG2Jh6@corigine.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZGM5VmGcuEFG2Jh6@corigine.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?s172XiiF9oXrabOlz6oYRH7rbxUBsjMmq0/GkHTDZAvhkz2+SG0pz8HdCJLl?=
 =?us-ascii?Q?50oXLedpLRLbmd9yjVheUxyoFSA6ZVHWpyn1N3XRrccOYoIaYoQdxFBObM2V?=
 =?us-ascii?Q?kqezqTIa8ZD4XUbGMBWaFHoY/Akp//UnY6eESe6Xis9vBbGVbSt/ErRkaamP?=
 =?us-ascii?Q?avO8A1aAeSCXqsbsjFWCuNj4MIy8kQ5tr3WVqzBHMonR3cCXXAFzb95oeIPt?=
 =?us-ascii?Q?IQvQ81ufa2XLGJT8WaFC3XzJPO+1RfwA/34hdX6GMpuvb3CkOthlxJBMS29V?=
 =?us-ascii?Q?Zobd4+z48+92tlzpQK+S8FYte7na0CxCRccB1ysxXgCfAWLdoiZtw7KckCae?=
 =?us-ascii?Q?fjXoVP9hlHwDsMkbSWojS6JuZkyIDCczM7j2D9yGRNjZjC1FMXoZFIr2wL8b?=
 =?us-ascii?Q?hNTDP37S2TPwIP18d+uG9z1kCJZ3BDRZrs4ShfRLn2b22mbzx8MtmRP1/UW8?=
 =?us-ascii?Q?+PS1XlM69MrGPMVzsu/o7bR1xIJiUSurj40Hq5C5SUTtdjDpZ8niHEa4njIz?=
 =?us-ascii?Q?3u3CrkVG4R/ysBM47WQ1T85ssPYjAfEIcD4JLJztihG1O/6QOrLoZrK0k8EI?=
 =?us-ascii?Q?kqOacSXc9r2W5Rtqt3i15/UQxs1gVPe8Jenm449OHOibyyI4PfLYXYZVhl9c?=
 =?us-ascii?Q?xAa1zF0+R30BVhaKaPH/c1YplhhRgp+DOTUBUuhzp6yooTo96VemytAmvfDR?=
 =?us-ascii?Q?UmL8YPEggFeN2xVZVP0bquasUH7UfsE0K5WIRjZO6mdXLHf6n3Ot5Ke1+EBA?=
 =?us-ascii?Q?i9BFNB0w/tZrmRDhzj6JcuL9CO9kxzDiON/rKoCl2cLF5JwrFY6Dr01Uofe+?=
 =?us-ascii?Q?CORNTNb4wwxqLpeTaA7mZ6nRdXPJrbI8adZQtJlXUU+vblwTXjIGo9yaQ8g5?=
 =?us-ascii?Q?xIAHhmfd9jHYyf/IQPwMaWyIhrj0JKQfydbqHFZdY9oqtMtnelGKkloH+bc/?=
 =?us-ascii?Q?BNmp/ZeWrWUdFpnuwCKII4k47YCTXHShEWhBCEJ1EfUrKdnUSeHrBQhHAYI0?=
 =?us-ascii?Q?nj5gwGGROZ2j4SKHvFIzEjHYbTnnWYN9aV0hVb9I8PmOnkMc+SWPPKXP/mT5?=
 =?us-ascii?Q?S/Bii3s/C3ATmSIbE59UL94JrabjB6fdgA6qGq4lNhlVz+UWj+Uld4dqkj8e?=
 =?us-ascii?Q?dNGgGv2YIAoe+TvW9nfu3F9ccWDtAzsHue54JNgqsNNshjwZYdxZEALyfEzH?=
 =?us-ascii?Q?4JuEghff/SSND8jck3fuh6IGFsMkFku8RS3so0fcEkeg2Y3S7B5quxvcZlzn?=
 =?us-ascii?Q?BTNXnj84TMi5c8G9k44xh+QDoWTfOeD7RsELuvrrtqd3y+UTd9vA7Hg/UviE?=
 =?us-ascii?Q?2uxq0EC+iD+2fWnW1JEg+VSkhH+aUmfVAhXtPn91mWuPXivMAZ4k0uZ8bNPx?=
 =?us-ascii?Q?20M8UQU/AjZOpy6aBe3sMVp/xW6MWqKWsizcCUKbgxZz89lTR0TEofk/uV9h?=
 =?us-ascii?Q?zJdz+kiwGV/HLFRAFV7pWDErTvgtsgDSeqGis8D9ot+b2vBDYIufF+kIaw2x?=
 =?us-ascii?Q?qje7rirxVmRq6dMsQvy9BYf2uBmY19YC8CBOigDnSTp33TkvF9zCw+SvvoQs?=
 =?us-ascii?Q?jr6v2GsIkbd8roLY5Asre0gyXGixALFAn+tqgOGLl1ILh+fWKamSPcKU2LdW?=
 =?us-ascii?Q?GBiqjSPWJ5TTOkYCegpaS/IkERgNZtElJNqvKTXddTat3rqPWsh3h2uFBGIh?=
 =?us-ascii?Q?Xyw3mg=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 072fa85b-98df-4edc-f0c4-08db55e48dc4
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 May 2023 08:07:12.3797
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: K1UJnq6m+30JRjMyyTB6DeUVKGOudr1vPUTWT52Whd1gnkRLbsfLcfnYMUJj2tm3Hh5OMmeWohwe8wA6tRPaxDSM2ShImoP9kxCrtRsSM7o=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR13MB4840
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham 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 Tue, May 16, 2023 at 10:05:54AM +0200, Simon Horman wrote:
> Hi Hariprasad,
> 
> On Sat, May 13, 2023 at 02:21:39PM +0530, Hariprasad Kelam wrote:
> > 1. Upon txschq free request, the transmit schedular config in hardware
> > is not getting reset. This patch adds necessary changes to do the same.
> > 
> 
> nit: s/schedular/scheduler/
> 
> > 2. Current implementation calls txschq alloc during interface
> > initialization and in response handler updates the default txschq array.
> > This creates a problem for htb offload where txsch alloc will be called
> > for every tc class. This patch addresses the issue by reading txschq
> > response in mbox caller function instead in the response handler.
> > 
> > 3. Current otx2_txschq_stop routine tries to free all txschq nodes
> > allocated to the interface. This creates a problem for htb offload.
> > This patch introduces the otx2_txschq_free_one to free txschq in a
> > given level.
> 
> This patch seems to be doing three things.
> Could it be split into three patches?

I see that I was a bit late with my review as the
series was applied yesterday.

> ...
> 
> > -int otx2_txschq_stop(struct otx2_nic *pfvf)
> > +void otx2_txschq_free_one(struct otx2_nic *pfvf, u16 lvl, u16 schq)
> >  {
> >  	struct nix_txsch_free_req *free_req;
> > -	int lvl, schq, err;
> > +	int err;
> >  
> >  	mutex_lock(&pfvf->mbox.lock);
> > -	/* Free the transmit schedulers */
> > +
> >  	free_req = otx2_mbox_alloc_msg_nix_txsch_free(&pfvf->mbox);
> 
> Mainly for my own edification:
> 
> 	- otx2_mbox_alloc_msg_nix_txsch_free is created via the
> 	  M(_name, _id, _fn_name, _req_type, _rsp_type) macro
> 	  around line 844 of  otx2_common.h
> 	- It calls otx2_mbox_alloc_msg_rsp
> 	- Which does not call any allocation functions such as kmalloc
> 
> >  	if (!free_req) {
> >  		mutex_unlock(&pfvf->mbox.lock);
> > -		return -ENOMEM;
> > +		netdev_err(pfvf->netdev,
> > +			   "Failed alloc txschq free req\n");
> 
> I think that given the above it's ok to log an error here.
> As the allocation core won't have (because it's not used here.
> But I wonder if it would be more consistent with how
> allocation errors are usually handled to move the logging into
> otx2_mbox_alloc_msg_rsp().
> 
> > +		return;
> >  	}
> >  
> > -	free_req->flags = TXSCHQ_FREE_ALL;
> > +	free_req->schq_lvl = lvl;
> > +	free_req->schq = schq;
> > +
> >  	err = otx2_sync_mbox_msg(&pfvf->mbox);
> > +	if (err) {
> > +		netdev_err(pfvf->netdev,
> > +			   "Failed stop txschq %d at level %d\n", schq, lvl);
> > +	}
> > +
> >  	mutex_unlock(&pfvf->mbox.lock);
> > +}
> > +
> > +void otx2_txschq_stop(struct otx2_nic *pfvf)
> > +{
> > +	int lvl, schq;
> > +
> > +	/* free non QOS TLx nodes */
> > +	for (lvl = 0; lvl < NIX_TXSCH_LVL_CNT; lvl++)
> > +		otx2_txschq_free_one(pfvf, lvl,
> > +				     pfvf->hw.txschq_list[lvl][0]);
> >  
> >  	/* Clear the txschq list */
> >  	for (lvl = 0; lvl < NIX_TXSCH_LVL_CNT; lvl++) {
> >  		for (schq = 0; schq < MAX_TXSCHQ_PER_FUNC; schq++)
> >  			pfvf->hw.txschq_list[lvl][schq] = 0;
> >  	}
> > -	return err;
> > +
> 
> nit: no blank line here.
> 
> >  }
> >  
> >  void otx2_sqb_flush(struct otx2_nic *pfvf)
> 
> ...