Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Mon, 16 Oct 2023 18:41:48 +0200
From:   Damian Muszynski <damian.muszynski@intel.com>
To:     Tero Kristo <tero.kristo@intel.com>
CC:     "herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        qat-linux <qat-linux@intel.com>,
        "Cabiddu, Giovanni" <giovanni.cabiddu@intel.com>
Subject: Re: [PATCH 08/11] crypto: qat - add rate limiting feature to qat_4xxx
Message-ID: <ZS1nzBes3laXwAW1@dmuszyns-mobl.ger.corp.intel.com>
References: <20231011121934.45255-1-damian.muszynski@intel.com>
 <0dfe871ec970482090680d4fa2423243@DM4PR11MB8129.namprd11.prod.outlook.com>
 <30b7af7e-b3ab-4277-80a4-28bd4cf917d5@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <30b7af7e-b3ab-4277-80a4-28bd4cf917d5@intel.com>
Organization: Intel Technology Poland sp. z o.o. - ul. Slowackiego 173,
 80-298 Gdansk - KRS 101882 - NIP 957-07-52-316
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?8ojYjWE86mkiEvV3aBGToE6juTnVPdd3F9taKMQA1ccraWJdy8q+hYeA90AU?=
 =?us-ascii?Q?0MqvZB9sI25fJEQEU8mQWYviGHDABMXj7Sg+35tCWBwrcTDedUqTaqlYvt7l?=
 =?us-ascii?Q?BrNuTNE8umRyaccm84gI0QgKLv9XvfZEyrUtYaKmCbwoO27VBkTL5xOv9bCG?=
 =?us-ascii?Q?UsPY+Uvaakb2QlyOL9GsvsaQc/B3/f1mjLPLla2lxMEL+BNz81aFNuMBnMg8?=
 =?us-ascii?Q?0Joa3azruw9jhX+vnnJsg6Iq8DDSW5x4ZvmSAqgzN+suv4suZwDkAEpQbpXF?=
 =?us-ascii?Q?8Krv/yADIRQ1u1S2h7TU5Cr5C3kfVrTSKWzFLVBNEi3RofV4e2ihgkWnAqzj?=
 =?us-ascii?Q?gA8TrQO37ylU/22zQcuXQ+I58DwFJ7C2VWMUK4cvPxyfJX6yOLDUsoZd2xK2?=
 =?us-ascii?Q?ulCliaoFr5hc5LPu/1MTpTFh8aeIRaszaRSdn9wg246bFKkMpBWCnyWFOo6E?=
 =?us-ascii?Q?+lYpL2/KO0yIwgCKM49wsv9eam33EsMHTifBGj1WNt4i5aGCHnbQUDPCDuc5?=
 =?us-ascii?Q?PrhLVoPj3lf6nwMoE/shbmsMKqol/OQqAsSYoeK3/5JmSZAn6J8OwCB/+uh5?=
 =?us-ascii?Q?/sveMlp2V2bHx5OhntWKktlz7eRLH4MgW7nsHcoXMrzvObc79vetoakNnXr/?=
 =?us-ascii?Q?/8PbHVrn3/UIZ9hGGItLY6PzKIp5YPJG0atXRpao6scqEVJ2EqkBL4zVcc/h?=
 =?us-ascii?Q?s995C+nyRHB0XwKP/MNCmiBKmBiZanqlGV8OpxOpFQb0NJkqT/9HMNJn2XFb?=
 =?us-ascii?Q?wvP1M9xyYDc9uh9DF3cSButk0k52SL9TZYmWwHhitqSrqeAkpnWNVCWRT/LY?=
 =?us-ascii?Q?YAXKCgEF8rABkKwz0ZbMwL1a9NWw32Cb06NMK4w/VasqK+kILHM66DYj/yu+?=
 =?us-ascii?Q?bN6buVEiQAUaGQepi2xeKKw0JWvehJVYvkrF5irDPIB/ZXt4tG3X+nxQr658?=
 =?us-ascii?Q?GReq/WMBy0E16FCW6TzRTbaZ6xvDyOyWX+IfPc8qMb/Jegu0/yTnnLeaSmVx?=
 =?us-ascii?Q?oOZRhOwFz9wygY7yu1CtOBxkZ2t6EUEY1R9CPxmvRCIFKDsl7FdFTsudUnFe?=
 =?us-ascii?Q?OJRwcNLxKmZbxmzoe4N5IihWyoGWa/D3e5QSmVDYtQq3Au9BJJ8UByPgA2nD?=
 =?us-ascii?Q?iN5rMzU2Jy+Qzi31NxocaHPckNqbofgivWdrGSI4VY+H4x17UYht1dg9QFoo?=
 =?us-ascii?Q?MEzG2l3dImMfoANR/30gTEukiCzSuGCQC1x9vQ73jLpjSjO21n5PzsV9yY4x?=
 =?us-ascii?Q?5wAc1qOKTFVXvf5EBC+xVfB4KWq/sR3BSyZCLS1C0B/LSNEHwYl9QGgxvhMT?=
 =?us-ascii?Q?+UW+6JRb/xJmoDRXwAzIkGCUvSp1du8/y1pKPWZx7vb6i5YhM/bA5T9M1wHS?=
 =?us-ascii?Q?c4DiVbTU9XU4PTWp2Sph0zn85mxmpRIkUMhhjJwwhzrj43gHTBQkNwNXIimy?=
 =?us-ascii?Q?m8ytzcOH0aDRGdSdr0E8CT4mo2wPwYkK21YZus1q5nkh0kskuCC5YrSmCnCx?=
 =?us-ascii?Q?4WB/Q1UcAjBgyX+tnWIJjro4UKdnXIf0zwyIWjv9ULCOZaQ3f9ukAACk0y1U?=
 =?us-ascii?Q?RwjCB+vDnvgSKnRMHShqaHTDzBWnbWfvwtiDd/3dLqjiwu3Htjvl4CuIFLZg?=
 =?us-ascii?Q?0w=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fa9cae58-cf01-44a6-24c8-08dbce66ce6e
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3734.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Oct 2023 16:41:55.0183
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ksO6jNfJF4P1rpXUB+FT6GzPo4Y7RXDbJDTd6W1iyyP4q9BQqASToSkLeiqnKcs2Q/ttAywU0pcMiq6jknXHKYgU0HbuLZhaU645bPvEeS4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8155
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Mon, 16 Oct 2023 09:53:21 -0700 (PDT)

