Re: [dpdk-dev] [PATCH v5 5/5] app/testpmd: add flex item CLI commands

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/12/2021 1:54 PM, Viacheslav Ovsiienko wrote:
> From: Gregory Etelson <getelson@nvidia.com>
> 
> Network port hardware is shipped with fixed number of
> supported network protocols. If application must work with a
> protocol that is not included in the port hardware by default, it
> can try to add the new protocol to port hardware.
> 
> Flex item or flex parser is port infrastructure that allows
> application to add support for a custom network header and
> offload flows to match the header elements.
> 
> Application must complete the following tasks to create a flow
> rule that matches custom header:
> 
> 1. Create flow item object in port hardware.
> Application must provide custom header configuration to PMD.
> PMD will use that configuration to create flex item object in
> port hardware.
> 
> 2. Create flex patterns to match. Flex pattern has a spec and a mask
> components, like a regular flow item. Combined together, spec and mask
> can target unique data sequence or a number of data sequences in the
> custom header.
> Flex patterns of the same flex item can have different lengths.
> Flex pattern is identified by unique handler value.
> 
> 3. Create a flow rule with a flex flow item that references
> flow pattern.
> 
> Testpmd flex CLI commands are:
> 
> testpmd> flow flex_item create <port> <flex_id> <filename>
> 

The file here is .json file, right? What do you think to provide some
sample .json file? I am not quite sure though where can be a place for
them, perhaps a sub-folder under testpmd?

> testpmd> set flex_pattern <pattern_id> \
>           spec <spec data> mask <mask data>
> 
> testpmd> set flex_pattern <pattern_id> is <spec_data>
> 
> testpmd> flow create <port> ... \
> / flex item is <flex_id> pattern is <pattern_id> / ...
> 
> The patch works with the jansson library API.
> Jansson development files must be present:
> jansson.pc, jansson.h libjansson.[a,so]
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> Reviewed-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

<...>

> +static void
> +flex_item_create(portid_t port_id, uint16_t flex_id, const char *filename)
> +{
> +	struct rte_flow_error flow_error;
> +	json_error_t json_error;
> +	json_t *jroot = NULL;
> +	struct flex_item *fp = flex_parser_fetch(port_id, flex_id);
> +	int ret;
> +
> +	if (fp == FLEX_PARSER_ERR) {
> +		printf("Bad parameters: port_id=%u flex_id=%u\n",
> +		       port_id, flex_id);
> +		return;
> +	}
> +	if (fp) {
> +		printf("port-%u: flex item #%u is already in use\n",
> +		       port_id, flex_id);
> +		return;
> +	}
> +	jroot = json_load_file(filename, 0, &json_error)> +	if (!jroot) {
> +		printf("Bad JSON file \"%s\": %s\n", filename, json_error.text);
> +		return;
> +	}
> +	fp = flex_item_init();
> +	if (!fp) {
> +		printf("Could not allocate flex item\n");
> +		goto out;
> +	}
> +	ret = flex_item_config(jroot, &fp->flex_conf);

What do you think to decouple json & flex item support a little more?


Like:
flex_item_config(&fp->flex_conf);
	flex_item_config_json(&fp->flex_conf);
		jroot = json_load_file()
		parse json & fill flex_conf
		json_decref(jroot);


> +	if (ret)
> +		goto out;
> +	fp->flex_handle = rte_flow_flex_item_create(port_id,
> +						    &fp->flex_conf,
> +						    &flow_error);
> +	if (fp->flex_handle) {
> +		flex_items[port_id][flex_id] = fp;
> +		printf("port-%u: created flex item #%u\n", port_id, flex_id);
> +		fp = NULL;
> +	} else {
> +		printf("port-%u: flex item #%u creation failed: %s\n",
> +		       port_id, flex_id,
> +		       flow_error.message ? flow_error.message : "");
> +	}
> +out:
> +	if (fp)
> +		free(fp);
> +	if (jroot)
> +		json_decref(jroot);
> +}
> +
> +#else /* RTE_HAS_JANSSON */
> +static void flex_item_create(__rte_unused portid_t port_id,
> +			     __rte_unused uint16_t flex_id,
> +			     __rte_unused const char *filename)
> +{
> +	printf("no JSON library\n");
> +}
> +
> +static void flex_item_destroy(__rte_unused portid_t port_id,
> +			     __rte_unused uint16_t flex_id)
> +{
> +	printf("no JSON library\n");
> +}
> +#endif /* RTE_HAS_JANSSON */

Does it make sense to move all above code (ifdef block) to a separate file?

Just because 'cmdline_flow.c' is getting bigger, I want to get your comment,
no strong opinion.