[v16,8/8] app/testpmd: add command to set supported ptype mask
diff mbox series

Message ID 20191106191803.15098-9-pbhagavatula@marvell.com
State Superseded
Delegated to: Ferruh Yigit
Headers show
Series
  • ethdev: add new Rx offload flags
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Pavan Nikhilesh Bhagavatula Nov. 6, 2019, 7:18 p.m. UTC
From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Add command to set supported ptype mask.
Usage:
	set port <port_id> ptype_mask <ptype_mask>

Disable ptype parsing by default.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 app/test-pmd/cmdline.c                      | 83 +++++++++++++++++++++
 app/test-pmd/testpmd.c                      |  5 ++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 ++
 3 files changed, 95 insertions(+)

Comments

Iremonger, Bernard Nov. 7, 2019, 11:57 a.m. UTC | #1
Hi Pavan,

> -----Original Message-----
> From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> Sent: Wednesday, November 6, 2019 7:18 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com;
> jerinj@marvell.com; thomas@monjalon.net; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger,
> Bernard <bernard.iremonger@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Kovacevic, Marko
> <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavatula@marvell.com>
> Subject: [dpdk-dev] [PATCH v16 8/8] app/testpmd: add command to set
> supported ptype mask
> 
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> Add command to set supported ptype mask.
> Usage:
> 	set port <port_id> ptype_mask <ptype_mask>
> 
> Disable ptype parsing by default.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>  app/test-pmd/cmdline.c                      | 83 +++++++++++++++++++++
>  app/test-pmd/testpmd.c                      |  5 ++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 ++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> 49c45a3f0..7af2c58c3 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c

The help text should be updated to describe the new command at line 240 in cmdline.c

