[dpdk-dev] [PATCH v7 1/4] librte_flow_classify: add librte_flow_classify library

[Iremonger, Bernard]

Hi Jasvinder,

> -----Original Message-----
> From: Singh, Jasvinder
> Sent: Friday, October 6, 2017 4:01 PM
> To: Iremonger, Bernard <bernard.iremonger at intel.com>; dev at dpdk.org;
> Yigit, Ferruh <ferruh.yigit at intel.com>; Ananyev, Konstantin
> <konstantin.ananyev at intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu at intel.com>; adrien.mazarguil at 6wind.com
> Cc: Iremonger, Bernard <bernard.iremonger at intel.com>
> Subject: RE: [dpdk-dev] [PATCH v7 1/4] librte_flow_classify: add
> librte_flow_classify library
> 
> Hi Bernard,
> 
> <snip>
> 
> > +struct rte_flow_classify *
> > +rte_flow_classify_create(void *table_handle,
> > +		uint32_t entry_size,
> > +		const struct rte_flow_attr *attr,
> > +		const struct rte_flow_item pattern[],
> > +		const struct rte_flow_action actions[],
> > +		struct rte_flow_error *error)
> > +{
> > +	struct rte_flow_classify *flow_classify;
> > +	int ret;
> > +
> > +	if (!error)
> > +		return NULL;
> > +
> > +	if (!table_handle) {
> > +		rte_flow_error_set(error, EINVAL,
> > RTE_FLOW_ERROR_TYPE_HANDLE,
> > +				   NULL, "NULL table_handle.");
> > +		return NULL;
> > +	}
> > +
> > +	if (!pattern) {
> > +		rte_flow_error_set(error, EINVAL,
> > RTE_FLOW_ERROR_TYPE_ITEM_NUM,
> > +				   NULL, "NULL pattern.");
> > +		return NULL;
> > +	}
> > +
> > +	if (!actions) {
> > +		rte_flow_error_set(error, EINVAL,
> > +				   RTE_FLOW_ERROR_TYPE_ACTION_NUM,
> > +				   NULL, "NULL action.");
> > +		return NULL;
> > +	}
> > +
> > +	if (!attr) {
> > +		rte_flow_error_set(error, EINVAL,
> > +				   RTE_FLOW_ERROR_TYPE_ATTR,
> > +				   NULL, "NULL attribute.");
> > +		return NULL;
> > +	}
> > +
> > +	/* parse attr, pattern and actions */
> > +	ret = rte_flow_classify_validate(table_handle, attr, pattern,
> > +			actions, error);
> > +	if (ret < 0)
> > +		return NULL;
> > +
> > +	flow_classify = allocate_5tuple();
> > +	if (!flow_classify)
> > +		return NULL;
> > +
> > +	flow_classify->entry = malloc(entry_size);
> > +	if (!flow_classify->entry) {
> > +		free(flow_classify);
> > +		flow_classify = NULL;
> > +		return NULL;
> > +	}
> > +	memset(flow_classify->entry, 0, entry_size);
> > +	memmove(flow_classify->entry, &flow_classify->id,
> > sizeof(uint32_t));
> > +
> > +	ret = rte_table_acl_ops.f_add(table_handle, &flow_classify-
> > >key_add,
> > +			flow_classify->entry, &flow_classify->key_found,
> > +			&flow_classify->entry_ptr);
> > +	if (ret) {
> > +		free(flow_classify->entry);
> > +		free(flow_classify);
> > +		flow_classify = NULL;
> > +		return NULL;
> > +	}
> > +
> > +	return flow_classify;
> > +}
> 
> The API in its current form creates the classifier object which will always use
> librte_acl based classification mechanism. This behavior imposes restriction
> on the application to always pass only ACL table related parameters for flow
> classification. In my opinion, API implementation should be agnostic to
> specific classification method and should be generic enough to allow
> application to select any of the available flow classification method (for e.g.
> acl, hash, LPM, etc.). Otherwise, this library will become another abstraction
> of librte_acl for flow classification.
> 
> Also, library allows table entries to be added while creating the classifier
> object, not later. Is there any specific reason?

Thanks for reviewing this patchset.

I will rework the code so that the API's are table agnostic.
In the v7 patchset the application creates the table ACL and then classify rules can be added and deleted at will.

I will send a v8 patchset.

Regards,

Bernard.