On 2023-10-16 at 13:53:14 +0300, Tero Kristo wrote:
> Hello,
> 
> On 11/10/2023 15:15, Muszynski, Damian wrote:
> > The Rate Limiting (RL) feature allows to control the rate of requests
> > that can be submitted on a ring pair (RP). This allows sharing a QAT
> > device among multiple users while ensuring a guaranteed throughput.

...

> > +static int validate_user_input(struct adf_accel_dev *accel_dev,
> > +			       struct adf_rl_sla_input_data *sla_in,
> > +			       bool is_update)
> > +{
> > +	const unsigned long rp_mask = sla_in->rp_mask;
> > +	size_t rp_mask_size;
> > +	int i, cnt;
> > +
> > +	if (sla_in->sla_id < RL_SLA_EMPTY_ID || sla_in->sla_id >= RL_NODES_CNT_MAX) {
> 
> Should this be <= RL_SLA_EMPTY_ID? Additionally this check seems unnecessary
> as similar check is done in the validate_sla_id() which gets always called
> in the same path where the validate_user_input() is called.

You're right this check can be safely eliminated.

> 
> > +		dev_notice(&GET_DEV(accel_dev), "Wrong SLA ID\n");
> > +		goto ret_inval;
> > +	}
> > +
> > +	if (sla_in->pir < sla_in->cir) {
> > +		dev_notice(&GET_DEV(accel_dev),
> > +			   "PIR must be >= CIR, setting PIR to CIR\n");
> > +		sla_in->pir = sla_in->cir;
> > +	}
> > +
> > +	if (!is_update) {
> > +		cnt = 0;
> > +		rp_mask_size = sizeof(sla_in->rp_mask) * BITS_PER_BYTE;
> > +		for_each_set_bit(i, &rp_mask, rp_mask_size) {
> > +			if (++cnt > RL_RP_CNT_PER_LEAF_MAX) {
> > +				dev_notice(&GET_DEV(accel_dev),
> > +					   "Too many ring pairs selected for this SLA\n");
> > +				goto ret_inval;
> 
> The error gotos seem useless in this function, maybe just directly return
> -EINVAL.

Ok, will change that.

> 
> > +			}
> > +		}
> > +
> > +		if (sla_in->srv >= ADF_SVC_NONE) {
> > +			dev_notice(&GET_DEV(accel_dev),
> > +				   "Wrong service type\n");
> > +			goto ret_inval;
> > +		}
> > +
> > +		if (sla_in->type > RL_LEAF) {
> > +			dev_notice(&GET_DEV(accel_dev),
> > +				   "Wrong node type\n");
> > +			goto ret_inval;
> > +		}
> > +
> > +		if (sla_in->parent_id < RL_PARENT_DEFAULT_ID ||
> > +		    sla_in->parent_id >= RL_NODES_CNT_MAX) {
> > +			dev_notice(&GET_DEV(accel_dev),
> > +				   "Wrong parent ID\n");
> > +			goto ret_inval;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +ret_inval:
> > +	return -EINVAL;
> > +}
> > +
> > +static int validate_sla_id(struct adf_accel_dev *accel_dev, int sla_id)
> > +{
> > +	struct rl_sla *sla;
> > +
> > +	if (sla_id < 0 || sla_id >= RL_NODES_CNT_MAX)
> > +		goto ret_not_exists;
> > +
> > +	sla = accel_dev->rate_limiting->sla[sla_id];
> > +
> > +	if (!sla)
> > +		goto ret_not_exists;
> > +
> > +	if (sla->type != RL_LEAF) {
> > +		dev_notice(&GET_DEV(accel_dev),
> > +			   "This ID is reserved for internal use\n");
> > +		goto ret_inval;
> 
> Maybe just direct return here.

Done.

...

> > +static void update_budget(struct rl_sla *sla, u32 old_cir, bool is_update)
> > +{
> > +	u32 new_rem;
> > +
> > +	switch (sla->type) {
> > +	case RL_LEAF:
> > +		if (is_update)
> > +			sla->parent->rem_cir += old_cir;
> > +
> > +		sla->parent->rem_cir -= sla->cir;
> > +		sla->rem_cir = 0;
> > +		break;
> > +	case RL_CLUSTER:
> > +		if (is_update) {
> > +			sla->parent->rem_cir += old_cir;
> > +			new_rem = sla->cir - (old_cir - sla->rem_cir);
> > +			sla->rem_cir = new_rem;
> 
> Get rid of new_rem and directly assign.

Ok.

> 
> > +		} else {
> > +			sla->rem_cir = sla->cir;
> > +		}
> > +
> > +		sla->parent->rem_cir -= sla->cir;
> > +		break;
> > +	case RL_ROOT:
> > +		if (is_update) {
> > +			new_rem = sla->cir - (old_cir - sla->rem_cir);
> > +			sla->rem_cir = new_rem;
> 
> Same here. You avoid one local variable that way.

Ok.

...

> > +static int add_new_sla_entry(struct adf_accel_dev *accel_dev,
> > +			     struct adf_rl_sla_input_data *sla_in,
> > +			     struct rl_sla **sla_out)
> > +{
> > +	struct adf_rl *rl_data = accel_dev->rate_limiting;
> > +	struct rl_sla *sla;
> > +	int ret = 0;
> > +
> > +	sla = kzalloc(sizeof(*sla), GFP_KERNEL);
> 
> Why not use devm_kzalloc()? You have access to the device handle.

Our driver allows user to put a device 'down' and 'up'. During that procedure 
many components needs to be unloaded and frees it's memory. This component also 
belongs to that group. So devm_* is pointless here.

...

> > +	if (sla->type == RL_LEAF) {
> > +		ret = prepare_rp_ids(accel_dev, sla, sla_in->rp_mask);
> > +		if (!sla->ring_pairs_cnt || ret) {
> > +			dev_notice(&GET_DEV(accel_dev),
> > +				   "Unable to find ring pairs to assign to the leaf");
> > +			if (!ret)
> > +				ret = -EINVAL;
> > +
> > +			goto ret_err;
> > +		}
> > +	}
> > +
> > +	ret = 0;
> > +
> > +ret_err:
> > +	/* Allocated sla will be freed at the bottom of calling function */
> 
> I would rather recommend you release the sla here if the function fails.

Ok, will add.

...

> > +
> > +		ret = adf_rl_add_sla(accel_dev, &sla_in);
> > +		if (ret)
> > +			goto err_ret;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_ret:
> > +	dev_notice(&GET_DEV(accel_dev),
> > +		   "Initialization of default nodes failed\n");
> 
> dev_notice() here and you also do dev_err() for the exact same reason in the
> callee (adf_rl_start().) You can make this function cleaner by removing the
> dev_notice here, and directly returning in error cases without goto.

Thanks. Will fix that.

...

> > +int adf_rl_remove_sla(struct adf_accel_dev *accel_dev, u32 sla_id)
> > +{
> > +	struct adf_rl *rl_data = accel_dev->rate_limiting;
> > +	struct rl_sla *sla;
> > +	int ret;
> > +
> > +	ret = validate_sla_id(accel_dev, sla_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	sla = rl_data->sla[sla_id];
> > +
> > +	if (sla->type < RL_LEAF && sla->rem_cir != sla->cir) {
> > +		dev_notice(&GET_DEV(accel_dev),
> > +			   "To remove parent SLA all its children must be removed first");
> > +		return -EINVAL;
> > +	}
> 
> Mutex is not protecting the above portion of code, which means you can call
> clear_sla() with identical pointer value multiple times.

Thanks. Will fix that.

> > +
> > +	mutex_lock(&rl_data->rl_lock);
> > +	clear_sla(rl_data, sla);
> > +	mutex_unlock(&rl_data->rl_lock);
> > +
> > +	return 0;
> > +}

...

> > +	rl = kzalloc(sizeof(*rl), GFP_KERNEL);
> 
> devm_kzalloc()?

No, explanation above.

...


> > +int adf_rl_send_admin_init_msg(struct adf_accel_dev *accel_dev,
> > +			       struct rl_slice_cnt *slices_int)
> > +{
> > +	struct icp_qat_fw_init_admin_slice_cnt slices_resp = { };
> > +	int ret;
> > +
> > +	ret = adf_send_admin_rl_init(accel_dev, &slices_resp);
> > +	if (ret) > +		goto err_ret;
> 
> Just return ret.

Ok.


---
Damian Muszynski