linux-kernel - Re: [net-next Patch v10 4/8] octeontx2-pf: Refactor schedular queue alloc/free calls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 May 2023 10:05:42 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Hariprasad Kelam <hkelam@...vell.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kuba@...nel.org, davem@...emloft.net,
        willemdebruijn.kernel@...il.com, andrew@...n.ch,
        sgoutham@...vell.com, lcherian@...vell.com, gakula@...vell.com,
        jerinj@...vell.com, sbhatta@...vell.com, naveenm@...vell.com,
        edumazet@...gle.com, pabeni@...hat.com, jhs@...atatu.com,
        xiyou.wangcong@...il.com, jiri@...nulli.us, maxtram95@...il.com,
        corbet@....net, linux-doc@...r.kernel.org
Subject: Re: [net-next Patch v10 4/8] octeontx2-pf: Refactor schedular queue
 alloc/free calls

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)

...