> @@ -668,6 +668,9 @@ static void cmd_help_long_parsed(void
> *parsed_result,
>  			"ptype mapping update (port_id) (hw_ptype)
> (sw_ptype)\n"
>  			"    Update a ptype mapping item on a port\n\n"
> 
> +			"set port (port_id) ptype_mask (ptype_mask)\n"
> +			"    set packet types classification for a specific
> port\n\n"
> +
>  			"set port (port_id) queue-region region_id (value) "
>  			"queue_start_index (value) queue_num (value)\n"
>  			"    Set a queue region on a port\n\n"
> @@ -18916,6 +18919,85 @@ cmdline_parse_inst_t
> cmd_show_port_supported_ptypes = {
>  	},
>  };
> 
> +/* Common result structure for set port ptypes */ struct
> +cmd_set_port_ptypes_result {
> +	cmdline_fixed_string_t set;
> +	cmdline_fixed_string_t port;
> +	portid_t port_id;
> +	cmdline_fixed_string_t ptype_mask;
> +	uint32_t mask;
> +};
> +
> +/* Common CLI fields for set port ptypes */
> +cmdline_parse_token_string_t cmd_set_port_ptypes_set =
> +	TOKEN_STRING_INITIALIZER
> +		(struct cmd_set_port_ptypes_result,
> +		 set, "set");
> +cmdline_parse_token_string_t cmd_set_port_ptypes_port =
> +	TOKEN_STRING_INITIALIZER
> +		(struct cmd_set_port_ptypes_result,
> +		 port, "port");
> +cmdline_parse_token_num_t cmd_set_port_ptypes_port_id =
> +	TOKEN_NUM_INITIALIZER
> +		(struct cmd_set_port_ptypes_result,
> +		 port_id, UINT16);
> +cmdline_parse_token_string_t cmd_set_port_ptypes_mask_str =
> +	TOKEN_STRING_INITIALIZER
> +		(struct cmd_set_port_ptypes_result,
> +		 ptype_mask, "ptype_mask");
> +cmdline_parse_token_num_t cmd_set_port_ptypes_mask_u32 =
> +	TOKEN_NUM_INITIALIZER
> +		(struct cmd_set_port_ptypes_result,
> +		 mask, UINT32);
> +
> +static void
> +cmd_set_port_ptypes_parsed(
> +	void *parsed_result,
> +	__attribute__((unused)) struct cmdline *cl,
> +	__attribute__((unused)) void *data)
> +{
> +	struct cmd_set_port_ptypes_result *res = parsed_result;
> +#define PTYPE_NAMESIZE        256
> +	char ptype_name[PTYPE_NAMESIZE];
> +	uint16_t port_id = res->port_id;
> +	uint32_t ptype_mask = res->mask;
> +	int ret, i;
> +
> +	ret = rte_eth_dev_get_supported_ptypes(port_id, ptype_mask,
> NULL, 0);
> +	if (ret <= 0) {
> +		printf("Port %d doesn't support any ptypes.\n", port_id);
> +		return;
> +	}
> +
> +	uint32_t ptypes[ret];
> +
> +	ret = rte_eth_dev_set_ptypes(port_id, ptype_mask, ptypes, ret);
> +	if (ret <= 0) {
> +		printf("Unable to set requested ptypes for Port %d\n",
> port_id);
> +		return;
> +	}
> +
> +	printf("Successfully set following ptypes for Port %d\n", port_id);
> +	for (i = 0; i < ret && ptypes[i] != RTE_PTYPE_UNKNOWN; i++) {
> +		rte_get_ptype_name(ptypes[i], ptype_name,
> sizeof(ptype_name));
> +		printf("%s\n", ptype_name);
> +	}
> +}
> +
> +cmdline_parse_inst_t cmd_set_port_ptypes = {
> +	.f = cmd_set_port_ptypes_parsed,
> +	.data = NULL,
> +	.help_str = "set port <port_id> ptype_mask <mask>",
> +	.tokens = {
> +		(void *)&cmd_set_port_ptypes_set,
> +		(void *)&cmd_set_port_ptypes_port,
> +		(void *)&cmd_set_port_ptypes_port_id,
> +		(void *)&cmd_set_port_ptypes_mask_str,
> +		(void *)&cmd_set_port_ptypes_mask_u32,
> +		NULL,
> +	},
> +};
> +
>  /*
> **********************************************************
> ********************** */
> 
>  /* list of instructions */
> @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_show_vf_stats,
>  	(cmdline_parse_inst_t *)&cmd_clear_vf_stats,
>  	(cmdline_parse_inst_t *)&cmd_show_port_supported_ptypes,
> +	(cmdline_parse_inst_t *)&cmd_set_port_ptypes,
>  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
>  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
>  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_reset, diff --git
> a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 5ba974162..812aebf35 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
>  	queueid_t qi;
>  	struct rte_port *port;
>  	struct rte_ether_addr mac_addr;
> +	static uint8_t clr_ptypes = 1;
> 
>  	if (port_id_is_invalid(pid, ENABLED_WARN))
>  		return 0;
> @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
>  			}
>  		}
>  		configure_rxtx_dump_callbacks(verbose_level);
> +		if (clr_ptypes) {
> +			clr_ptypes = 0;
> +			rte_eth_dev_set_ptypes(pi,
> RTE_PTYPE_UNKNOWN, NULL, 0);
> +		}
>  		/* start port */
>  		if (rte_eth_dev_start(pi) < 0) {
>  			printf("Fail to start port %d\n", pi); diff --git
> a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index c68a742eb..f78ac9444 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -472,6 +472,13 @@ Show ptypes supported for a specific port::
> 
>     testpmd> show port (port_id) ptypes
> 
> +set port supported ptypes
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +set packet types classification for a specific port::
> +
> +   testpmd> set port (port_id) ptypes_mask (mask)
> +
>  show device info
>  ~~~~~~~~~~~~~~~~
> 
> --
> 2.17.1
Regards,

Bernard.
Iremonger, Bernard Nov. 7, 2019, 12:27 p.m. UTC | #2
Hi Pavin,

<snip>

> > Subject: [dpdk-dev] [PATCH v16 8/8] app/testpmd: add command to set
> > supported ptype mask
> >
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >
> > Add command to set supported ptype mask.
> > Usage:
> > 	set port <port_id> ptype_mask <ptype_mask>
> >
> > Disable ptype parsing by default.
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > ---
> >  app/test-pmd/cmdline.c                      | 83 +++++++++++++++++++++
> >  app/test-pmd/testpmd.c                      |  5 ++
> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 ++
> >  3 files changed, 95 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 49c45a3f0..7af2c58c3 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> 
> The help text should be updated to describe the new command at line 240 in
> cmdline.c
> 
> > @@ -668,6 +668,9 @@ static void cmd_help_long_parsed(void
> > *parsed_result,
> >  			"ptype mapping update (port_id) (hw_ptype)
> (sw_ptype)\n"
> >  			"    Update a ptype mapping item on a port\n\n"
> >
> > +			"set port (port_id) ptype_mask (ptype_mask)\n"
> > +			"    set packet types classification for a specific
> > port\n\n"
> > +
> >  			"set port (port_id) queue-region region_id (value) "
> >  			"queue_start_index (value) queue_num (value)\n"
> >  			"    Set a queue region on a port\n\n"
> > @@ -18916,6 +18919,85 @@ cmdline_parse_inst_t
> > cmd_show_port_supported_ptypes = {
> >  	},
> >  };
> >
> > +/* Common result structure for set port ptypes */ struct
> > +cmd_set_port_ptypes_result {
> > +	cmdline_fixed_string_t set;
> > +	cmdline_fixed_string_t port;
> > +	portid_t port_id;
> > +	cmdline_fixed_string_t ptype_mask;
> > +	uint32_t mask;
> > +};
> > +
> > +/* Common CLI fields for set port ptypes */
> > +cmdline_parse_token_string_t cmd_set_port_ptypes_set =
> > +	TOKEN_STRING_INITIALIZER
> > +		(struct cmd_set_port_ptypes_result,
> > +		 set, "set");
> > +cmdline_parse_token_string_t cmd_set_port_ptypes_port =
> > +	TOKEN_STRING_INITIALIZER
> > +		(struct cmd_set_port_ptypes_result,
> > +		 port, "port");
> > +cmdline_parse_token_num_t cmd_set_port_ptypes_port_id =
> > +	TOKEN_NUM_INITIALIZER
> > +		(struct cmd_set_port_ptypes_result,
> > +		 port_id, UINT16);
> > +cmdline_parse_token_string_t cmd_set_port_ptypes_mask_str =
> > +	TOKEN_STRING_INITIALIZER
> > +		(struct cmd_set_port_ptypes_result,
> > +		 ptype_mask, "ptype_mask");
> > +cmdline_parse_token_num_t cmd_set_port_ptypes_mask_u32 =
> > +	TOKEN_NUM_INITIALIZER
> > +		(struct cmd_set_port_ptypes_result,
> > +		 mask, UINT32);
> > +
> > +static void
> > +cmd_set_port_ptypes_parsed(
> > +	void *parsed_result,
> > +	__attribute__((unused)) struct cmdline *cl,
> > +	__attribute__((unused)) void *data)
> > +{
> > +	struct cmd_set_port_ptypes_result *res = parsed_result;
> > +#define PTYPE_NAMESIZE        256
> > +	char ptype_name[PTYPE_NAMESIZE];
> > +	uint16_t port_id = res->port_id;
> > +	uint32_t ptype_mask = res->mask;
> > +	int ret, i;
> > +
> > +	ret = rte_eth_dev_get_supported_ptypes(port_id, ptype_mask,
> > NULL, 0);

The last 2 parameters to the above function do not look correct, here is the function declaration:
int rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,  uint32_t *ptypes, int num);

ptypes should be a pointer to an array to hold the ptypes, and num should be the size of the array.

> > +	if (ret <= 0) {
> > +		printf("Port %d doesn't support any ptypes.\n", port_id);
> > +		return;
> > +	}
> > +
> > +	uint32_t ptypes[ret];

The above declaration can then be moved to the top of the function with the other declarations.

> > +
> > +	ret = rte_eth_dev_set_ptypes(port_id, ptype_mask, ptypes, ret);
> > +	if (ret <= 0) {
> > +		printf("Unable to set requested ptypes for Port %d\n",
> > port_id);
> > +		return;
> > +	}
> > +
> > +	printf("Successfully set following ptypes for Port %d\n", port_id);
> > +	for (i = 0; i < ret && ptypes[i] != RTE_PTYPE_UNKNOWN; i++) {
> > +		rte_get_ptype_name(ptypes[i], ptype_name,
> > sizeof(ptype_name));
> > +		printf("%s\n", ptype_name);
> > +	}
> > +}
> > +
> > +cmdline_parse_inst_t cmd_set_port_ptypes = {
> > +	.f = cmd_set_port_ptypes_parsed,
> > +	.data = NULL,
> > +	.help_str = "set port <port_id> ptype_mask <mask>",
> > +	.tokens = {
> > +		(void *)&cmd_set_port_ptypes_set,
> > +		(void *)&cmd_set_port_ptypes_port,
> > +		(void *)&cmd_set_port_ptypes_port_id,
> > +		(void *)&cmd_set_port_ptypes_mask_str,
> > +		(void *)&cmd_set_port_ptypes_mask_u32,
> > +		NULL,
> > +	},
> > +};
> > +
> >  /*
> >
> **********************************************************
> > ********************** */
> >
> >  /* list of instructions */
> > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] = {
> >  	(cmdline_parse_inst_t *)&cmd_show_vf_stats,
> >  	(cmdline_parse_inst_t *)&cmd_clear_vf_stats,
> >  	(cmdline_parse_inst_t *)&cmd_show_port_supported_ptypes,
> > +	(cmdline_parse_inst_t *)&cmd_set_port_ptypes,
> >  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
> >  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
> >  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_reset, diff --git
> > a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 5ba974162..812aebf35 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
> >  	queueid_t qi;
> >  	struct rte_port *port;
> >  	struct rte_ether_addr mac_addr;
> > +	static uint8_t clr_ptypes = 1;
> >
> >  	if (port_id_is_invalid(pid, ENABLED_WARN))
> >  		return 0;
> > @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
> >  			}
> >  		}
> >  		configure_rxtx_dump_callbacks(verbose_level);
> > +		if (clr_ptypes) {
> > +			clr_ptypes = 0;
> > +			rte_eth_dev_set_ptypes(pi,
> > RTE_PTYPE_UNKNOWN, NULL, 0);
> > +		}
> >  		/* start port */
> >  		if (rte_eth_dev_start(pi) < 0) {
> >  			printf("Fail to start port %d\n", pi); diff --git
> > a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > index c68a742eb..f78ac9444 100644
> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > @@ -472,6 +472,13 @@ Show ptypes supported for a specific port::
> >
> >     testpmd> show port (port_id) ptypes
> >
> > +set port supported ptypes
> > +~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +set packet types classification for a specific port::
> > +
> > +   testpmd> set port (port_id) ptypes_mask (mask)
> > +
> >  show device info
> >  ~~~~~~~~~~~~~~~~
> >
> > --
> > 2.17.1
 Regards,
 
 Bernard.
Pavan Nikhilesh Bhagavatula Nov. 7, 2019, 2:36 p.m. UTC | #3
Hi Bernard,

>Hi Pavin,
>
><snip>
>
>> > Subject: [dpdk-dev] [PATCH v16 8/8] app/testpmd: add command
>to set
>> > supported ptype mask
>> >
>> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> >
>> > Add command to set supported ptype mask.
>> > Usage:
>> > 	set port <port_id> ptype_mask <ptype_mask>
>> >
>> > Disable ptype parsing by default.
>> >
>> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> > ---
>> >  app/test-pmd/cmdline.c                      | 83 +++++++++++++++++++++
>> >  app/test-pmd/testpmd.c                      |  5 ++
>> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 ++
>> >  3 files changed, 95 insertions(+)
>> >
>> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>index
>> > 49c45a3f0..7af2c58c3 100644
>> > --- a/app/test-pmd/cmdline.c
>> > +++ b/app/test-pmd/cmdline.c
>>
>> The help text should be updated to describe the new command at
>line 240 in
>> cmdline.c
>>
>> > @@ -668,6 +668,9 @@ static void cmd_help_long_parsed(void
>> > *parsed_result,
>> >  			"ptype mapping update (port_id) (hw_ptype)
>> (sw_ptype)\n"
>> >  			"    Update a ptype mapping item on a port\n\n"
>> >
>> > +			"set port (port_id) ptype_mask
>(ptype_mask)\n"
>> > +			"    set packet types classification for a specific
>> > port\n\n"
>> > +
>> >  			"set port (port_id) queue-region region_id
>(value) "
>> >  			"queue_start_index (value) queue_num
>(value)\n"
>> >  			"    Set a queue region on a port\n\n"
>> > @@ -18916,6 +18919,85 @@ cmdline_parse_inst_t
>> > cmd_show_port_supported_ptypes = {
>> >  	},
>> >  };
>> >
>> > +/* Common result structure for set port ptypes */ struct
>> > +cmd_set_port_ptypes_result {
>> > +	cmdline_fixed_string_t set;
>> > +	cmdline_fixed_string_t port;
>> > +	portid_t port_id;
>> > +	cmdline_fixed_string_t ptype_mask;
>> > +	uint32_t mask;
>> > +};
>> > +
>> > +/* Common CLI fields for set port ptypes */
>> > +cmdline_parse_token_string_t cmd_set_port_ptypes_set =
>> > +	TOKEN_STRING_INITIALIZER
>> > +		(struct cmd_set_port_ptypes_result,
>> > +		 set, "set");
>> > +cmdline_parse_token_string_t cmd_set_port_ptypes_port =
>> > +	TOKEN_STRING_INITIALIZER
>> > +		(struct cmd_set_port_ptypes_result,
>> > +		 port, "port");
>> > +cmdline_parse_token_num_t cmd_set_port_ptypes_port_id =
>> > +	TOKEN_NUM_INITIALIZER
>> > +		(struct cmd_set_port_ptypes_result,
>> > +		 port_id, UINT16);
>> > +cmdline_parse_token_string_t cmd_set_port_ptypes_mask_str =
>> > +	TOKEN_STRING_INITIALIZER
>> > +		(struct cmd_set_port_ptypes_result,
>> > +		 ptype_mask, "ptype_mask");
>> > +cmdline_parse_token_num_t cmd_set_port_ptypes_mask_u32 =
>> > +	TOKEN_NUM_INITIALIZER
>> > +		(struct cmd_set_port_ptypes_result,
>> > +		 mask, UINT32);
>> > +
>> > +static void
>> > +cmd_set_port_ptypes_parsed(
>> > +	void *parsed_result,
>> > +	__attribute__((unused)) struct cmdline *cl,
>> > +	__attribute__((unused)) void *data)
>> > +{
>> > +	struct cmd_set_port_ptypes_result *res = parsed_result;
>> > +#define PTYPE_NAMESIZE        256
>> > +	char ptype_name[PTYPE_NAMESIZE];
>> > +	uint16_t port_id = res->port_id;
>> > +	uint32_t ptype_mask = res->mask;
>> > +	int ret, i;
>> > +
>> > +	ret = rte_eth_dev_get_supported_ptypes(port_id,
>ptype_mask,
>> > NULL, 0);
>
>The last 2 parameters to the above function do not look correct, here is
>the function declaration:
>int rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t
>ptype_mask,  uint32_t *ptypes, int num);
>
>ptypes should be a pointer to an array to hold the ptypes, and num
>should be the size of the array.

We can use the same API to get the number of ptypes supported to initialize the 
array below.

@see examples/l3fwd/l3fwd_lpm.c +424

>
>> > +	if (ret <= 0) {
>> > +		printf("Port %d doesn't support any ptypes.\n",
>port_id);
>> > +		return;
>> > +	}
>> > +
>> > +	uint32_t ptypes[ret];
>
>The above declaration can then be moved to the top of the function
>with the other declarations.


I intentionally placed it here as the array size depends on ret and there is 
no readily available macro for max number of packet types.


Thanks,
Pavan.

>
>> > +
>> > +	ret = rte_eth_dev_set_ptypes(port_id, ptype_mask, ptypes,
>ret);
>> > +	if (ret <= 0) {
>> > +		printf("Unable to set requested ptypes for Port %d\n",
>> > port_id);
>> > +		return;
>> > +	}
>> > +
>> > +	printf("Successfully set following ptypes for Port %d\n",
>port_id);
>> > +	for (i = 0; i < ret && ptypes[i] != RTE_PTYPE_UNKNOWN; i++) {
>> > +		rte_get_ptype_name(ptypes[i], ptype_name,
>> > sizeof(ptype_name));
>> > +		printf("%s\n", ptype_name);
>> > +	}
>> > +}
>> > +
>> > +cmdline_parse_inst_t cmd_set_port_ptypes = {
>> > +	.f = cmd_set_port_ptypes_parsed,
>> > +	.data = NULL,
>> > +	.help_str = "set port <port_id> ptype_mask <mask>",
>> > +	.tokens = {
>> > +		(void *)&cmd_set_port_ptypes_set,
>> > +		(void *)&cmd_set_port_ptypes_port,
>> > +		(void *)&cmd_set_port_ptypes_port_id,
>> > +		(void *)&cmd_set_port_ptypes_mask_str,
>> > +		(void *)&cmd_set_port_ptypes_mask_u32,
>> > +		NULL,
>> > +	},
>> > +};
>> > +
>> >  /*
>> >
>>
>******************************************************
>****
>> > ********************** */
>> >
>> >  /* list of instructions */
>> > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] = {
>> >  	(cmdline_parse_inst_t *)&cmd_show_vf_stats,
>> >  	(cmdline_parse_inst_t *)&cmd_clear_vf_stats,
>> >  	(cmdline_parse_inst_t
>*)&cmd_show_port_supported_ptypes,
>> > +	(cmdline_parse_inst_t *)&cmd_set_port_ptypes,
>> >  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
>> >  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
>> >  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_reset, diff --
>git
>> > a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> > 5ba974162..812aebf35 100644
>> > --- a/app/test-pmd/testpmd.c
>> > +++ b/app/test-pmd/testpmd.c
>> > @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
>> >  	queueid_t qi;
>> >  	struct rte_port *port;
>> >  	struct rte_ether_addr mac_addr;
>> > +	static uint8_t clr_ptypes = 1;
>> >
>> >  	if (port_id_is_invalid(pid, ENABLED_WARN))
>> >  		return 0;
>> > @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
>> >  			}
>> >  		}
>> >  		configure_rxtx_dump_callbacks(verbose_level);
>> > +		if (clr_ptypes) {
>> > +			clr_ptypes = 0;
>> > +			rte_eth_dev_set_ptypes(pi,
>> > RTE_PTYPE_UNKNOWN, NULL, 0);
>> > +		}
>> >  		/* start port */
>> >  		if (rte_eth_dev_start(pi) < 0) {
>> >  			printf("Fail to start port %d\n", pi); diff --git
>> > a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> > index c68a742eb..f78ac9444 100644
>> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> > @@ -472,6 +472,13 @@ Show ptypes supported for a specific port::
>> >
>> >     testpmd> show port (port_id) ptypes
>> >
>> > +set port supported ptypes
>> > +~~~~~~~~~~~~~~~~~~~~~~~~~
>> > +
>> > +set packet types classification for a specific port::
>> > +
>> > +   testpmd> set port (port_id) ptypes_mask (mask)
>> > +
>> >  show device info
>> >  ~~~~~~~~~~~~~~~~
>> >
>> > --
>> > 2.17.1
> Regards,
>
> Bernard.
Iremonger, Bernard Nov. 7, 2019, 3 p.m. UTC | #4
Hi Pavin,

<snip>

> >> > Subject: [dpdk-dev] [PATCH v16 8/8] app/testpmd: add command
> >to set
> >> > supported ptype mask
> >> >
> >> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >> >
> >> > Add command to set supported ptype mask.
> >> > Usage:
> >> > 	set port <port_id> ptype_mask <ptype_mask>
> >> >
> >> > Disable ptype parsing by default.
> >> >
> >> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >> > ---
> >> >  app/test-pmd/cmdline.c                      | 83 +++++++++++++++++++++
> >> >  app/test-pmd/testpmd.c                      |  5 ++
> >> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 ++
> >> >  3 files changed, 95 insertions(+)
> >> >
> >> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> >index
> >> > 49c45a3f0..7af2c58c3 100644
> >> > --- a/app/test-pmd/cmdline.c
> >> > +++ b/app/test-pmd/cmdline.c
> >>
> >> The help text should be updated to describe the new command at
> >line 240 in
> >> cmdline.c
> >>
> >> > @@ -668,6 +668,9 @@ static void cmd_help_long_parsed(void
> >> > *parsed_result,
> >> >  			"ptype mapping update (port_id) (hw_ptype)
> >> (sw_ptype)\n"
> >> >  			"    Update a ptype mapping item on a port\n\n"
> >> >
> >> > +			"set port (port_id) ptype_mask
> >(ptype_mask)\n"
> >> > +			"    set packet types classification for a specific
> >> > port\n\n"
> >> > +
> >> >  			"set port (port_id) queue-region region_id
> >(value) "
> >> >  			"queue_start_index (value) queue_num
> >(value)\n"
> >> >  			"    Set a queue region on a port\n\n"
> >> > @@ -18916,6 +18919,85 @@ cmdline_parse_inst_t
> >> > cmd_show_port_supported_ptypes = {
> >> >  	},
> >> >  };
> >> >
> >> > +/* Common result structure for set port ptypes */ struct
> >> > +cmd_set_port_ptypes_result {
> >> > +	cmdline_fixed_string_t set;
> >> > +	cmdline_fixed_string_t port;
> >> > +	portid_t port_id;
> >> > +	cmdline_fixed_string_t ptype_mask;
> >> > +	uint32_t mask;
> >> > +};
> >> > +
> >> > +/* Common CLI fields for set port ptypes */
> >> > +cmdline_parse_token_string_t cmd_set_port_ptypes_set =
> >> > +	TOKEN_STRING_INITIALIZER
> >> > +		(struct cmd_set_port_ptypes_result,
> >> > +		 set, "set");
> >> > +cmdline_parse_token_string_t cmd_set_port_ptypes_port =
> >> > +	TOKEN_STRING_INITIALIZER
> >> > +		(struct cmd_set_port_ptypes_result,
> >> > +		 port, "port");
> >> > +cmdline_parse_token_num_t cmd_set_port_ptypes_port_id =
> >> > +	TOKEN_NUM_INITIALIZER
> >> > +		(struct cmd_set_port_ptypes_result,
> >> > +		 port_id, UINT16);
> >> > +cmdline_parse_token_string_t cmd_set_port_ptypes_mask_str =
> >> > +	TOKEN_STRING_INITIALIZER
> >> > +		(struct cmd_set_port_ptypes_result,
> >> > +		 ptype_mask, "ptype_mask");
> >> > +cmdline_parse_token_num_t cmd_set_port_ptypes_mask_u32 =
> >> > +	TOKEN_NUM_INITIALIZER
> >> > +		(struct cmd_set_port_ptypes_result,
> >> > +		 mask, UINT32);
> >> > +
> >> > +static void
> >> > +cmd_set_port_ptypes_parsed(
> >> > +	void *parsed_result,
> >> > +	__attribute__((unused)) struct cmdline *cl,
> >> > +	__attribute__((unused)) void *data) {
> >> > +	struct cmd_set_port_ptypes_result *res = parsed_result;
> >> > +#define PTYPE_NAMESIZE        256
> >> > +	char ptype_name[PTYPE_NAMESIZE];
> >> > +	uint16_t port_id = res->port_id;
> >> > +	uint32_t ptype_mask = res->mask;
> >> > +	int ret, i;
> >> > +
> >> > +	ret = rte_eth_dev_get_supported_ptypes(port_id,
> >ptype_mask,
> >> > NULL, 0);
> >
> >The last 2 parameters to the above function do not look correct, here
> >is the function declaration:
> >int rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t
> >ptype_mask,  uint32_t *ptypes, int num);
> >
> >ptypes should be a pointer to an array to hold the ptypes, and num
> >should be the size of the array.
> 
> We can use the same API to get the number of ptypes supported to initialize
> the array below.
> 
> @see examples/l3fwd/l3fwd_lpm.c +424

Yes, you are correct, sorry for the noise.

> >> > +	if (ret <= 0) {
> >> > +		printf("Port %d doesn't support any ptypes.\n",
> >port_id);
> >> > +		return;
> >> > +	}
> >> > +
> >> > +	uint32_t ptypes[ret];
> >
> >The above declaration can then be moved to the top of the function with
> >the other declarations.
> 
> 
> I intentionally placed it here as the array size depends on ret and there is no
> readily available macro for max number of packet types.

Yes, this is correct too

<snip>

Regards,

Bernard.
Ferruh Yigit Nov. 7, 2019, 6:40 p.m. UTC | #5
On 11/6/2019 7:18 PM, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> Add command to set supported ptype mask.
> Usage:
> 	set port <port_id> ptype_mask <ptype_mask>
> 
> Disable ptype parsing by default.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>  app/test-pmd/cmdline.c                      | 83 +++++++++++++++++++++
>  app/test-pmd/testpmd.c                      |  5 ++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 ++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 49c45a3f0..7af2c58c3 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -668,6 +668,9 @@ static void cmd_help_long_parsed(void *parsed_result,
>  			"ptype mapping update (port_id) (hw_ptype) (sw_ptype)\n"
>  			"    Update a ptype mapping item on a port\n\n"
>  
> +			"set port (port_id) ptype_mask (ptype_mask)\n"
> +			"    set packet types classification for a specific port\n\n"
> +
>  			"set port (port_id) queue-region region_id (value) "
>  			"queue_start_index (value) queue_num (value)\n"
>  			"    Set a queue region on a port\n\n"
> @@ -18916,6 +18919,85 @@ cmdline_parse_inst_t cmd_show_port_supported_ptypes = {
>  	},
>  };
>  
> +/* Common result structure for set port ptypes */
> +struct cmd_set_port_ptypes_result {
> +	cmdline_fixed_string_t set;
> +	cmdline_fixed_string_t port;
> +	portid_t port_id;
> +	cmdline_fixed_string_t ptype_mask;
> +	uint32_t mask;
> +};
> +
> +/* Common CLI fields for set port ptypes */
> +cmdline_parse_token_string_t cmd_set_port_ptypes_set =
> +	TOKEN_STRING_INITIALIZER
> +		(struct cmd_set_port_ptypes_result,
> +		 set, "set");
> +cmdline_parse_token_string_t cmd_set_port_ptypes_port =
> +	TOKEN_STRING_INITIALIZER
> +		(struct cmd_set_port_ptypes_result,
> +		 port, "port");
> +cmdline_parse_token_num_t cmd_set_port_ptypes_port_id =
> +	TOKEN_NUM_INITIALIZER
> +		(struct cmd_set_port_ptypes_result,
> +		 port_id, UINT16);
> +cmdline_parse_token_string_t cmd_set_port_ptypes_mask_str =
> +	TOKEN_STRING_INITIALIZER
> +		(struct cmd_set_port_ptypes_result,
> +		 ptype_mask, "ptype_mask");
> +cmdline_parse_token_num_t cmd_set_port_ptypes_mask_u32 =
> +	TOKEN_NUM_INITIALIZER
> +		(struct cmd_set_port_ptypes_result,
> +		 mask, UINT32);
> +
> +static void
> +cmd_set_port_ptypes_parsed(
> +	void *parsed_result,
> +	__attribute__((unused)) struct cmdline *cl,
> +	__attribute__((unused)) void *data)
> +{
> +	struct cmd_set_port_ptypes_result *res = parsed_result;
> +#define PTYPE_NAMESIZE        256
> +	char ptype_name[PTYPE_NAMESIZE];
> +	uint16_t port_id = res->port_id;
> +	uint32_t ptype_mask = res->mask;
> +	int ret, i;
> +
> +	ret = rte_eth_dev_get_supported_ptypes(port_id, ptype_mask, NULL, 0);
> +	if (ret <= 0) {
> +		printf("Port %d doesn't support any ptypes.\n", port_id);
> +		return;

"ptype_mask" is the list of ptypes that we are interested right, having it 0
means we are not interested with any. But using "ptype_mask" as mask in
'rte_eth_dev_get_supported_ptypes()' will cause this API return 0 and it will
fails this command.
Why not use 'RTE_PTYPE_ALL_MASK' as mask value?

> +	}
> +
> +	uint32_t ptypes[ret];
> +
> +	ret = rte_eth_dev_set_ptypes(port_id, ptype_mask, ptypes, ret);
> +	if (ret <= 0) {
> +		printf("Unable to set requested ptypes for Port %d\n", port_id);
> +		return;
> +	}
> +
> +	printf("Successfully set following ptypes for Port %d\n", port_id);
> +	for (i = 0; i < ret && ptypes[i] != RTE_PTYPE_UNKNOWN; i++) {
> +		rte_get_ptype_name(ptypes[i], ptype_name, sizeof(ptype_name));
> +		printf("%s\n", ptype_name);
> +	}
> +}
> +
> +cmdline_parse_inst_t cmd_set_port_ptypes = {
> +	.f = cmd_set_port_ptypes_parsed,
> +	.data = NULL,
> +	.help_str = "set port <port_id> ptype_mask <mask>",
> +	.tokens = {
> +		(void *)&cmd_set_port_ptypes_set,
> +		(void *)&cmd_set_port_ptypes_port,
> +		(void *)&cmd_set_port_ptypes_port_id,
> +		(void *)&cmd_set_port_ptypes_mask_str,
> +		(void *)&cmd_set_port_ptypes_mask_u32,
> +		NULL,
> +	},
> +};
> +
>  /* ******************************************************************************** */
>  
>  /* list of instructions */
> @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_show_vf_stats,
>  	(cmdline_parse_inst_t *)&cmd_clear_vf_stats,
>  	(cmdline_parse_inst_t *)&cmd_show_port_supported_ptypes,
> +	(cmdline_parse_inst_t *)&cmd_set_port_ptypes,
>  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
>  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
>  	(cmdline_parse_inst_t *)&cmd_ptype_mapping_reset,
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 5ba974162..812aebf35 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
>  	queueid_t qi;
>  	struct rte_port *port;
>  	struct rte_ether_addr mac_addr;
> +	static uint8_t clr_ptypes = 1;
>  
>  	if (port_id_is_invalid(pid, ENABLED_WARN))
>  		return 0;
> @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
>  			}
>  		}
>  		configure_rxtx_dump_callbacks(verbose_level);
> +		if (clr_ptypes) {
> +			clr_ptypes = 0;
> +			rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN, NULL, 0);
> +		}

