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:05:42 +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: <ZGM5VmGcuEFG2Jh6@corigine.com>
References: <20230513085143.3289-1-hkelam@marvell.com>
 <20230513085143.3289-5-hkelam@marvell.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230513085143.3289-5-hkelam@marvell.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?gLLoFbA07zQuboSjI01O6Z264XFxSB1rjS38srIKnQcn62QW5WK+TCyqcEMQ?=
 =?us-ascii?Q?zxRfvXTvYe0MSJV2PvQRQq2pHsnfvn2QqBRbZQGFmpO1w8OgrcuhV58rLTBy?=
 =?us-ascii?Q?AZVESTBA5UGWwZAuR5ItyWGcWmHbbNUaG7GOGvUXYVtPnK7pBsHsbZN9Igg8?=
 =?us-ascii?Q?VUobFvUnwpykcqg6HZsnX++YTvcZBPVG2DTjrwbRxeuQ96tBbGaPDQgKkhTB?=
 =?us-ascii?Q?nsHmKfd5PgZwpgiJcddMbpswfgGBHkAYsL7s15GRPAoFHhM5dGPIzui4don+?=
 =?us-ascii?Q?DIQLorDwOA6nQu1gLjJYnfxu7RohItV4QLuatHt6oCGniskaeLLcrhll+Uuy?=
 =?us-ascii?Q?dQZ1bQpY7bsI2A/8Sj4xo717Q24UXmjUoRpatSqGjFj9T0/KObEe6bX/zTll?=
 =?us-ascii?Q?2GjcRWE7r6YCqP7sOoQfPVD7PnrEYjgGZq9TPXBe3jAOp53brQt9nEIqOaEU?=
 =?us-ascii?Q?TKL7e+ESJlZnuKS+5BtWbybuY2GCUht73JhnWMWjLhlHIXBEceCQu/E7l4EU?=
 =?us-ascii?Q?EBYIw/AN84cmc02JLdc0KctUi+iptS3iHFM2a+g9XoD3xVep3HRHMnk3kMn0?=
 =?us-ascii?Q?AdgKPxd3LYuhMIJ1kq14LAtFi8/dCmTBx9j+xsEc2uIqAT0FjQWXiH3fiIbP?=
 =?us-ascii?Q?oLLj4niKAo5JtzW0UoqMd4QrQu36WWuDDCdBqAuWAaYYCKOGe6at00mB1tfW?=
 =?us-ascii?Q?koBRWJYCTJHHfxWYv4YkeVV38jqU14Yrt0H9YtGB6hI3enHdyeg0ziPChdMR?=
 =?us-ascii?Q?I5c4MfxepHcIPMzblcpvtY/eAzMnpWbmF1HdDkT/78Xao8rz337zAdiNVZFi?=
 =?us-ascii?Q?t3tuqsNHdOeSF+aV7LRcfT5ANQWjkeKIVclznkGxposLJQCw1Va2kUyoybTz?=
 =?us-ascii?Q?bBJADfJVkbQuFoZiAfentgJYQziNS2aLJZszYQeaDRUNW/kzNzbzbINSosFA?=
 =?us-ascii?Q?WF7toQFCODAx1MwEA/rcRaigxSBG0nXR/jg0stdXecOMgfgVaNzbTqHy1TDX?=
 =?us-ascii?Q?307ph1U3/fcS5qksr9cMSZySMbD0uVhkySroDgIapWV3JijbKY0H7kK2B6Iz?=
 =?us-ascii?Q?MRJXY6PPs+dTcxI3wi4f/5vLgowIt/VzUHpNED0m9b5hQ7E3nun7iNAYlmEU?=
 =?us-ascii?Q?l2ayfqR0OVtsE236c+q2oCDsmOfP16MLoSX6Twfjp9+bEDk5oalw2rdraepR?=
 =?us-ascii?Q?M+GLIWKJuYO53zD/IjzRlTuawzFrnVm2sR4v8R759voS4AtBUqUc9LNzNqgE?=
 =?us-ascii?Q?0QXy7H6C4OREas8xv/zB3OHB/120hib0MS8b8g73560+Zp+5Huskm3yniB2W?=
 =?us-ascii?Q?/qXXw257bB0H2W6RjP2fPrPISRc81yqChgQO+thNwszg2Nrin2rmPnbONfKe?=
 =?us-ascii?Q?Mio6EJ0siO3d8Tv9vQdxmEVpoOEmSKcV436rvo0Wjlr9wPupyV+ChKVnLX5m?=
 =?us-ascii?Q?ZQedn5P5IJ0PZu3/DVRhOutI3homDD1gP3x7edHAtNWEmHvuWUFEbrP7nAQa?=
 =?us-ascii?Q?5Tg7lPYFTwv0YxJLpsRw6mbcWmoW0vrTy5UjOzdqAghXmUKm7UXmX9L5gPh0?=
 =?us-ascii?Q?S3qEwl8egZ3KNDN6g81h/gmcDjaHH5TFZBwXxc05EGdN8rqmBuSEfPrq8sB8?=
 =?us-ascii?Q?jX6/tE+tlnUr623VnKpvNFD3urx06l8sIgoo+lsYPsxRSb2a2IUd/wyqVb1l?=
 =?us-ascii?Q?NE5OqA=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dc06bb30-965d-4b3b-c216-08db55e45d99
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:05:51.5803
 (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: N0QQ9PRlBj/3crDPTsuhxkRQlsaFKGt6WwrnxLGChNdIWQQAkJMrKBQ3Lma5+DNe8UwAzUuC3sf35S/NUQP2+MCETL7GMHHkdGha4q+HhkI=
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

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?

...

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

...