Re: [dpdk-dev] [PATCH v2 4/4] regexdev: implement regex rte level functions

[Ori Kam <orika@mellanox.com>]

Hi Guy,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Guy Kaneti
> Sent: Tuesday, April 21, 2020 2:36 PM
> To: Ori Kam <orika@mellanox.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; xiang.w.wang@intel.com
> Cc: dev@dpdk.org; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; Shahaf Shuler <shahafs@mellanox.com>;
> hemant.agrawal@nxp.com; Opher Reviv <opher@mellanox.com>; Alex
> Rosenbaum <alexr@mellanox.com>; Dovrat Zifroni <dovrat@marvell.com>;
> Prasun Kapoor <pkapoor@marvell.com>; nipun.gupta@nxp.com;
> bruce.richardson@intel.com; yang.a.hong@intel.com; harry.chang@intel.com;
> gu.jian1@zte.com.cn; shanjiangh@chinatelecom.cn;
> zhangy.yun@chinatelecom.cn; lixingfu@huachentel.com; wushuai@inspur.com;
> yuyingxia@yxlink.com; fanchenggang@sunyainfo.com;
> davidfgao@tencent.com; liuzhong1@chinaunicom.cn;
> zhaoyong11@huawei.com; oc@yunify.com; jim@netgate.com;
> hongjun.ni@intel.com; j.bromhead@titan-ic.com; deri@ntop.org;
> fc@napatech.com; arthur.su@lionic.com; Thomas Monjalon
> <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] regexdev: implement regex rte level
> functions
> 
> Hi,
> 
> > +int
> > +rte_regexdev_configure(uint8_t dev_id, const struct rte_regexdev_config
> > +*cfg) {
> > +	struct rte_regexdev *dev;
> > +	struct rte_regexdev_info dev_info;
> > +	int ret;
> > +
> > +	RTE_REGEXDEV_VALID_DEV_ID_OR_ERR_RET(dev_id, -EINVAL);
> > +	if (cfg == NULL)
> > +		return -EINVAL;
> > +	dev = &rte_regex_devices[dev_id];
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -
> > ENOTSUP);
> > +	if (dev->data->dev_started) {
> > +		RTE_REGEXDEV_LOG
> > +			(ERR, "Dev %u must be stopped to allow
> > configuration\n",
> > +			 dev_id);
> > +		return -EBUSY;
> > +	}
> > +	ret = regexdev_info_get(dev_id, &dev_info);
> > +	if (ret < 0)
> > +		return ret;
> > +	if ((cfg->dev_cfg_flags &
> > RTE_REGEXDEV_CFG_CROSS_BUFFER_SCAN_F) &&
> > +	    !(dev_info.regexdev_capa &
> > RTE_REGEXDEV_SUPP_CROSS_BUFFER_F)) {
> > +		RTE_REGEXDEV_LOG(ERR,
> > +				 "Dev %u doesn't support cross buffer
> > scan\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if ((cfg->dev_cfg_flags & RTE_REGEXDEV_CFG_MATCH_AS_END_F)
> > &&
> > +	    !(dev_info.regexdev_capa &
> > RTE_REGEXDEV_SUPP_MATCH_AS_END_F)) {
> > +		RTE_REGEXDEV_LOG(ERR,
> > +				 "Dev %u doesn't support match as end\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if ((cfg->dev_cfg_flags & RTE_REGEXDEV_CFG_MATCH_ALL_F) &&
> > +	    !(dev_info.regexdev_capa &
> > RTE_REGEXDEV_SUPP_MATCH_ALL_F)) {
> > +		RTE_REGEXDEV_LOG(ERR,
> > +				 "Dev %u doesn't support match all\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if (cfg->nb_groups == 0) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of groups must be >
> > 0\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if (cfg->nb_groups >= dev_info.max_groups) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of groups %d >
> > %d\n",
> > +				 dev_id, cfg->nb_groups,
> > dev_info.max_groups);
> > +		return -EINVAL;
> > +	}
> 
> The comparison should be > and not >=
> 

Yes, you are correct will fix.

> > +	if (cfg->nb_max_matches == 0) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of matches must be
> > > 0\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if (cfg->nb_max_matches >= dev_info.max_matches) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of matches %d >
> > %d\n",
> > +				 dev_id, cfg->nb_max_matches,
> > +				 dev_info.max_matches);
> > +		return -EINVAL;
> > +	}
> 
> The comparison should be > and not >=
> 

Yes, will fix.

> > +	if (cfg->nb_queue_pairs == 0) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of queues must be
> > > 0\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if (cfg->nb_queue_pairs >= dev_info.max_queue_pairs) {
> > +		RTE_REGEXDEV_LOG(ERR, "Dev %u num of queues %d >
> > %d\n",
> > +				 dev_id, cfg->nb_queue_pairs,
> > +				 dev_info.max_queue_pairs);
> > +		return -EINVAL;
> > +	}
> 
> The comparison should be > and not >=
> 

Will fix.

> > +	if (cfg->nb_rules_per_group == 0) {
> > +		RTE_REGEXDEV_LOG(ERR,
> > +				 "Dev %u num of rules per group must be >
> > 0\n",
> > +				 dev_id);
> > +		return -EINVAL;
> > +	}
> > +	if (cfg->nb_rules_per_group >= dev_info.max_rules_per_group) {
> > +		RTE_REGEXDEV_LOG(ERR,
> > +				 "Dev %u num of rules per group %d > %d\n",
> > +				 dev_id, cfg->nb_rules_per_group,
> > +				 dev_info.max_rules_per_group);
> > +		return -EINVAL;
> > +	}
> 
> The comparison should be > and not >=
> 

Will fix.

> > +	ret = (*dev->dev_ops->dev_configure)(dev, cfg);
> > +	if (ret == 0)
> > +		dev->data->dev_conf = *cfg;
> > +	return ret;
> > +}
> 
> In general I think that the validation of the cfg values should be done by the
> PMD

This was done in the first version.
after comments from the community, I changed it.
As much as I like the idea that PMD should handle everything by itself.
there is no point of code duplication, all PMD will require to do those test,
and there is no advantage of doing it inside the PMD.
Also it is common practice in DPDK to assume that the input was tested in the above 
layer. (you can see ethdev)