Hi Hanumanth,

Thanks for expanding support for the NIC-to-PMD Rx metadata feature.
I do not object the idea of the patch, it looks aceeptable.
However, please find my comments below.

On Mon, 1 Aug 2022, Hanumanth Pothula wrote:

> Presently, rx metadata is sent to PMD by default, leading
> to a performance drop as processing for the same in rx path
> takes extra cycles.
>
> Hence, introducing command line argument, 'rx-metadata' to
> control passing rx metadata to PMD. By default it’s disabled.
>
> Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>
> ---
> app/test-pmd/parameters.c | 4 ++++
> app/test-pmd/testpmd.c | 6 +++++-
> app/test-pmd/testpmd.h | 2 ++
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index e3c9757f3f..daf1218977 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -213,6 +213,7 @@ usage(char* progname)
> printf(" --hairpin-mode=0xXX: bitmask set the hairpin port mode.\n"
> " 0x10 - explicit Tx rule, 0x02 - hairpin ports paired\n"
> " 0x01 - hairpin ports loop, 0x00 - hairpin port self\n");
> + printf(" --rx-metadata: send rx metadata to driver \n");

In accordance with the above "printf", one should have 2 space
characters between the opening double quote character and "--".
Also, it is quite common to capitalise "R" in "Rx".
The space character before "\n" seems redundant.

Regarding the option name, I would appreciate if it gets more
precise and clear. Consider: "--nic-to-pmd-rx-metadata".
I do not insist on this particular name.

Perhaps it pays to have a bit more verbose description as well.
Consider: "let the NIC deliver per-packet Rx metadata to PMD".

> }
> #ifdef RTE_LIB_CMDLINE
> @@ -710,6 +711,7 @@ launch_args_parse(int argc, char** argv)
> { "record-burst-stats", 0, 0, 0 },
> { PARAM_NUM_PROCS, 1, 0, 0 },
> { PARAM_PROC_ID, 1, 0, 0 },
> + { "rx-metadata", 0, 0, 0 },
> { 0, 0, 0, 0 },
> };
> @@ -1510,6 +1512,8 @@ launch_args_parse(int argc, char** argv)
> num_procs = atoi(optarg);
> if (!strcmp(lgopts[opt_idx].name, PARAM_PROC_ID))
> proc_id = atoi(optarg);
> + if (!strcmp(lgopts[opt_idx].name, "rx-metadata"))
> + rx_metadata_negotiate = 1;
> break;
> case 'h':
> usage(argv[0]);
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index addcbcac85..ebbde5dfc9 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -411,6 +411,9 @@ uint8_t clear_ptypes = true;
> /* Hairpin ports configuration mode. */
> uint16_t hairpin_mode;
> +/* send rx metadata */

Consider: "/* Send Rx metadata */".

> +uint8_t rx_metadata_negotiate;
> +
> /* Pretty printing of ethdev events */
> static const char * const eth_event_desc[] = {
> [RTE_ETH_EVENT_UNKNOWN] = "unknown",
> @@ -1628,7 +1631,8 @@ init_config_port_offloads(portid_t pid, uint32_t socket_id)
> int ret;
> int i;
> - eth_rx_metadata_negotiate_mp(pid);
> + if (rx_metadata_negotiate)
> + eth_rx_metadata_negotiate_mp(pid);
> port->dev_conf.txmode = tx_mode;
> port->dev_conf.rxmode = rx_mode;
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index fb2f5195d3..8a9168c51e 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -621,6 +621,8 @@ extern struct rte_ether_addr peer_eth_addrs[RTE_MAX_ETHPORTS];
> extern uint32_t burst_tx_delay_time; /**< Burst tx delay time(us) for mac-retry. */
> extern uint32_t burst_tx_retry_num; /**< Burst tx retry number for mac-retry. */
> +extern uint8_t rx_metadata_negotiate;
> +
> #ifdef RTE_LIB_GRO
> #define GRO_DEFAULT_ITEM_NUM_PER_FLOW 32
> #define GRO_DEFAULT_FLOW_NUM (RTE_GRO_MAX_BURST_ITEM_NUM / \
>
> -- 
> 2.25.1
>
>

--
Best regards,
Ivan M