I am not sure about this command, we have now capability to set/disable ptypes
on demand, why disabling them by default?
Jerin Jacob Nov. 7, 2019, 6:55 p.m. UTC | #6
On Fri, 8 Nov, 2019, 12:10 am Ferruh Yigit, <ferruh.yigit@intel.com> wrote:

> On 11/6/2019 7:18 PM, pbhagavatula@marvell.com wrote:
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >
> > Add command to set supported ptype mask.
> > Usage:
> >       set port <port_id> ptype_mask >  /* ***************
> >
> >  /* list of instructions */
> > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] = {
> >       (cmdline_parse_inst_t *)&cmd_show_vf_stats,
> >       (cmdline_parse_inst_t *)&cmd_clear_vf_stats,
> >       (cmdline_parse_inst_t *)&cmd_show_port_supported_ptypes,
> > +     (cmdline_parse_inst_t *)&cmd_set_port_ptypes,
> >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
> >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
> >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_reset,
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > index 5ba974162..812aebf35 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
> >       queueid_t qi;
> >       struct rte_port *port;
> >       struct rte_ether_addr mac_addr;
> > +     static uint8_t clr_ptypes = 1;
> >
> >       if (port_id_is_invalid(pid, ENABLED_WARN))
> >               return 0;
> > @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
> >                       }
> >               }
> >               configure_rxtx_dump_callbacks(verbose_level);
> > +             if (clr_ptypes) {
> > +                     clr_ptypes = 0;
> > +                     rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN,
> NULL, 0);
> > +             }
>
> I am not sure about this command, we have now capability to set/disable
> ptypes
> on demand, why disabling them by default?
>

