Re: [PATCH v2 1/2] app/testpmd: add commands to test new Rx offload API

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 03/13/2018 09:42 AM, Wei Dai wrote:
> Add following testpmd run-time commands to support test of
> new Rx offload API:
> rx_offload get capability <port_id>
> rx_offload get configuration <port_id>
> rx_offload enable|disable per_port <offload> <port_id>
> rx_offload enable|disable per_queue <offload> <port_id> <queue_id>
>
> Above last 2 commands should be run when the port is stopped.
> And <offload> can be one of "vlan_strip", "ipv4_cksum", ...
>
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>   app/test-pmd/cmdline.c | 424 +++++++++++++++++++++++++++++++++++++++++++++++++
>   app/test-pmd/testpmd.c |  15 +-
>   app/test-pmd/testpmd.h |   1 +
>   3 files changed, 438 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index d1dc1de..dfd0ca6 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -15996,6 +15996,426 @@ cmdline_parse_inst_t cmd_ptype_mapping_update = {
>   	},
>   };
>   
> +/* Get Rx offloads capability */
> +struct cmd_rx_offload_get_capa_result {
> +	cmdline_fixed_string_t rx_offload;
> +	cmdline_fixed_string_t get;
> +	cmdline_fixed_string_t capability;
> +	portid_t port_id;
> +};
> +
> +cmdline_parse_token_string_t cmd_rx_offload_get_capa_rx_offload =
> +	TOKEN_STRING_INITIALIZER
> +		(struct cmd_rx_offload_get_capa_result,
> +		 rx_offload, "rx_offload");
> +cmdline_parse_token_string_t cmd_rx_offload_get_capa_get =
> +	TOKEN_STRING_INITIALIZER
> +		(struct cmd_rx_offload_get_capa_result,
> +		 get, "get");
> +cmdline_parse_token_string_t cmd_rx_offload_get_capa_capability =
> +	TOKEN_STRING_INITIALIZER
> +		(struct cmd_rx_offload_get_capa_result,
> +		 capability, "capability");
> +cmdline_parse_token_num_t cmd_rx_offload_get_capa_port_id =
> +	TOKEN_NUM_INITIALIZER
> +		(struct cmd_rx_offload_get_capa_result,
> +		 port_id, UINT16);
> +
> +static void
> +print_rx_offloads(uint64_t offloads)
> +{
> +	uint64_t single_offload;
> +
> +	for (single_offload = DEV_RX_OFFLOAD_VLAN_STRIP;
> +	     single_offload <= DEV_RX_OFFLOAD_SECURITY;

It requires attention when a new offload is added.
Please, consider to use something like __builtin_ffsll(), print name of
the found bit, clear it and retry. It allows to avoid boundaries 
specification.

<snip>


> +static int
> +config_rx_offload(const char *name, uint64_t *offload, int on)
> +{
> +	uint64_t local = *offload;
> +
> +	if (!strcmp(name, "vlan_strip")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_VLAN_STRIP;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_VLAN_STRIP;
> +	} else if (!strcmp(name, "ipv4_cksum")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_IPV4_CKSUM;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_IPV4_CKSUM;
> +	} else if (!strcmp(name, "udp_cksum")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_UDP_CKSUM;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_UDP_CKSUM;
> +	} else if (!strcmp(name, "tcp_cksum")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_TCP_CKSUM;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_TCP_CKSUM;
> +	} else if (!strcmp(name, "tcp_lro")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_TCP_LRO;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_TCP_LRO;
> +	} else if (!strcmp(name, "qinq_strip")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_QINQ_STRIP;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_QINQ_STRIP;
> +	} else if (!strcmp(name, "outer_ipv4_cksum")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM;
> +	} else if (!strcmp(name, "macsec_strip")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_MACSEC_STRIP;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_MACSEC_STRIP;
> +	} else if (!strcmp(name, "header_split")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_HEADER_SPLIT;
> +	} else if (!strcmp(name, "vlan_filter")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_VLAN_FILTER;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_VLAN_FILTER;
> +	} else if (!strcmp(name, "vlan_extend")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_VLAN_EXTEND;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_VLAN_EXTEND;
> +	} else if (!strcmp(name, "jumbo_frame")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> +	} else if (!strcmp(name, "crc_strip")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_CRC_STRIP;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_CRC_STRIP;
> +	} else if (!strcmp(name, "scatter")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_SCATTER;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_SCATTER;
> +	} else if (!strcmp(name, "timestamp")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_TIMESTAMP;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_TIMESTAMP;
> +	} else if (!strcmp(name, "security")) {
> +		if (on)
> +			local |= DEV_RX_OFFLOAD_SECURITY;
> +		else
> +			local &= ~DEV_RX_OFFLOAD_SECURITY;
> +	} else
> +		return -1;

I was going to suggest rte_eth_dev_rx_offload_name(), strcasecmp() and loop.
It is possible to iterate on all U64 bits or add a way to get all 
offload bits from rte_ethdev.
However it still requires list of offloads in the help below. So, it is 
not easy to get rig of
offloads list completely. Just an idea. Up to you.

<snip>

> +cmdline_parse_inst_t cmd_config_per_port_rx_offload = {
> +	.f = cmd_config_per_port_rx_offload_parsed,
> +	.data = NULL,
> +	.help_str = "rx_offload enable|disable per_port vlan_strip|ipv4_cksum|"
> +		    "udp_cksum|tcp_cksum|tcp_lro|qinq_strip|outer_ipv4_cksum|"
> +		    "macsec_strip|header_split|vlan_filter|vlan_extend|"
> +		    "jumbo_frame|crc_strip|scatter|timestamp|security "
> +		    "<port_id>",
>

<snip>