Hello Ferruh,

As a brainstorm, what do you think move hairpin related code to its own
file, and have a kind of register logic to the commands, argument
parsing and usage() functions etc.

Please consider an option for the testpmd to dynamically add supported commands.
Testpmd can still provide some build-in core functionality, but custom test commands
can be added and removed on demand.
In addition, the custom command code does not have to be part of the testpmd.
That will reduce maintanance.

Regards,
Gregory

________________________________
From: Ferruh Yigit <ferruh.yigit@amd.com>
Sent: Friday, March 1, 2024 20:54
To: Gregory Etelson <getelson@nvidia.com>; dev@dpdk.org <dev@dpdk.org>
Cc: Maayan Kashani <mkashani@nvidia.com>; NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>; David Marchand <david.marchand@redhat.com>
Subject: Re: [PATCH v3] testpmd: add hairpin-map parameter

External email: Use caution opening links or attachments


On 9/28/2023 4:36 PM, Gregory Etelson wrote:
> Hairpin offloads packet forwarding between ports.
> Packet is expected on Rx port <rp>, Rx queue <rq> and is delivered
> to Tx port <tp> Tx queue <tq>.
>
> Testpmd implements a static hairpin configuration scheme.
> The scheme implicitly matches next valid port for given <rp> or <tp>.
> That approach can be used in a single or double port setups only.
>
> The new parameter allows explicit selection of Rx and Tx ports and
> queues in hairpin configuration.
> The new `hairpin-map` parameter is provided with 5 parameters,
> separated by `:`
>
> `--hairpin-map=Rx port id:Rx queue:Tx port id:Tx queue:queues number`
>
> Testpmd operator can provide several `hairpin-map` parameters for
> different hairpin maps.
> Example:
>
> dpdk-testpmd <EAL params> -- \
>   <testpmd params> \
>   --rxq=2 --txq=2 --hairpinq=2 --hairpin-mode=0x12 \
>   --hairpin-map=0:2:1:2:1 \ # [1]
>   --hairpin-map=0:3:2:2:3   # [2]
>
> Hairpin map [1] binds Rx port 0, queue 2 with Tx port 1, queue 2.
> Hairpin map [2] binds
>   Rx port 0, queue 3 with Tx port 2, queue 2,
>   Rx port 0, queue 4 with Tx port 2, queue 3,
>   Rx port 0, queue 5 with Tx port 2, queue 4.
>
> The new `hairpin-map` parameter is optional.
> If omitted, testpmd will create "default" hairpin maps.
>

Hi Gregory,

This patch is waiting for a while in the patchwork, not because there is
a problem with the implementation, but I have a feeling that it is
bloating the testpmd code with the hairpin feature which is not commonly
supported, and I don't know what to do.

Also there was no review on the code ...


As a brainstorm, what do you think move hairpin related code to its own
file, and have a kind of register logic to the commands, argument
parsing and usage() functions etc.

@David did something similar to move PMD specific testpmd code to the
drivers [1].

This can be initial steps for the more modular testpmd design and we can
introduce more features with this approach in the future, I think
hairpin support is a good instance to start this work, because of its
limited impact to users.


[1]
Commit 592ab76f9f0f ("app/testpmd: register driver specific commands")
Commit 94b3c1a72507 ("net/i40e: move testpmd commands")

> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> ---
> v2: Fix Windows Server 2019 compilation failure.
> v3: Update the patch comment.
>     Fix typo.
> ---
>  app/test-pmd/parameters.c             |  63 ++++++++
>  app/test-pmd/testpmd.c                | 212 ++++++++++++++++++--------
>  app/test-pmd/testpmd.h                |  18 +++
>  doc/guides/testpmd_app_ug/run_app.rst |   3 +
>  4 files changed, 230 insertions(+), 66 deletions(-)
>
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index a9ca58339d..c6bdfdf06f 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -206,6 +206,12 @@ 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("  --hairpin-map=rxpi:rxq:txpi:txq:n: hairpin map.\n"
> +            "    rxpi - Rx port index.\n"
> +            "    rxq  - Rx queue.\n"
> +            "    txpi - Tx port index.\n"
> +            "    txq  - Tx queue.\n"
> +            "    n    - hairpin queues number.\n");
>

'rxpi' or 'port index' are not common usage in DPDK, what about
'rx_port' & 'Rx port id' or similar?

>  }
>
>  #ifdef RTE_LIB_CMDLINE
> @@ -588,6 +594,55 @@ parse_link_speed(int n)
>       return speed;
>  }
>
> +static __rte_always_inline
> +char *parse_hairpin_map_entry(char *input, char **next)
> +{
>

Why forcing this function to be inline, can make it static and left
decision to compiler.

<...>

> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
> index 6e9c552e76..a202c98b4c 100644
> --- a/doc/guides/testpmd_app_ug/run_app.rst
> +++ b/doc/guides/testpmd_app_ug/run_app.rst
> @@ -566,6 +566,9 @@ The command line options are:
>
>      The default value is 0. Hairpin will use single port mode and implicit Tx flow mode.
>
> +*   ``--hairpin-map=Rx port id:Rx queue:Tx port id:Tx queue:queues number``
> +
> +    Set explicit hairpin configuration.
>

What 'queues number' does is not intuitive, sample in the commit log
makes it easier to understand.
Can you please either explain or provide sample to clarify what "queues
number" is for?