As forward engines are not using the ptype offload. If a specific forward
mode need the offload, it can be enabled.



>
Ferruh Yigit Nov. 7, 2019, 7:40 p.m. UTC | #7
On 11/7/2019 6:55 PM, Jerin Jacob wrote:
> 
> 
> On Fri, 8 Nov, 2019, 12:10 am Ferruh Yigit, <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 11/6/2019 7:18 PM, pbhagavatula@marvell.com
>     <mailto:pbhagavatula@marvell.com> wrote:
>     > From: Pavan Nikhilesh <pbhagavatula@marvell.com
>     <mailto:pbhagavatula@marvell.com>>
>     >
>     > Add command to set supported ptype mask.
>     > Usage:
>     >       set port <port_id> ptype_mask >  /* ***************
>     > 
>     >  /* list of instructions */
>     > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] = {
>     >       (cmdline_parse_inst_t *)&cmd_show_vf_stats,
>     >       (cmdline_parse_inst_t *)&cmd_clear_vf_stats,
>     >       (cmdline_parse_inst_t *)&cmd_show_port_supported_ptypes,
>     > +     (cmdline_parse_inst_t *)&cmd_set_port_ptypes,
>     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
>     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
>     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_reset,
>     > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>     > index 5ba974162..812aebf35 100644
>     > --- a/app/test-pmd/testpmd.c
>     > +++ b/app/test-pmd/testpmd.c
>     > @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
>     >       queueid_t qi;
>     >       struct rte_port *port;
>     >       struct rte_ether_addr mac_addr;
>     > +     static uint8_t clr_ptypes = 1;
>     > 
>     >       if (port_id_is_invalid(pid, ENABLED_WARN))
>     >               return 0;
>     > @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
>     >                       }
>     >               }
>     >               configure_rxtx_dump_callbacks(verbose_level);
>     > +             if (clr_ptypes) {
>     > +                     clr_ptypes = 0;
>     > +                     rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN, NULL, 0);
>     > +             }
> 
>     I am not sure about this command, we have now capability to set/disable ptypes
>     on demand, why disabling them by default?
> 
> 
> As forward engines are not using the ptype offload. If a specific forward mode
> need the offload, it can be enabled.
> 

OK, I am still not sure but I understand your reasoning.

But this is a behavior change and it may caught people, what about following
more operational updates:
- Separate this into its own patch, this is different than adding new command
- move "clr_ptypes" next to other static global variables, to make this
selection more obvious, and those global variables tend to have some comments,
we can add comment to this one as well.
- print a log when "rte_eth_dev_set_ptypes()" returns success, to say packets
types parsing is disabled, to help user to understand what is happening. Because
currently there is no other way to get the current configured ptypes from PMD.
Jerin Jacob Nov. 8, 2019, 4:13 a.m. UTC | #8
On Fri, Nov 8, 2019 at 1:11 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 11/7/2019 6:55 PM, Jerin Jacob wrote:
> >
> >
> > On Fri, 8 Nov, 2019, 12:10 am Ferruh Yigit, <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>> wrote:
> >
> >     On 11/6/2019 7:18 PM, pbhagavatula@marvell.com
> >     <mailto:pbhagavatula@marvell.com> wrote:
> >     > From: Pavan Nikhilesh <pbhagavatula@marvell.com
> >     <mailto:pbhagavatula@marvell.com>>
> >     >
> >     > Add command to set supported ptype mask.
> >     > Usage:
> >     >       set port <port_id> ptype_mask >  /* ***************
> >     >
> >     >  /* list of instructions */
> >     > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] = {
> >     >       (cmdline_parse_inst_t *)&cmd_show_vf_stats,
> >     >       (cmdline_parse_inst_t *)&cmd_clear_vf_stats,
> >     >       (cmdline_parse_inst_t *)&cmd_show_port_supported_ptypes,
> >     > +     (cmdline_parse_inst_t *)&cmd_set_port_ptypes,
> >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
> >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
> >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_reset,
> >     > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> >     > index 5ba974162..812aebf35 100644
> >     > --- a/app/test-pmd/testpmd.c
> >     > +++ b/app/test-pmd/testpmd.c
> >     > @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
> >     >       queueid_t qi;
> >     >       struct rte_port *port;
> >     >       struct rte_ether_addr mac_addr;
> >     > +     static uint8_t clr_ptypes = 1;
> >     >
> >     >       if (port_id_is_invalid(pid, ENABLED_WARN))
> >     >               return 0;
> >     > @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
> >     >                       }
> >     >               }
> >     >               configure_rxtx_dump_callbacks(verbose_level);
> >     > +             if (clr_ptypes) {
> >     > +                     clr_ptypes = 0;
> >     > +                     rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN, NULL, 0);
> >     > +             }
> >
> >     I am not sure about this command, we have now capability to set/disable ptypes
> >     on demand, why disabling them by default?
> >
> >
> > As forward engines are not using the ptype offload. If a specific forward mode
> > need the offload, it can be enabled.
> >
>
> OK, I am still not sure but I understand your reasoning.
>
> But this is a behavior change and it may caught people, what about following
> more operational updates:
> - Separate this into its own patch, this is different than adding new command
> - move "clr_ptypes" next to other static global variables, to make this
> selection more obvious, and those global variables tend to have some comments,
> we can add comment to this one as well.
> - print a log when "rte_eth_dev_set_ptypes()" returns success, to say packets
> types parsing is disabled, to help user to understand what is happening. Because
> currently there is no other way to get the current configured ptypes from PMD.

+1 for operational updates.
Ananyev, Konstantin Nov. 8, 2019, 1:54 p.m. UTC | #9
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
> Sent: Thursday, November 7, 2019 7:41 PM
> To: Jerin Jacob <jerinjacobk@gmail.com>
> Cc: Pavan Nikhilesh <pbhagavatula@marvell.com>; Andrew Rybchenko <arybchenko@solarflare.com>; jerinj@marvell.com;
> thomas@monjalon.net; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko <marko.kovacevic@intel.com>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v16 8/8] app/testpmd: add command to set supported ptype mask
> 
> On 11/7/2019 6:55 PM, Jerin Jacob wrote:
> >
> >
> > On Fri, 8 Nov, 2019, 12:10 am Ferruh Yigit, <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>> wrote:
> >
> >     On 11/6/2019 7:18 PM, pbhagavatula@marvell.com
> >     <mailto:pbhagavatula@marvell.com> wrote:
> >     > From: Pavan Nikhilesh <pbhagavatula@marvell.com
> >     <mailto:pbhagavatula@marvell.com>>
> >     >
> >     > Add command to set supported ptype mask.
> >     > Usage:
> >     >       set port <port_id> ptype_mask >  /* ***************
> >     >
> >     >  /* list of instructions */
> >     > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] = {
> >     >       (cmdline_parse_inst_t *)&cmd_show_vf_stats,
> >     >       (cmdline_parse_inst_t *)&cmd_clear_vf_stats,
> >     >       (cmdline_parse_inst_t *)&cmd_show_port_supported_ptypes,
> >     > +     (cmdline_parse_inst_t *)&cmd_set_port_ptypes,
> >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
> >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
> >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_reset,
> >     > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> >     > index 5ba974162..812aebf35 100644
> >     > --- a/app/test-pmd/testpmd.c
> >     > +++ b/app/test-pmd/testpmd.c
> >     > @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
> >     >       queueid_t qi;
> >     >       struct rte_port *port;
> >     >       struct rte_ether_addr mac_addr;
> >     > +     static uint8_t clr_ptypes = 1;
> >     >
> >     >       if (port_id_is_invalid(pid, ENABLED_WARN))
> >     >               return 0;
> >     > @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
> >     >                       }
> >     >               }
> >     >               configure_rxtx_dump_callbacks(verbose_level);
> >     > +             if (clr_ptypes) {
> >     > +                     clr_ptypes = 0;
> >     > +                     rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN, NULL, 0);
> >     > +             }
> >
> >     I am not sure about this command, we have now capability to set/disable ptypes
> >     on demand, why disabling them by default?
> >
> >
> > As forward engines are not using the ptype offload. If a specific forward mode
> > need the offload, it can be enabled.

Well, at least now user can see ptype in pkt dump with 'set verbose > 0'
It's definitely looks loke a a behavior change. 
Wonder why it can't be done visa-versa?
Keep current behavior as default one (all ptypes are un-masked) and
have special command to disable/enable ptypes.
as I understand such command was added by these series. correct? 
Konstantin 	

> >
> 
> OK, I am still not sure but I understand your reasoning.
> 
> But this is a behavior change and it may caught people, what about following
> more operational updates:
> - Separate this into its own patch, this is different than adding new command
> - move "clr_ptypes" next to other static global variables, to make this
> selection more obvious, and those global variables tend to have some comments,
> we can add comment to this one as well.
> - print a log when "rte_eth_dev_set_ptypes()" returns success, to say packets
> types parsing is disabled, to help user to understand what is happening. Because
> currently there is no other way to get the current configured ptypes from PMD.
Jerin Jacob Nov. 8, 2019, 3:01 p.m. UTC | #10
On Fri, Nov 8, 2019 at 7:24 PM Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
> > Sent: Thursday, November 7, 2019 7:41 PM
> > To: Jerin Jacob <jerinjacobk@gmail.com>
> > Cc: Pavan Nikhilesh <pbhagavatula@marvell.com>; Andrew Rybchenko <arybchenko@solarflare.com>; jerinj@marvell.com;
> > thomas@monjalon.net; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard
> > <bernard.iremonger@intel.com>; Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko <marko.kovacevic@intel.com>;
> > dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v16 8/8] app/testpmd: add command to set supported ptype mask
> >
> > On 11/7/2019 6:55 PM, Jerin Jacob wrote:
> > >
> > >
> > > On Fri, 8 Nov, 2019, 12:10 am Ferruh Yigit, <ferruh.yigit@intel.com
> > > <mailto:ferruh.yigit@intel.com>> wrote:
> > >
> > >     On 11/6/2019 7:18 PM, pbhagavatula@marvell.com
> > >     <mailto:pbhagavatula@marvell.com> wrote:
> > >     > From: Pavan Nikhilesh <pbhagavatula@marvell.com
> > >     <mailto:pbhagavatula@marvell.com>>
> > >     >
> > >     > Add command to set supported ptype mask.
> > >     > Usage:
> > >     >       set port <port_id> ptype_mask >  /* ***************
> > >     >
> > >     >  /* list of instructions */
> > >     > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] = {
> > >     >       (cmdline_parse_inst_t *)&cmd_show_vf_stats,
> > >     >       (cmdline_parse_inst_t *)&cmd_clear_vf_stats,
> > >     >       (cmdline_parse_inst_t *)&cmd_show_port_supported_ptypes,
> > >     > +     (cmdline_parse_inst_t *)&cmd_set_port_ptypes,
> > >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
> > >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
> > >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_reset,
> > >     > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > >     > index 5ba974162..812aebf35 100644
> > >     > --- a/app/test-pmd/testpmd.c
> > >     > +++ b/app/test-pmd/testpmd.c
> > >     > @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
> > >     >       queueid_t qi;
> > >     >       struct rte_port *port;
> > >     >       struct rte_ether_addr mac_addr;
> > >     > +     static uint8_t clr_ptypes = 1;
> > >     >
> > >     >       if (port_id_is_invalid(pid, ENABLED_WARN))
> > >     >               return 0;
> > >     > @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
> > >     >                       }
> > >     >               }
> > >     >               configure_rxtx_dump_callbacks(verbose_level);
> > >     > +             if (clr_ptypes) {
> > >     > +                     clr_ptypes = 0;
> > >     > +                     rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN, NULL, 0);
> > >     > +             }
> > >
> > >     I am not sure about this command, we have now capability to set/disable ptypes
> > >     on demand, why disabling them by default?
> > >
> > >
> > > As forward engines are not using the ptype offload. If a specific forward mode
> > > need the offload, it can be enabled.
>
> Well, at least now user can see ptype in pkt dump with 'set verbose > 0'
> It's definitely looks loke a a behavior change.
> Wonder why it can't be done visa-versa?
> Keep current behavior as default one (all ptypes are un-masked) and
> have special command to disable/enable ptypes.
> as I understand such command was added by these series. correct?

IMO, we are following the principle that by default all offloads are
disabled and enable it
based on the need to have optimal performance across the DPDK. This
change will be
on the same theme.

Regarding the verbose > 0 cases, I think, we can call
rte_eth_dev_set_ptypes() to all PTYPES on
the setting verbose callback.
Pavan Nikhilesh Bhagavatula Nov. 11, 2019, 4:56 a.m. UTC | #11
>On Fri, Nov 8, 2019 at 7:24 PM Ananyev, Konstantin
><konstantin.ananyev@intel.com> wrote:
>>
>>
>>
>> > -----Original Message-----
>> > From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
>> > Sent: Thursday, November 7, 2019 7:41 PM
>> > To: Jerin Jacob <jerinjacobk@gmail.com>
>> > Cc: Pavan Nikhilesh <pbhagavatula@marvell.com>; Andrew
>Rybchenko <arybchenko@solarflare.com>; jerinj@marvell.com;
>> > thomas@monjalon.net; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard
>> > <bernard.iremonger@intel.com>; Mcnamara, John
><john.mcnamara@intel.com>; Kovacevic, Marko
><marko.kovacevic@intel.com>;
>> > dev@dpdk.org
>> > Subject: Re: [dpdk-dev] [PATCH v16 8/8] app/testpmd: add
>command to set supported ptype mask
>> >
>> > On 11/7/2019 6:55 PM, Jerin Jacob wrote:
>> > >
>> > >
>> > > On Fri, 8 Nov, 2019, 12:10 am Ferruh Yigit, <ferruh.yigit@intel.com
>> > > <mailto:ferruh.yigit@intel.com>> wrote:
>> > >
>> > >     On 11/6/2019 7:18 PM, pbhagavatula@marvell.com
>> > >     <mailto:pbhagavatula@marvell.com> wrote:
>> > >     > From: Pavan Nikhilesh <pbhagavatula@marvell.com
>> > >     <mailto:pbhagavatula@marvell.com>>
>> > >     >
>> > >     > Add command to set supported ptype mask.
>> > >     > Usage:
>> > >     >       set port <port_id> ptype_mask >  /* ***************
>> > >     >
>> > >     >  /* list of instructions */
>> > >     > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] =
>{
>> > >     >       (cmdline_parse_inst_t *)&cmd_show_vf_stats,
>> > >     >       (cmdline_parse_inst_t *)&cmd_clear_vf_stats,
>> > >     >       (cmdline_parse_inst_t
>*)&cmd_show_port_supported_ptypes,
>> > >     > +     (cmdline_parse_inst_t *)&cmd_set_port_ptypes,
>> > >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
>> > >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
>> > >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_reset,
>> > >     > diff --git a/app/test-pmd/testpmd.c b/app/test-
>pmd/testpmd.c
>> > >     > index 5ba974162..812aebf35 100644
>> > >     > --- a/app/test-pmd/testpmd.c
>> > >     > +++ b/app/test-pmd/testpmd.c
>> > >     > @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
>> > >     >       queueid_t qi;
>> > >     >       struct rte_port *port;
>> > >     >       struct rte_ether_addr mac_addr;
>> > >     > +     static uint8_t clr_ptypes = 1;
>> > >     >
>> > >     >       if (port_id_is_invalid(pid, ENABLED_WARN))
>> > >     >               return 0;
>> > >     > @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
>> > >     >                       }
>> > >     >               }
>> > >     >               configure_rxtx_dump_callbacks(verbose_level);
>> > >     > +             if (clr_ptypes) {
>> > >     > +                     clr_ptypes = 0;
>> > >     > +                     rte_eth_dev_set_ptypes(pi,
>RTE_PTYPE_UNKNOWN, NULL, 0);
>> > >     > +             }
>> > >
>> > >     I am not sure about this command, we have now capability to
>set/disable ptypes
>> > >     on demand, why disabling them by default?
>> > >
>> > >
>> > > As forward engines are not using the ptype offload. If a specific
>forwa
>> > > need the offload, it can be enabled.
>>
>> Well, at least now user can see ptype in pkt dump with 'set verbose >
>0'
>> It's definitely looks loke a a behavior change.
>> Wonder why it can't be done visa-versa?
>> Keep current behavior as default one (all ptypes are un-masked) and
>> have special command to disable/enable ptypes.
>> as I understand such command was added by these series. correct?
>
>IMO, we are following the principle that by default all offloads are
>disabled and enable it
>based on the need to have optimal performance across the DPDK. This
>change will be
>on the same theme.
>
>Regarding the verbose > 0 cases, I think, we can call
>rte_eth_dev_set_ptypes() to all PTYPES on
>the setting verbose callback.

Agree verbose > 0 case we can do set_ptypes with 
RTE_PTYPE_ALL_MASK. But what if the user wants to verify 
rte_eth_dev_set_ptypes() call itself in verbose mode?. 
Example:

	set port 0 ptype_mask RTE_PTYPE_L3_MASK
	set verbose 1

In this case user expects to see only L3 mask set. Wouldn’t it be confusing if
we set RTE_PTYPE_ALL_MASK under the hood when verbose level is increased?

Pavan.
Jerin Jacob Nov. 11, 2019, 5:02 a.m. UTC | #12
On Mon, Nov 11, 2019 at 10:26 AM Pavan Nikhilesh Bhagavatula
<pbhagavatula@marvell.com> wrote:
>
> >On Fri, Nov 8, 2019 at 7:24 PM Ananyev, Konstantin
> ><konstantin.ananyev@intel.com> wrote:
> >>
> >>
> >>
> >> > -----Original Message-----
> >> > From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
> >> > Sent: Thursday, November 7, 2019 7:41 PM
> >> > To: Jerin Jacob <jerinjacobk@gmail.com>
> >> > Cc: Pavan Nikhilesh <pbhagavatula@marvell.com>; Andrew
> >Rybchenko <arybchenko@solarflare.com>; jerinj@marvell.com;
> >> > thomas@monjalon.net; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> >Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard
> >> > <bernard.iremonger@intel.com>; Mcnamara, John
> ><john.mcnamara@intel.com>; Kovacevic, Marko
> ><marko.kovacevic@intel.com>;
> >> > dev@dpdk.org
> >> > Subject: Re: [dpdk-dev] [PATCH v16 8/8] app/testpmd: add
> >command to set supported ptype mask
> >> >
> >> > On 11/7/2019 6:55 PM, Jerin Jacob wrote:
> >> > >
> >> > >
> >> > > On Fri, 8 Nov, 2019, 12:10 am Ferruh Yigit, <ferruh.yigit@intel.com
> >> > > <mailto:ferruh.yigit@intel.com>> wrote:
> >> > >
> >> > >     On 11/6/2019 7:18 PM, pbhagavatula@marvell.com
> >> > >     <mailto:pbhagavatula@marvell.com> wrote:
> >> > >     > From: Pavan Nikhilesh <pbhagavatula@marvell.com
> >> > >     <mailto:pbhagavatula@marvell.com>>
> >> > >     >
> >> > >     > Add command to set supported ptype mask.
> >> > >     > Usage:
> >> > >     >       set port <port_id> ptype_mask >  /* ***************
> >> > >     >
> >> > >     >  /* list of instructions */
> >> > >     > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] =
> >{
> >> > >     >       (cmdline_parse_inst_t *)&cmd_show_vf_stats,
> >> > >     >       (cmdline_parse_inst_t *)&cmd_clear_vf_stats,
> >> > >     >       (cmdline_parse_inst_t
> >*)&cmd_show_port_supported_ptypes,
> >> > >     > +     (cmdline_parse_inst_t *)&cmd_set_port_ptypes,
> >> > >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
> >> > >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
> >> > >     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_reset,
> >> > >     > diff --git a/app/test-pmd/testpmd.c b/app/test-
> >pmd/testpmd.c
> >> > >     > index 5ba974162..812aebf35 100644
> >> > >     > --- a/app/test-pmd/testpmd.c
> >> > >     > +++ b/app/test-pmd/testpmd.c
> >> > >     > @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
> >> > >     >       queueid_t qi;
> >> > >     >       struct rte_port *port;
> >> > >     >       struct rte_ether_addr mac_addr;
> >> > >     > +     static uint8_t clr_ptypes = 1;
> >> > >     >
> >> > >     >       if (port_id_is_invalid(pid, ENABLED_WARN))
> >> > >     >               return 0;
> >> > >     > @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
> >> > >     >                       }
> >> > >     >               }
> >> > >     >               configure_rxtx_dump_callbacks(verbose_level);
> >> > >     > +             if (clr_ptypes) {
> >> > >     > +                     clr_ptypes = 0;
> >> > >     > +                     rte_eth_dev_set_ptypes(pi,
> >RTE_PTYPE_UNKNOWN, NULL, 0);
> >> > >     > +             }
> >> > >
> >> > >     I am not sure about this command, we have now capability to
> >set/disable ptypes
> >> > >     on demand, why disabling them by default?
> >> > >
> >> > >
> >> > > As forward engines are not using the ptype offload. If a specific
> >forwa
> >> > > need the offload, it can be enabled.
> >>
> >> Well, at least now user can see ptype in pkt dump with 'set verbose >
> >0'
> >> It's definitely looks loke a a behavior change.
> >> Wonder why it can't be done visa-versa?
> >> Keep current behavior as default one (all ptypes are un-masked) and
> >> have special command to disable/enable ptypes.
> >> as I understand such command was added by these series. correct?
> >
> >IMO, we are following the principle that by default all offloads are
> >disabled and enable it
> >based on the need to have optimal performance across the DPDK. This
> >change will be
> >on the same theme.
> >
> >Regarding the verbose > 0 cases, I think, we can call
> >rte_eth_dev_set_ptypes() to all PTYPES on
> >the setting verbose callback.
>
> Agree verbose > 0 case we can do set_ptypes with
> RTE_PTYPE_ALL_MASK. But what if the user wants to verify
> rte_eth_dev_set_ptypes() call itself in verbose mode?.
> Example:
>
>         set port 0 ptype_mask RTE_PTYPE_L3_MASK
>         set verbose 1
>
> In this case user expects to see only L3 mask set. Wouldn’t it be confusing if
> we set RTE_PTYPE_ALL_MASK under the hood when verbose level is increased?

I thought of adding RTE_PTYPE_ALL_MASK in the callback to maintain the
existing behavior
if there is any concern in that area.

Either way is fine with me. (implicit RTE_PTYPE_ALL_MASK  in a
callback or explicit mask set in command line)
Ferruh Yigit Nov. 11, 2019, 10:44 a.m. UTC | #13
On 11/11/2019 5:02 AM, Jerin Jacob wrote:
> On Mon, Nov 11, 2019 at 10:26 AM Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com> wrote:
>>
>>> On Fri, Nov 8, 2019 at 7:24 PM Ananyev, Konstantin
>>> <konstantin.ananyev@intel.com> wrote:
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
>>>>> Sent: Thursday, November 7, 2019 7:41 PM
>>>>> To: Jerin Jacob <jerinjacobk@gmail.com>
>>>>> Cc: Pavan Nikhilesh <pbhagavatula@marvell.com>; Andrew
>>> Rybchenko <arybchenko@solarflare.com>; jerinj@marvell.com;
>>>>> thomas@monjalon.net; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>>> Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard
>>>>> <bernard.iremonger@intel.com>; Mcnamara, John
>>> <john.mcnamara@intel.com>; Kovacevic, Marko
>>> <marko.kovacevic@intel.com>;
>>>>> dev@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH v16 8/8] app/testpmd: add
>>> command to set supported ptype mask
>>>>>
>>>>> On 11/7/2019 6:55 PM, Jerin Jacob wrote:
>>>>>>
>>>>>>
>>>>>> On Fri, 8 Nov, 2019, 12:10 am Ferruh Yigit, <ferruh.yigit@intel.com
>>>>>> <mailto:ferruh.yigit@intel.com>> wrote:
>>>>>>
>>>>>>     On 11/6/2019 7:18 PM, pbhagavatula@marvell.com
>>>>>>     <mailto:pbhagavatula@marvell.com> wrote:
>>>>>>     > From: Pavan Nikhilesh <pbhagavatula@marvell.com
>>>>>>     <mailto:pbhagavatula@marvell.com>>
>>>>>>     >
>>>>>>     > Add command to set supported ptype mask.
>>>>>>     > Usage:
>>>>>>     >       set port <port_id> ptype_mask >  /* ***************
>>>>>>     >
>>>>>>     >  /* list of instructions */
>>>>>>     > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] =
>>> {
>>>>>>     >       (cmdline_parse_inst_t *)&cmd_show_vf_stats,
>>>>>>     >       (cmdline_parse_inst_t *)&cmd_clear_vf_stats,
>>>>>>     >       (cmdline_parse_inst_t
>>> *)&cmd_show_port_supported_ptypes,
>>>>>>     > +     (cmdline_parse_inst_t *)&cmd_set_port_ptypes,
>>>>>>     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
>>>>>>     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
>>>>>>     >       (cmdline_parse_inst_t *)&cmd_ptype_mapping_reset,
>>>>>>     > diff --git a/app/test-pmd/testpmd.c b/app/test-
>>> pmd/testpmd.c
>>>>>>     > index 5ba974162..812aebf35 100644
>>>>>>     > --- a/app/test-pmd/testpmd.c
>>>>>>     > +++ b/app/test-pmd/testpmd.c
>>>>>>     > @@ -2024,6 +2024,7 @@ start_port(portid_t pid)
>>>>>>     >       queueid_t qi;
>>>>>>     >       struct rte_port *port;
>>>>>>     >       struct rte_ether_addr mac_addr;
>>>>>>     > +     static uint8_t clr_ptypes = 1;
>>>>>>     >
>>>>>>     >       if (port_id_is_invalid(pid, ENABLED_WARN))
>>>>>>     >               return 0;
>>>>>>     > @@ -2153,6 +2154,10 @@ start_port(portid_t pid)
>>>>>>     >                       }
>>>>>>     >               }
>>>>>>     >               configure_rxtx_dump_callbacks(verbose_level);
>>>>>>     > +             if (clr_ptypes) {
>>>>>>     > +                     clr_ptypes = 0;
>>>>>>     > +                     rte_eth_dev_set_ptypes(pi,
>>> RTE_PTYPE_UNKNOWN, NULL, 0);
>>>>>>     > +             }
>>>>>>
>>>>>>     I am not sure about this command, we have now capability to
>>> set/disable ptypes
>>>>>>     on demand, why disabling them by default?
>>>>>>
>>>>>>
>>>>>> As forward engines are not using the ptype offload. If a specific
>>> forwa
>>>>>> need the offload, it can be enabled.
>>>>
>>>> Well, at least now user can see ptype in pkt dump with 'set verbose >
>>> 0'
>>>> It's definitely looks loke a a behavior change.
>>>> Wonder why it can't be done visa-versa?
>>>> Keep current behavior as default one (all ptypes are un-masked) and
>>>> have special command to disable/enable ptypes.
>>>> as I understand such command was added by these series. correct?
>>>
>>> IMO, we are following the principle that by default all offloads are
>>> disabled and enable it
>>> based on the need to have optimal performance across the DPDK. This
>>> change will be
>>> on the same theme.
>>>
>>> Regarding the verbose > 0 cases, I think, we can call
>>> rte_eth_dev_set_ptypes() to all PTYPES on
>>> the setting verbose callback.
>>
>> Agree verbose > 0 case we can do set_ptypes with
>> RTE_PTYPE_ALL_MASK. But what if the user wants to verify
>> rte_eth_dev_set_ptypes() call itself in verbose mode?.
>> Example:
>>
>>         set port 0 ptype_mask RTE_PTYPE_L3_MASK
>>         set verbose 1
>>
>> In this case user expects to see only L3 mask set. Wouldn’t it be confusing if
>> we set RTE_PTYPE_ALL_MASK under the hood when verbose level is increased?
> 
> I thought of adding RTE_PTYPE_ALL_MASK in the callback to maintain the
> existing behavior
> if there is any concern in that area.
> 
> Either way is fine with me. (implicit RTE_PTYPE_ALL_MASK  in a
> callback or explicit mask set in command line)
> 

I think explicit set in command line is better, setting in the callback can be
confusing.

Patch
diff mbox series

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 49c45a3f0..7af2c58c3 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -668,6 +668,9 @@  static void cmd_help_long_parsed(void *parsed_result,
 			"ptype mapping update (port_id) (hw_ptype) (sw_ptype)\n"
 			"    Update a ptype mapping item on a port\n\n"
 
+			"set port (port_id) ptype_mask (ptype_mask)\n"
+			"    set packet types classification for a specific port\n\n"
+
 			"set port (port_id) queue-region region_id (value) "
 			"queue_start_index (value) queue_num (value)\n"
 			"    Set a queue region on a port\n\n"
@@ -18916,6 +18919,85 @@  cmdline_parse_inst_t cmd_show_port_supported_ptypes = {
 	},
 };
 
+/* Common result structure for set port ptypes */
+struct cmd_set_port_ptypes_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t port;
+	portid_t port_id;
+	cmdline_fixed_string_t ptype_mask;
+	uint32_t mask;
+};
+
+/* Common CLI fields for set port ptypes */
+cmdline_parse_token_string_t cmd_set_port_ptypes_set =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_set_port_ptypes_result,
+		 set, "set");
+cmdline_parse_token_string_t cmd_set_port_ptypes_port =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_set_port_ptypes_result,
+		 port, "port");
+cmdline_parse_token_num_t cmd_set_port_ptypes_port_id =
+	TOKEN_NUM_INITIALIZER
+		(struct cmd_set_port_ptypes_result,
+		 port_id, UINT16);
+cmdline_parse_token_string_t cmd_set_port_ptypes_mask_str =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_set_port_ptypes_result,
+		 ptype_mask, "ptype_mask");
+cmdline_parse_token_num_t cmd_set_port_ptypes_mask_u32 =
+	TOKEN_NUM_INITIALIZER
+		(struct cmd_set_port_ptypes_result,
+		 mask, UINT32);
+
+static void
+cmd_set_port_ptypes_parsed(
+	void *parsed_result,
+	__attribute__((unused)) struct cmdline *cl,
+	__attribute__((unused)) void *data)
+{
+	struct cmd_set_port_ptypes_result *res = parsed_result;
+#define PTYPE_NAMESIZE        256
+	char ptype_name[PTYPE_NAMESIZE];
+	uint16_t port_id = res->port_id;
+	uint32_t ptype_mask = res->mask;
+	int ret, i;
+
+	ret = rte_eth_dev_get_supported_ptypes(port_id, ptype_mask, NULL, 0);
+	if (ret <= 0) {
+		printf("Port %d doesn't support any ptypes.\n", port_id);
+		return;
+	}
+
+	uint32_t ptypes[ret];
+
+	ret = rte_eth_dev_set_ptypes(port_id, ptype_mask, ptypes, ret);
+	if (ret <= 0) {
+		printf("Unable to set requested ptypes for Port %d\n", port_id);
+		return;
+	}
+
+	printf("Successfully set following ptypes for Port %d\n", port_id);
+	for (i = 0; i < ret && ptypes[i] != RTE_PTYPE_UNKNOWN; i++) {
+		rte_get_ptype_name(ptypes[i], ptype_name, sizeof(ptype_name));
+		printf("%s\n", ptype_name);
+	}
+}
+
+cmdline_parse_inst_t cmd_set_port_ptypes = {
+	.f = cmd_set_port_ptypes_parsed,
+	.data = NULL,
+	.help_str = "set port <port_id> ptype_mask <mask>",
+	.tokens = {
+		(void *)&cmd_set_port_ptypes_set,
+		(void *)&cmd_set_port_ptypes_port,
+		(void *)&cmd_set_port_ptypes_port_id,
+		(void *)&cmd_set_port_ptypes_mask_str,
+		(void *)&cmd_set_port_ptypes_mask_u32,
+		NULL,
+	},
+};
+
 /* ******************************************************************************** */
 
 /* list of instructions */
@@ -19155,6 +19237,7 @@  cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_show_vf_stats,
 	(cmdline_parse_inst_t *)&cmd_clear_vf_stats,
 	(cmdline_parse_inst_t *)&cmd_show_port_supported_ptypes,
+	(cmdline_parse_inst_t *)&cmd_set_port_ptypes,
 	(cmdline_parse_inst_t *)&cmd_ptype_mapping_get,
 	(cmdline_parse_inst_t *)&cmd_ptype_mapping_replace,
 	(cmdline_parse_inst_t *)&cmd_ptype_mapping_reset,
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 5ba974162..812aebf35 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2024,6 +2024,7 @@  start_port(portid_t pid)
 	queueid_t qi;
 	struct rte_port *port;
 	struct rte_ether_addr mac_addr;
+	static uint8_t clr_ptypes = 1;
 
 	if (port_id_is_invalid(pid, ENABLED_WARN))
 		return 0;
@@ -2153,6 +2154,10 @@  start_port(portid_t pid)
 			}
 		}
 		configure_rxtx_dump_callbacks(verbose_level);
+		if (clr_ptypes) {
+			clr_ptypes = 0;
+			rte_eth_dev_set_ptypes(pi, RTE_PTYPE_UNKNOWN, NULL, 0);
+		}
 		/* start port */
 		if (rte_eth_dev_start(pi) < 0) {
 			printf("Fail to start port %d\n", pi);
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index c68a742eb..f78ac9444 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -472,6 +472,13 @@  Show ptypes supported for a specific port::
 
    testpmd> show port (port_id) ptypes
 
+set port supported ptypes
+~~~~~~~~~~~~~~~~~~~~~~~~~
+
+set packet types classification for a specific port::
+
+   testpmd> set port (port_id) ptypes_mask (mask)
+
 show device info
 ~~~~~~~~~~~~~~~~