[v15] app/testpmd: support multi-process
Checks
Commit Message
From: "Min Hu (Connor)" <humin29@huawei.com>
For example the following commands run two testpmd processes:
* the primary process:
./dpdk-testpmd --proc-type=auto -l 0-1 -- -i \
--rxq=4 --txq=4 --num-procs=2 --proc-id=0
* the secondary process:
./dpdk-testpmd --proc-type=auto -l 2-3 -- -i \
--rxq=4 --txq=4 --num-procs=2 --proc-id=1
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
Signed-off-by: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
v15:
* Fixed release notes.
* Cleanup documentation.
v14:
* Fixed comments by Andrew Rybchenko.
v13:
* Modified the doc syntax.
v12:
* Updated doc info.
v11:
* Fixed some minor syntax.
v10:
* Hid process type checks behind new functions.
* Added comments.
v9:
* Updated release notes and rst doc.
* Deleted deprecated codes.
* move macro and variable.
v8:
* Added warning info about queue numbers and process numbers.
v7:
* Fixed compiling error for unexpected unindent.
v6:
* Add rte flow description for multiple process.
v5:
* Fixed run_app.rst for multiple process description.
* Fix compiling error.
v4:
* Fixed minimum vlaue of Rxq or Txq in doc.
v3:
* Fixed compiling error using gcc10.0.
v2:
* Added document for this patch.
---
app/test-pmd/cmdline.c | 6 ++
app/test-pmd/config.c | 20 +++-
app/test-pmd/parameters.c | 9 ++
app/test-pmd/testpmd.c | 121 ++++++++++++++++++++-----
app/test-pmd/testpmd.h | 9 ++
doc/guides/rel_notes/release_21_08.rst | 5 +
doc/guides/testpmd_app_ug/run_app.rst | 82 +++++++++++++++++
7 files changed, 228 insertions(+), 24 deletions(-)
Comments
On 7/2/21 3:09 PM, Andrew Rybchenko wrote:
> From: "Min Hu (Connor)" <humin29@huawei.com>
>
> For example the following commands run two testpmd processes:
>
> * the primary process:
>
> ./dpdk-testpmd --proc-type=auto -l 0-1 -- -i \
> --rxq=4 --txq=4 --num-procs=2 --proc-id=0
>
> * the secondary process:
>
> ./dpdk-testpmd --proc-type=auto -l 2-3 -- -i \
> --rxq=4 --txq=4 --num-procs=2 --proc-id=1
>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> Signed-off-by: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
[snip]
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 1cdd3cdd1..a5da0c272 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -520,6 +520,62 @@ enum rte_eth_rx_mq_mode rx_mq_mode = ETH_MQ_RX_VMDQ_DCB_RSS;
> */
> uint32_t eth_link_speed;
>
> +/*
> + * ID of the current process in multi-process, used to
> + * configure the queues to be polled.
> + */
> +int proc_id;
> +
> +/*
> + * Number of processes in multi-process, used to
> + * configure the queues to be polled.
> + */
> +unsigned int num_procs = 1;
> +
> +static int
> +eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> + const struct rte_eth_conf *dev_conf)
> +{
> + if (is_proc_primary())
> + return rte_eth_dev_configure(port_id, nb_rx_q, nb_tx_q,
> + dev_conf);
> + return 0;
> +}
> +
> +static int
> +eth_dev_start_mp(uint16_t port_id)
> +{
> + if (is_proc_primary())
> + return rte_eth_dev_start(port_id);
> +
> + return 0;
> +}
> +
> +static int
> +eth_dev_stop_mp(uint16_t port_id)
> +{
> + if (is_proc_primary())
> + return rte_eth_dev_stop(port_id);
> +
> + return 0;
> +}
> +
> +static void
> +mempool_free_mp(struct rte_mempool *mp)
> +{
> + if (is_proc_primary())
> + rte_mempool_free(mp);
> +}
> +
> +static int
> +eth_dev_set_mtu_mp(uint16_t port_id, uint16_t mtu)
> +{
> + if (is_proc_primary())
> + return rte_eth_dev_set_mtu(port_id, mtu);
> +
> + return 0;
> +}
> +
I think above functions should be removed and corresponding
checks should be done in caller directly since above functions
are used in single place only and just hide what actually
happens in the case of secondary process. It is very
misleading.
[snip]
> @@ -2495,21 +2565,24 @@ start_port(portid_t pid)
> return -1;
> }
> /* configure port */
> - diag = rte_eth_dev_configure(pi, nb_rxq + nb_hairpinq,
> - nb_txq + nb_hairpinq,
> - &(port->dev_conf));
> + diag = eth_dev_configure_mp(pi,
> + nb_rxq + nb_hairpinq,
> + nb_txq + nb_hairpinq,
> + &(port->dev_conf));
> if (diag != 0) {
> - if (rte_atomic16_cmpset(&(port->port_status),
> - RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
> - printf("Port %d can not be set back "
> - "to stopped\n", pi);
> + if (rte_atomic16_cmpset(
> + &(port->port_status),
> + RTE_PORT_HANDLING,
> + RTE_PORT_STOPPED) == 0)
> + printf("Port %d cannot be set back to stopped\n",
> + pi);
Unrelated changes in the patch should be avoided since
it just makes the review harder.
> printf("Fail to configure port %d\n", pi);
> /* try to reconfigure port next time */
> port->need_reconfig = 1;
> return -1;
> }
> }
> - if (port->need_reconfig_queues > 0) {
> + if (port->need_reconfig_queues > 0 && is_proc_primary()) {
> port->need_reconfig_queues = 0;
> /* setup tx queues */
> for (qi = 0; qi < nb_txq; qi++) {
> @@ -2532,8 +2605,8 @@ start_port(portid_t pid)
> if (rte_atomic16_cmpset(&(port->port_status),
> RTE_PORT_HANDLING,
> RTE_PORT_STOPPED) == 0)
> - printf("Port %d can not be set back "
> - "to stopped\n", pi);
> + printf("Port %d cannot be set back to stopped\n",
> + pi);
Unrelated changes in the patch should be avoided.
> printf("Fail to configure port %d tx queues\n",
> pi);
> /* try to reconfigure queues next time */
> @@ -2610,16 +2683,16 @@ start_port(portid_t pid)
> cnt_pi++;
>
> /* start port */
> - diag = rte_eth_dev_start(pi);
> + diag = eth_dev_start_mp(pi);
> if (diag < 0) {
> printf("Fail to start port %d: %s\n", pi,
> rte_strerror(-diag));
>
> /* Fail to setup rx queue, return */
> if (rte_atomic16_cmpset(&(port->port_status),
> - RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
> - printf("Port %d can not be set back to "
> - "stopped\n", pi);
> + RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
> + printf("Port %d cannot be set back to stopped\n",
> + pi);
Unrelated changes in the patch should be avoided.
[snip]
> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
> index eb4831835..348e5fcac 100644
> --- a/doc/guides/testpmd_app_ug/run_app.rst
> +++ b/doc/guides/testpmd_app_ug/run_app.rst
> @@ -545,3 +545,85 @@ The command line options are:
> bit 0 - two hairpin ports loop
>
> The default value is 0. Hairpin will use single port mode and implicit Tx flow mode.
> +
> +
> +Testpmd Multi-Process Command-line Options
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The following are the command-line options for testpmd multi-process support:
> +
> +* primary process:
> +
> +.. code-block:: console
> +
> + sudo ./dpdk-testpmd --proc-type=auto -l 0-1 -- -i --rxq=4 --txq=4 \
> + --num-procs=2 --proc-id=0
> +
> +* secondary process:
> +
> +.. code-block:: console
> +
> + sudo ./dpdk-testpmd --proc-type=auto -l 2-3 -- -i --rxq=4 --txq=4 \
> + --num-procs=2 --proc-id=1
> +
> +The command line options are:
> +
> +* ``--num-procs=N``
> +
> + The number of processes which will be used.
> +
> +* ``--proc-id=ID``
> +
> + The ID of the current process (ID < num-procs). ID should be different in
> + primary process and secondary process, which starts from '0'.
> +
> +Calculation rule for queue:
> +All queues are allocated to different processes based on ``proc_num`` and
> +``proc_id``.
> +Calculation rule for the testpmd to allocate queues to each process:
> +
> +* start(queue start id) = proc_id * nb_q / num_procs
> +
> +* end(queue end id) = start + nb_q / num_procs
> +
> +For example, if testpmd is configured to have 4 Tx and Rx queues,
> +queues 0 and 1 will be used by the primary process and
> +queues 2 and 3 will be used by the secondary process.
> +
> +The number of queues should be a multiple of the number of processes. If not,
> +redundant queues will exist after queues are allocated to processes. If RSS
> +is enabled, packet loss occurs when traffic is sent to all processes at the same
> +time. Some traffic goes to redundant queues and cannot be forwarded.
> +
> +All the dev ops is supported in primary process. While secondary process is
> +not permitted to allocate or release shared memory, so some ops are not supported
> +as follows:
> +
> +- ``dev_configure``
> +- ``dev_start``
> +- ``dev_stop``
> +- ``rx_queue_setup``
> +- ``tx_queue_setup``
> +- ``rx_queue_release``
> +- ``tx_queue_release``
> +
> +So, any command from testpmd which calls those APIs will not be supported in
> +secondary process, like:
> +
> +.. code-block:: console
> +
> + port config all rxq|txq|rxd|txd <value>
> + port config <port_id> rx_offload xxx on/off
> + port config <port_id> tx_offload xxx on/off
> +
> +etc.
I did the formatting cleanup, but I still think that testpmd
guide should not dive into such level of details. It should
rather highlight multi-process behaviour specifics.
Shouldn't testpmd store state in shared memory to avoid
problems when primary is stopped while secondary is running
etc.
Some testpmd features rely on reconfigure (i.e. simply change
configuration and set flag that reconfigure is required), but
configure does nothing and will simply ignore new settings.
So, it could look very-very confusing from user point of view.
I'm not sure that it is acceptable to apply the patch in such
state and open huge number of bugs in testpmd behaviour when
multi-process is used.
I'd even consider to exclude unsupported commands from help
etc. However, such level of care about user could be excessive
for test tool.
IMHO, it should be no requirement to repeat the primary
process command-line configuration in the second process
command line (see --rxq=4 --txq=4 above). The information
should be obtained from shared state. In theory primary
process could even change some settings in interactive
mode. I think testpmd should guarantee consistent behaviour
even in such conditions. I.e. do not allow to stop ports
used by forwarding running in secondary processes.
Run-time queues setup and deferred start should be very
carefully handled as well.
> +
> +Stats is supported, stats will not change when one quits and starts, as they
> +share the same buffer to store the stats. Flow rules are maintained in process
> +level: primary and secondary has its own flow list (but one flow list in HW).
> +The two can see all the queues, so setting the flow rules for the other is OK.
> +But in the testpmd primary process receiving or transmitting packets from the
> +queue allocated for secondary process is not permitted, and same for secondary
> +process.
> +
> +Flow API and RSS are supported.
>
Hi, Andrew ,
在 2021/7/2 20:47, Andrew Rybchenko 写道:
> On 7/2/21 3:09 PM, Andrew Rybchenko wrote:
>> From: "Min Hu (Connor)" <humin29@huawei.com>
>>
>> For example the following commands run two testpmd processes:
>>
>> * the primary process:
>>
>> ./dpdk-testpmd --proc-type=auto -l 0-1 -- -i \
>> --rxq=4 --txq=4 --num-procs=2 --proc-id=0
>>
>> * the secondary process:
>>
>> ./dpdk-testpmd --proc-type=auto -l 2-3 -- -i \
>> --rxq=4 --txq=4 --num-procs=2 --proc-id=1
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> Signed-off-by: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
> [snip]
>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index 1cdd3cdd1..a5da0c272 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -520,6 +520,62 @@ enum rte_eth_rx_mq_mode rx_mq_mode = ETH_MQ_RX_VMDQ_DCB_RSS;
>> */
>> uint32_t eth_link_speed;
>>
>> +/*
>> + * ID of the current process in multi-process, used to
>> + * configure the queues to be polled.
>> + */
>> +int proc_id;
>> +
>> +/*
>> + * Number of processes in multi-process, used to
>> + * configure the queues to be polled.
>> + */
>> +unsigned int num_procs = 1;
>> +
>> +static int
>> +eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>> + const struct rte_eth_conf *dev_conf)
>> +{
>> + if (is_proc_primary())
>> + return rte_eth_dev_configure(port_id, nb_rx_q, nb_tx_q,
>> + dev_conf);
>> + return 0;
>> +}
>> +
>> +static int
>> +eth_dev_start_mp(uint16_t port_id)
>> +{
>> + if (is_proc_primary())
>> + return rte_eth_dev_start(port_id);
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +eth_dev_stop_mp(uint16_t port_id)
>> +{
>> + if (is_proc_primary())
>> + return rte_eth_dev_stop(port_id);
>> +
>> + return 0;
>> +}
>> +
>> +static void
>> +mempool_free_mp(struct rte_mempool *mp)
>> +{
>> + if (is_proc_primary())
>> + rte_mempool_free(mp);
>> +}
>> +
>> +static int
>> +eth_dev_set_mtu_mp(uint16_t port_id, uint16_t mtu)
>> +{
>> + if (is_proc_primary())
>> + return rte_eth_dev_set_mtu(port_id, mtu);
>> +
>> + return 0;
>> +}
>> +
>
> I think above functions should be removed and corresponding
> checks should be done in caller directly since above functions
> are used in single place only and just hide what actually
> happens in the case of secondary process. It is very
> misleading.
>
This was done as Ferruh suggested in V9， and this could reduce
the complexity for testpmd when added by the multi-process support.
> [snip]
>
>> @@ -2495,21 +2565,24 @@ start_port(portid_t pid)
>> return -1;
>> }
>> /* configure port */
>> - diag = rte_eth_dev_configure(pi, nb_rxq + nb_hairpinq,
>> - nb_txq + nb_hairpinq,
>> - &(port->dev_conf));
>> + diag = eth_dev_configure_mp(pi,
>> + nb_rxq + nb_hairpinq,
>> + nb_txq + nb_hairpinq,
>> + &(port->dev_conf));
>> if (diag != 0) {
>> - if (rte_atomic16_cmpset(&(port->port_status),
>> - RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
>> - printf("Port %d can not be set back "
>> - "to stopped\n", pi);
>> + if (rte_atomic16_cmpset(
>> + &(port->port_status),
>> + RTE_PORT_HANDLING,
>> + RTE_PORT_STOPPED) == 0)
>> + printf("Port %d cannot be set back to stopped\n",
>> + pi);
>
> Unrelated changes in the patch should be avoided since
> it just makes the review harder.This will be fixed in v16.
>
>> printf("Fail to configure port %d\n", pi);
>> /* try to reconfigure port next time */
>> port->need_reconfig = 1;
>> return -1;
>> }
>> }
>> - if (port->need_reconfig_queues > 0) {
>> + if (port->need_reconfig_queues > 0 && is_proc_primary()) {
>> port->need_reconfig_queues = 0;
>> /* setup tx queues */
>> for (qi = 0; qi < nb_txq; qi++) {
>> @@ -2532,8 +2605,8 @@ start_port(portid_t pid)
>> if (rte_atomic16_cmpset(&(port->port_status),
>> RTE_PORT_HANDLING,
>> RTE_PORT_STOPPED) == 0)
>> - printf("Port %d can not be set back "
>> - "to stopped\n", pi);
>> + printf("Port %d cannot be set back to stopped\n",
>> + pi);
>
> Unrelated changes in the patch should be avoided.
This will be fixed in v16.
>
>> printf("Fail to configure port %d tx queues\n",
>> pi);
>> /* try to reconfigure queues next time */
>> @@ -2610,16 +2683,16 @@ start_port(portid_t pid)
>> cnt_pi++;
>>
>> /* start port */
>> - diag = rte_eth_dev_start(pi);
>> + diag = eth_dev_start_mp(pi);
>> if (diag < 0) {
>> printf("Fail to start port %d: %s\n", pi,
>> rte_strerror(-diag));
>>
>> /* Fail to setup rx queue, return */
>> if (rte_atomic16_cmpset(&(port->port_status),
>> - RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
>> - printf("Port %d can not be set back to "
>> - "stopped\n", pi);
>> + RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
>> + printf("Port %d cannot be set back to stopped\n",
>> + pi);
>
> Unrelated changes in the patch should be avoided.
This will be fixed in v16.
>
> [snip]
>
>> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
>> index eb4831835..348e5fcac 100644
>> --- a/doc/guides/testpmd_app_ug/run_app.rst
>> +++ b/doc/guides/testpmd_app_ug/run_app.rst
>> @@ -545,3 +545,85 @@ The command line options are:
>> bit 0 - two hairpin ports loop
>>
>> The default value is 0. Hairpin will use single port mode and implicit Tx flow mode.
>> +
>> +
>> +Testpmd Multi-Process Command-line Options
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +The following are the command-line options for testpmd multi-process support:
>> +
>> +* primary process:
>> +
>> +.. code-block:: console
>> +
>> + sudo ./dpdk-testpmd --proc-type=auto -l 0-1 -- -i --rxq=4 --txq=4 \
>> + --num-procs=2 --proc-id=0
>> +
>> +* secondary process:
>> +
>> +.. code-block:: console
>> +
>> + sudo ./dpdk-testpmd --proc-type=auto -l 2-3 -- -i --rxq=4 --txq=4 \
>> + --num-procs=2 --proc-id=1
>> +
>> +The command line options are:
>> +
>> +* ``--num-procs=N``
>> +
>> + The number of processes which will be used.
>> +
>> +* ``--proc-id=ID``
>> +
>> + The ID of the current process (ID < num-procs). ID should be different in
>> + primary process and secondary process, which starts from '0'.
>> +
>> +Calculation rule for queue:
>> +All queues are allocated to different processes based on ``proc_num`` and
>> +``proc_id``.
>> +Calculation rule for the testpmd to allocate queues to each process:
>> +
>> +* start(queue start id) = proc_id * nb_q / num_procs
>> +
>> +* end(queue end id) = start + nb_q / num_procs
>> +
>> +For example, if testpmd is configured to have 4 Tx and Rx queues,
>> +queues 0 and 1 will be used by the primary process and
>> +queues 2 and 3 will be used by the secondary process.
>> +
>> +The number of queues should be a multiple of the number of processes. If not,
>> +redundant queues will exist after queues are allocated to processes. If RSS
>> +is enabled, packet loss occurs when traffic is sent to all processes at the same
>> +time. Some traffic goes to redundant queues and cannot be forwarded.
>> +
>> +All the dev ops is supported in primary process. While secondary process is
>> +not permitted to allocate or release shared memory, so some ops are not supported
>> +as follows:
>> +
>> +- ``dev_configure``
>> +- ``dev_start``
>> +- ``dev_stop``
>> +- ``rx_queue_setup``
>> +- ``tx_queue_setup``
>> +- ``rx_queue_release``
>> +- ``tx_queue_release``
>> +
>> +So, any command from testpmd which calls those APIs will not be supported in
>> +secondary process, like:
>> +
>> +.. code-block:: console
>> +
>> + port config all rxq|txq|rxd|txd <value>
>> + port config <port_id> rx_offload xxx on/off
>> + port config <port_id> tx_offload xxx on/off
>> +
>> +etc.
>
> I did the formatting cleanup, but I still think that testpmd
> guide should not dive into such level of details. It should
> rather highlight multi-process behaviour specifics.
>
> Shouldn't testpmd store state in shared memory to avoid
> problems when primary is stopped while secondary is running
This could be taken into consideration in future.
>
> Some testpmd features rely on reconfigure (i.e. simply change
> configuration and set flag that reconfigure is required), but
> configure does nothing and will simply ignore new settings.
> So, it could look very-very confusing from user point of view.
>
> I'm not sure that it is acceptable to apply the patch in such
> state and open huge number of bugs in testpmd behaviour when
> multi-process is used.
>
> I'd even consider to exclude unsupported commands from help
> etc. However, such level of care about user could be excessive
> for test tool.
This has been done in doc.
>
> IMHO, it should be no requirement to repeat the primary
> process command-line configuration in the second process
> command line (see --rxq=4 --txq=4 above). The information
> should be obtained from shared state. In theory primary
> process could even change some settings in interactive
We think keeping the command line in consistent between primary
and secondary is easy to understand for users. While shared memory for
keeping in order or communicating could be performed,but this could
be done in future patch.
> mode. I think testpmd should guarantee consistent behaviour
> even in such conditions. I.e. do not allow to stop ports
> used by forwarding running in secondary processes.
> Run-time queues setup and deferred start should be very
> carefully handled as well.
``dev_stop`` is not allowed in secondary, which has described in doc.
>> +
>> +Stats is supported, stats will not change when one quits and starts, as they
>> +share the same buffer to store the stats. Flow rules are maintained in process
>> +level: primary and secondary has its own flow list (but one flow list in HW).
>> +The two can see all the queues, so setting the flow rules for the other is OK.
>> +But in the testpmd primary process receiving or transmitting packets from the
>> +queue allocated for secondary process is not permitted, and same for secondary
>> +process.
>> +
>> +Flow API and RSS are supported.
>>Thanks for your comment,
This patch supports basic function for multi-process support in testpmd.
I think other patches in future could enhance or optimize it, thanks.
>
> .
>
On 7/8/21 3:20 PM, Min Hu (Connor) wrote:
> Hi, Andrew ,
>
> 在 2021/7/2 20:47, Andrew Rybchenko 写道:
>> On 7/2/21 3:09 PM, Andrew Rybchenko wrote:
>>> From: "Min Hu (Connor)" <humin29@huawei.com>
>>>
>>> For example the following commands run two testpmd processes:
>>>
>>> * the primary process:
>>>
>>> ./dpdk-testpmd --proc-type=auto -l 0-1 -- -i \
>>> --rxq=4 --txq=4 --num-procs=2 --proc-id=0
>>>
>>> * the secondary process:
>>>
>>> ./dpdk-testpmd --proc-type=auto -l 2-3 -- -i \
>>> --rxq=4 --txq=4 --num-procs=2 --proc-id=1
>>>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>> Signed-off-by: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
>>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>
>> [snip]
>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index 1cdd3cdd1..a5da0c272 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -520,6 +520,62 @@ enum rte_eth_rx_mq_mode rx_mq_mode =
>>> ETH_MQ_RX_VMDQ_DCB_RSS;
>>> */
>>> uint32_t eth_link_speed;
>>> +/*
>>> + * ID of the current process in multi-process, used to
>>> + * configure the queues to be polled.
>>> + */
>>> +int proc_id;
>>> +
>>> +/*
>>> + * Number of processes in multi-process, used to
>>> + * configure the queues to be polled.
>>> + */
>>> +unsigned int num_procs = 1;
>>> +
>>> +static int
>>> +eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t
>>> nb_tx_q,
>>> + const struct rte_eth_conf *dev_conf)
>>> +{
>>> + if (is_proc_primary())
>>> + return rte_eth_dev_configure(port_id, nb_rx_q, nb_tx_q,
>>> + dev_conf);
>>> + return 0;
>>> +}
>>> +
>>> +static int
>>> +eth_dev_start_mp(uint16_t port_id)
>>> +{
>>> + if (is_proc_primary())
>>> + return rte_eth_dev_start(port_id);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int
>>> +eth_dev_stop_mp(uint16_t port_id)
>>> +{
>>> + if (is_proc_primary())
>>> + return rte_eth_dev_stop(port_id);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void
>>> +mempool_free_mp(struct rte_mempool *mp)
>>> +{
>>> + if (is_proc_primary())
>>> + rte_mempool_free(mp);
>>> +}
>>> +
>>> +static int
>>> +eth_dev_set_mtu_mp(uint16_t port_id, uint16_t mtu)
>>> +{
>>> + if (is_proc_primary())
>>> + return rte_eth_dev_set_mtu(port_id, mtu);
>>> +
>>> + return 0;
>>> +}
>>> +
>>
>> I think above functions should be removed and corresponding
>> checks should be done in caller directly since above functions
>> are used in single place only and just hide what actually
>> happens in the case of secondary process. It is very
>> misleading.
>>
> This was done as Ferruh suggested in V9， and this could reduce
> the complexity for testpmd when added by the multi-process support.
>> [snip]
>>
>>> @@ -2495,21 +2565,24 @@ start_port(portid_t pid)
>>> return -1;
>>> }
>>> /* configure port */
>>> - diag = rte_eth_dev_configure(pi, nb_rxq + nb_hairpinq,
>>> - nb_txq + nb_hairpinq,
>>> - &(port->dev_conf));
>>> + diag = eth_dev_configure_mp(pi,
>>> + nb_rxq + nb_hairpinq,
>>> + nb_txq + nb_hairpinq,
>>> + &(port->dev_conf));
>>> if (diag != 0) {
>>> - if (rte_atomic16_cmpset(&(port->port_status),
>>> - RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
>>> - printf("Port %d can not be set back "
>>> - "to stopped\n", pi);
>>> + if (rte_atomic16_cmpset(
>>> + &(port->port_status),
>>> + RTE_PORT_HANDLING,
>>> + RTE_PORT_STOPPED) == 0)
>>> + printf("Port %d cannot be set back to stopped\n",
>>> + pi);
>>
>> Unrelated changes in the patch should be avoided since
>> it just makes the review harder.This will be fixed in v16.
>>
>>> printf("Fail to configure port %d\n", pi);
>>> /* try to reconfigure port next time */
>>> port->need_reconfig = 1;
>>> return -1;
>>> }
>>> }
>>> - if (port->need_reconfig_queues > 0) {
>>> + if (port->need_reconfig_queues > 0 && is_proc_primary()) {
>>> port->need_reconfig_queues = 0;
>>> /* setup tx queues */
>>> for (qi = 0; qi < nb_txq; qi++) {
>>> @@ -2532,8 +2605,8 @@ start_port(portid_t pid)
>>> if (rte_atomic16_cmpset(&(port->port_status),
>>> RTE_PORT_HANDLING,
>>> RTE_PORT_STOPPED) == 0)
>>> - printf("Port %d can not be set back "
>>> - "to stopped\n", pi);
>>> + printf("Port %d cannot be set back to stopped\n",
>>> + pi);
>>
>> Unrelated changes in the patch should be avoided.
> This will be fixed in v16.
>>
>>> printf("Fail to configure port %d tx queues\n",
>>> pi);
>>> /* try to reconfigure queues next time */
>>> @@ -2610,16 +2683,16 @@ start_port(portid_t pid)
>>> cnt_pi++;
>>> /* start port */
>>> - diag = rte_eth_dev_start(pi);
>>> + diag = eth_dev_start_mp(pi);
>>> if (diag < 0) {
>>> printf("Fail to start port %d: %s\n", pi,
>>> rte_strerror(-diag));
>>> /* Fail to setup rx queue, return */
>>> if (rte_atomic16_cmpset(&(port->port_status),
>>> - RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
>>> - printf("Port %d can not be set back to "
>>> - "stopped\n", pi);
>>> + RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
>>> + printf("Port %d cannot be set back to stopped\n",
>>> + pi);
>>
>> Unrelated changes in the patch should be avoided.
> This will be fixed in v16.
>>
>> [snip]
>>
>>> diff --git a/doc/guides/testpmd_app_ug/run_app.rst
>>> b/doc/guides/testpmd_app_ug/run_app.rst
>>> index eb4831835..348e5fcac 100644
>>> --- a/doc/guides/testpmd_app_ug/run_app.rst
>>> +++ b/doc/guides/testpmd_app_ug/run_app.rst
>>> @@ -545,3 +545,85 @@ The command line options are:
>>> bit 0 - two hairpin ports loop
>>> The default value is 0. Hairpin will use single port mode and
>>> implicit Tx flow mode.
>>> +
>>> +
>>> +Testpmd Multi-Process Command-line Options
>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +The following are the command-line options for testpmd multi-process
>>> support:
>>> +
>>> +* primary process:
>>> +
>>> +.. code-block:: console
>>> +
>>> + sudo ./dpdk-testpmd --proc-type=auto -l 0-1 -- -i --rxq=4
>>> --txq=4 \
>>> + --num-procs=2 --proc-id=0
>>> +
>>> +* secondary process:
>>> +
>>> +.. code-block:: console
>>> +
>>> + sudo ./dpdk-testpmd --proc-type=auto -l 2-3 -- -i --rxq=4
>>> --txq=4 \
>>> + --num-procs=2 --proc-id=1
>>> +
>>> +The command line options are:
>>> +
>>> +* ``--num-procs=N``
>>> +
>>> + The number of processes which will be used.
>>> +
>>> +* ``--proc-id=ID``
>>> +
>>> + The ID of the current process (ID < num-procs). ID should be
>>> different in
>>> + primary process and secondary process, which starts from '0'.
>>> +
>>> +Calculation rule for queue:
>>> +All queues are allocated to different processes based on
>>> ``proc_num`` and
>>> +``proc_id``.
>>> +Calculation rule for the testpmd to allocate queues to each process:
>>> +
>>> +* start(queue start id) = proc_id * nb_q / num_procs
>>> +
>>> +* end(queue end id) = start + nb_q / num_procs
>>> +
>>> +For example, if testpmd is configured to have 4 Tx and Rx queues,
>>> +queues 0 and 1 will be used by the primary process and
>>> +queues 2 and 3 will be used by the secondary process.
>>> +
>>> +The number of queues should be a multiple of the number of
>>> processes. If not,
>>> +redundant queues will exist after queues are allocated to processes.
>>> If RSS
>>> +is enabled, packet loss occurs when traffic is sent to all processes
>>> at the same
>>> +time. Some traffic goes to redundant queues and cannot be forwarded.
>>> +
>>> +All the dev ops is supported in primary process. While secondary
>>> process is
>>> +not permitted to allocate or release shared memory, so some ops are
>>> not supported
>>> +as follows:
>>> +
>>> +- ``dev_configure``
>>> +- ``dev_start``
>>> +- ``dev_stop``
>>> +- ``rx_queue_setup``
>>> +- ``tx_queue_setup``
>>> +- ``rx_queue_release``
>>> +- ``tx_queue_release``
>>> +
>>> +So, any command from testpmd which calls those APIs will not be
>>> supported in
>>> +secondary process, like:
>>> +
>>> +.. code-block:: console
>>> +
>>> + port config all rxq|txq|rxd|txd <value>
>>> + port config <port_id> rx_offload xxx on/off
>>> + port config <port_id> tx_offload xxx on/off
>>> +
>>> +etc.
>>
>> I did the formatting cleanup, but I still think that testpmd
>> guide should not dive into such level of details. It should
>> rather highlight multi-process behaviour specifics.
>>
>> Shouldn't testpmd store state in shared memory to avoid
>> problems when primary is stopped while secondary is running
> This could be taken into consideration in future.
>
>>
>> Some testpmd features rely on reconfigure (i.e. simply change
>> configuration and set flag that reconfigure is required), but
>> configure does nothing and will simply ignore new settings.
>> So, it could look very-very confusing from user point of view.
>>
>> I'm not sure that it is acceptable to apply the patch in such
>> state and open huge number of bugs in testpmd behaviour when
>> multi-process is used.
>>
>> I'd even consider to exclude unsupported commands from help
>> etc. However, such level of care about user could be excessive
>> for test tool.
> This has been done in doc.
>>
>> IMHO, it should be no requirement to repeat the primary
>> process command-line configuration in the second process
>> command line (see --rxq=4 --txq=4 above). The information
>> should be obtained from shared state. In theory primary
>> process could even change some settings in interactive
> We think keeping the command line in consistent between primary
> and secondary is easy to understand for users. While shared memory for
> keeping in order or communicating could be performed,but this could
> be done in future patch.
>
>> mode. I think testpmd should guarantee consistent behaviour
>> even in such conditions. I.e. do not allow to stop ports
>> used by forwarding running in secondary processes.
>> Run-time queues setup and deferred start should be very
>> carefully handled as well.
> ``dev_stop`` is not allowed in secondary, which has described in doc.
I'm talking about dev_stop in primary while secondary is
running.
>>> +
>>> +Stats is supported, stats will not change when one quits and starts,
>>> as they
>>> +share the same buffer to store the stats. Flow rules are maintained
>>> in process
>>> +level: primary and secondary has its own flow list (but one flow
>>> list in HW).
>>> +The two can see all the queues, so setting the flow rules for the
>>> other is OK.
>>> +But in the testpmd primary process receiving or transmitting packets
>>> from the
>>> +queue allocated for secondary process is not permitted, and same for
>>> secondary
>>> +process.
>>> +
>>> +Flow API and RSS are supported.
>>> Thanks for your comment,
> This patch supports basic function for multi-process support in testpmd.
> I think other patches in future could enhance or optimize it, thanks.
IMHO, as I state above, current state is insufficient to
consider is a start point to be applied.
Hi,Andrew ,
在 2021/7/8 20:30, Andrew Rybchenko 写道:
> On 7/8/21 3:20 PM, Min Hu (Connor) wrote:
>> Hi, Andrew ,
>>
>> 在 2021/7/2 20:47, Andrew Rybchenko 写道:
>>> On 7/2/21 3:09 PM, Andrew Rybchenko wrote:
>>>> From: "Min Hu (Connor)" <humin29@huawei.com>
>>>>
>>>> For example the following commands run two testpmd processes:
>>>>
>>>> * the primary process:
>>>>
>>>> ./dpdk-testpmd --proc-type=auto -l 0-1 -- -i \
>>>> --rxq=4 --txq=4 --num-procs=2 --proc-id=0
>>>>
>>>> * the secondary process:
>>>>
>>>> ./dpdk-testpmd --proc-type=auto -l 2-3 -- -i \
>>>> --rxq=4 --txq=4 --num-procs=2 --proc-id=1
>>>>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>> Signed-off-by: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
>>>> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>>>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>
>>> [snip]
>>>
>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>>> index 1cdd3cdd1..a5da0c272 100644
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -520,6 +520,62 @@ enum rte_eth_rx_mq_mode rx_mq_mode =
>>>> ETH_MQ_RX_VMDQ_DCB_RSS;
>>>> */
>>>> uint32_t eth_link_speed;
>>>> +/*
>>>> + * ID of the current process in multi-process, used to
>>>> + * configure the queues to be polled.
>>>> + */
>>>> +int proc_id;
>>>> +
>>>> +/*
>>>> + * Number of processes in multi-process, used to
>>>> + * configure the queues to be polled.
>>>> + */
>>>> +unsigned int num_procs = 1;
>>>> +
>>>> +static int
>>>> +eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t
>>>> nb_tx_q,
>>>> + const struct rte_eth_conf *dev_conf)
>>>> +{
>>>> + if (is_proc_primary())
>>>> + return rte_eth_dev_configure(port_id, nb_rx_q, nb_tx_q,
>>>> + dev_conf);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int
>>>> +eth_dev_start_mp(uint16_t port_id)
>>>> +{
>>>> + if (is_proc_primary())
>>>> + return rte_eth_dev_start(port_id);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int
>>>> +eth_dev_stop_mp(uint16_t port_id)
>>>> +{
>>>> + if (is_proc_primary())
>>>> + return rte_eth_dev_stop(port_id);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void
>>>> +mempool_free_mp(struct rte_mempool *mp)
>>>> +{
>>>> + if (is_proc_primary())
>>>> + rte_mempool_free(mp);
>>>> +}
>>>> +
>>>> +static int
>>>> +eth_dev_set_mtu_mp(uint16_t port_id, uint16_t mtu)
>>>> +{
>>>> + if (is_proc_primary())
>>>> + return rte_eth_dev_set_mtu(port_id, mtu);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>
>>> I think above functions should be removed and corresponding
>>> checks should be done in caller directly since above functions
>>> are used in single place only and just hide what actually
>>> happens in the case of secondary process. It is very
>>> misleading.
>>>
>> This was done as Ferruh suggested in V9， and this could reduce
>> the complexity for testpmd when added by the multi-process support.
>>> [snip]
>>>
>>>> @@ -2495,21 +2565,24 @@ start_port(portid_t pid)
>>>> return -1;
>>>> }
>>>> /* configure port */
>>>> - diag = rte_eth_dev_configure(pi, nb_rxq + nb_hairpinq,
>>>> - nb_txq + nb_hairpinq,
>>>> - &(port->dev_conf));
>>>> + diag = eth_dev_configure_mp(pi,
>>>> + nb_rxq + nb_hairpinq,
>>>> + nb_txq + nb_hairpinq,
>>>> + &(port->dev_conf));
>>>> if (diag != 0) {
>>>> - if (rte_atomic16_cmpset(&(port->port_status),
>>>> - RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
>>>> - printf("Port %d can not be set back "
>>>> - "to stopped\n", pi);
>>>> + if (rte_atomic16_cmpset(
>>>> + &(port->port_status),
>>>> + RTE_PORT_HANDLING,
>>>> + RTE_PORT_STOPPED) == 0)
>>>> + printf("Port %d cannot be set back to stopped\n",
>>>> + pi);
>>>
>>> Unrelated changes in the patch should be avoided since
>>> it just makes the review harder.This will be fixed in v16.
>>>
>>>> printf("Fail to configure port %d\n", pi);
>>>> /* try to reconfigure port next time */
>>>> port->need_reconfig = 1;
>>>> return -1;
>>>> }
>>>> }
>>>> - if (port->need_reconfig_queues > 0) {
>>>> + if (port->need_reconfig_queues > 0 && is_proc_primary()) {
>>>> port->need_reconfig_queues = 0;
>>>> /* setup tx queues */
>>>> for (qi = 0; qi < nb_txq; qi++) {
>>>> @@ -2532,8 +2605,8 @@ start_port(portid_t pid)
>>>> if (rte_atomic16_cmpset(&(port->port_status),
>>>> RTE_PORT_HANDLING,
>>>> RTE_PORT_STOPPED) == 0)
>>>> - printf("Port %d can not be set back "
>>>> - "to stopped\n", pi);
>>>> + printf("Port %d cannot be set back to stopped\n",
>>>> + pi);
>>>
>>> Unrelated changes in the patch should be avoided.
>> This will be fixed in v16.
>>>
>>>> printf("Fail to configure port %d tx queues\n",
>>>> pi);
>>>> /* try to reconfigure queues next time */
>>>> @@ -2610,16 +2683,16 @@ start_port(portid_t pid)
>>>> cnt_pi++;
>>>> /* start port */
>>>> - diag = rte_eth_dev_start(pi);
>>>> + diag = eth_dev_start_mp(pi);
>>>> if (diag < 0) {
>>>> printf("Fail to start port %d: %s\n", pi,
>>>> rte_strerror(-diag));
>>>> /* Fail to setup rx queue, return */
>>>> if (rte_atomic16_cmpset(&(port->port_status),
>>>> - RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
>>>> - printf("Port %d can not be set back to "
>>>> - "stopped\n", pi);
>>>> + RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
>>>> + printf("Port %d cannot be set back to stopped\n",
>>>> + pi);
>>>
>>> Unrelated changes in the patch should be avoided.
>> This will be fixed in v16.
>>>
>>> [snip]
>>>
>>>> diff --git a/doc/guides/testpmd_app_ug/run_app.rst
>>>> b/doc/guides/testpmd_app_ug/run_app.rst
>>>> index eb4831835..348e5fcac 100644
>>>> --- a/doc/guides/testpmd_app_ug/run_app.rst
>>>> +++ b/doc/guides/testpmd_app_ug/run_app.rst
>>>> @@ -545,3 +545,85 @@ The command line options are:
>>>> bit 0 - two hairpin ports loop
>>>> The default value is 0. Hairpin will use single port mode and
>>>> implicit Tx flow mode.
>>>> +
>>>> +
>>>> +Testpmd Multi-Process Command-line Options
>>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> +
>>>> +The following are the command-line options for testpmd multi-process
>>>> support:
>>>> +
>>>> +* primary process:
>>>> +
>>>> +.. code-block:: console
>>>> +
>>>> + sudo ./dpdk-testpmd --proc-type=auto -l 0-1 -- -i --rxq=4
>>>> --txq=4 \
>>>> + --num-procs=2 --proc-id=0
>>>> +
>>>> +* secondary process:
>>>> +
>>>> +.. code-block:: console
>>>> +
>>>> + sudo ./dpdk-testpmd --proc-type=auto -l 2-3 -- -i --rxq=4
>>>> --txq=4 \
>>>> + --num-procs=2 --proc-id=1
>>>> +
>>>> +The command line options are:
>>>> +
>>>> +* ``--num-procs=N``
>>>> +
>>>> + The number of processes which will be used.
>>>> +
>>>> +* ``--proc-id=ID``
>>>> +
>>>> + The ID of the current process (ID < num-procs). ID should be
>>>> different in
>>>> + primary process and secondary process, which starts from '0'.
>>>> +
>>>> +Calculation rule for queue:
>>>> +All queues are allocated to different processes based on
>>>> ``proc_num`` and
>>>> +``proc_id``.
>>>> +Calculation rule for the testpmd to allocate queues to each process:
>>>> +
>>>> +* start(queue start id) = proc_id * nb_q / num_procs
>>>> +
>>>> +* end(queue end id) = start + nb_q / num_procs
>>>> +
>>>> +For example, if testpmd is configured to have 4 Tx and Rx queues,
>>>> +queues 0 and 1 will be used by the primary process and
>>>> +queues 2 and 3 will be used by the secondary process.
>>>> +
>>>> +The number of queues should be a multiple of the number of
>>>> processes. If not,
>>>> +redundant queues will exist after queues are allocated to processes.
>>>> If RSS
>>>> +is enabled, packet loss occurs when traffic is sent to all processes
>>>> at the same
>>>> +time. Some traffic goes to redundant queues and cannot be forwarded.
>>>> +
>>>> +All the dev ops is supported in primary process. While secondary
>>>> process is
>>>> +not permitted to allocate or release shared memory, so some ops are
>>>> not supported
>>>> +as follows:
>>>> +
>>>> +- ``dev_configure``
>>>> +- ``dev_start``
>>>> +- ``dev_stop``
>>>> +- ``rx_queue_setup``
>>>> +- ``tx_queue_setup``
>>>> +- ``rx_queue_release``
>>>> +- ``tx_queue_release``
>>>> +
>>>> +So, any command from testpmd which calls those APIs will not be
>>>> supported in
>>>> +secondary process, like:
>>>> +
>>>> +.. code-block:: console
>>>> +
>>>> + port config all rxq|txq|rxd|txd <value>
>>>> + port config <port_id> rx_offload xxx on/off
>>>> + port config <port_id> tx_offload xxx on/off
>>>> +
>>>> +etc.
>>>
>>> I did the formatting cleanup, but I still think that testpmd
>>> guide should not dive into such level of details. It should
>>> rather highlight multi-process behaviour specifics.
>>>
>>> Shouldn't testpmd store state in shared memory to avoid
>>> problems when primary is stopped while secondary is running
>> This could be taken into consideration in future.
>>
>>>
>>> Some testpmd features rely on reconfigure (i.e. simply change
>>> configuration and set flag that reconfigure is required), but
>>> configure does nothing and will simply ignore new settings.
>>> So, it could look very-very confusing from user point of view.
>>>
>>> I'm not sure that it is acceptable to apply the patch in such
>>> state and open huge number of bugs in testpmd behaviour when
>>> multi-process is used.
>>>
>>> I'd even consider to exclude unsupported commands from help
>>> etc. However, such level of care about user could be excessive
>>> for test tool.
>> This has been done in doc.
>>>
>>> IMHO, it should be no requirement to repeat the primary
>>> process command-line configuration in the second process
>>> command line (see --rxq=4 --txq=4 above). The information
>>> should be obtained from shared state. In theory primary
>>> process could even change some settings in interactive
>> We think keeping the command line in consistent between primary
>> and secondary is easy to understand for users. While shared memory for
>> keeping in order or communicating could be performed,but this could
>> be done in future patch.
>>
>>> mode. I think testpmd should guarantee consistent behaviour
>>> even in such conditions. I.e. do not allow to stop ports
>>> used by forwarding running in secondary processes.
>>> Run-time queues setup and deferred start should be very
>>> carefully handled as well.
>> ``dev_stop`` is not allowed in secondary, which has described in doc.
>
> I'm talking about dev_stop in primary while secondary is
> running.
>
>>>> +
>>>> +Stats is supported, stats will not change when one quits and starts,
>>>> as they
>>>> +share the same buffer to store the stats. Flow rules are maintained
>>>> in process
>>>> +level: primary and secondary has its own flow list (but one flow
>>>> list in HW).
>>>> +The two can see all the queues, so setting the flow rules for the
>>>> other is OK.
>>>> +But in the testpmd primary process receiving or transmitting packets
>>>> from the
>>>> +queue allocated for secondary process is not permitted, and same for
>>>> secondary
>>>> +process.
>>>> +
>>>> +Flow API and RSS are supported.
>>>> Thanks for your comment,
>> This patch supports basic function for multi-process support in testpmd.
>> I think other patches in future could enhance or optimize it, thanks.
>
> IMHO, as I state above, current state is insufficient to
> consider is a start point to be applied.
OK, what is the critical(the most important) as a start point to be applied?
I will take action to fix it,as I will the patch could be applied before
V21.08.
> .
>
@@ -5459,6 +5459,12 @@ cmd_set_flush_rx_parsed(void *parsed_result,
__rte_unused void *data)
{
struct cmd_set_flush_rx *res = parsed_result;
+
+ if (num_procs > 1 && (strcmp(res->mode, "on") == 0)) {
+ printf("multi-process doesn't support to flush Rx queues.\n");
+ return;
+ }
+
no_flush_rx = (uint8_t)((strcmp(res->mode, "on") == 0) ? 0 : 1);
}
@@ -2976,6 +2976,8 @@ rss_fwd_config_setup(void)
queueid_t rxq;
queueid_t nb_q;
streamid_t sm_id;
+ int start;
+ int end;
nb_q = nb_rxq;
if (nb_q > nb_txq)
@@ -2993,7 +2995,21 @@ rss_fwd_config_setup(void)
init_fwd_streams();
setup_fwd_config_of_each_lcore(&cur_fwd_config);
- rxp = 0; rxq = 0;
+
+ if (proc_id > 0 && nb_q % num_procs != 0)
+ printf("Warning! queue numbers should be multiple of processes, or packet loss will happen.\n");
+
+ /**
+ * In multi-process, All queues are allocated to different
+ * processes based on num_procs and proc_id. For example:
+ * if supports 4 queues(nb_q), 2 processes(num_procs),
+ * the 0~1 queue for primary process.
+ * the 2~3 queue for secondary process.
+ */
+ start = proc_id * nb_q / num_procs;
+ end = start + nb_q / num_procs;
+ rxp = 0;
+ rxq = start;
for (sm_id = 0; sm_id < cur_fwd_config.nb_fwd_streams; sm_id++) {
struct fwd_stream *fs;
@@ -3010,6 +3026,8 @@ rss_fwd_config_setup(void)
continue;
rxp = 0;
rxq++;
+ if (rxq >= end)
+ rxq = start;
}
}
@@ -509,6 +509,9 @@ parse_link_speed(int n)
void
launch_args_parse(int argc, char** argv)
{
+#define PARAM_PROC_ID "proc-id"
+#define PARAM_NUM_PROCS "num-procs"
+
int n, opt;
char **argvopt;
int opt_idx;
@@ -628,6 +631,8 @@ launch_args_parse(int argc, char** argv)
{ "rx-mq-mode", 1, 0, 0 },
{ "record-core-cycles", 0, 0, 0 },
{ "record-burst-stats", 0, 0, 0 },
+ { PARAM_NUM_PROCS, 1, 0, 0 },
+ { PARAM_PROC_ID, 1, 0, 0 },
{ 0, 0, 0, 0 },
};
@@ -1394,6 +1399,10 @@ launch_args_parse(int argc, char** argv)
record_core_cycles = 1;
if (!strcmp(lgopts[opt_idx].name, "record-burst-stats"))
record_burst_stats = 1;
+ if (!strcmp(lgopts[opt_idx].name, PARAM_NUM_PROCS))
+ num_procs = atoi(optarg);
+ if (!strcmp(lgopts[opt_idx].name, PARAM_PROC_ID))
+ proc_id = atoi(optarg);
break;
case 'h':
usage(argv[0]);
@@ -520,6 +520,62 @@ enum rte_eth_rx_mq_mode rx_mq_mode = ETH_MQ_RX_VMDQ_DCB_RSS;
*/
uint32_t eth_link_speed;
+/*
+ * ID of the current process in multi-process, used to
+ * configure the queues to be polled.
+ */
+int proc_id;
+
+/*
+ * Number of processes in multi-process, used to
+ * configure the queues to be polled.
+ */
+unsigned int num_procs = 1;
+
+static int
+eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
+ const struct rte_eth_conf *dev_conf)
+{
+ if (is_proc_primary())
+ return rte_eth_dev_configure(port_id, nb_rx_q, nb_tx_q,
+ dev_conf);
+ return 0;
+}
+
+static int
+eth_dev_start_mp(uint16_t port_id)
+{
+ if (is_proc_primary())
+ return rte_eth_dev_start(port_id);
+
+ return 0;
+}
+
+static int
+eth_dev_stop_mp(uint16_t port_id)
+{
+ if (is_proc_primary())
+ return rte_eth_dev_stop(port_id);
+
+ return 0;
+}
+
+static void
+mempool_free_mp(struct rte_mempool *mp)
+{
+ if (is_proc_primary())
+ rte_mempool_free(mp);
+}
+
+static int
+eth_dev_set_mtu_mp(uint16_t port_id, uint16_t mtu)
+{
+ if (is_proc_primary())
+ return rte_eth_dev_set_mtu(port_id, mtu);
+
+ return 0;
+}
+
/* Forward function declarations */
static void setup_attached_port(portid_t pi);
static void check_all_ports_link_status(uint32_t port_mask);
@@ -983,6 +1039,15 @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
#endif
mbuf_poolname_build(socket_id, pool_name, sizeof(pool_name), size_idx);
+ if (!is_proc_primary()) {
+ rte_mp = rte_mempool_lookup(pool_name);
+ if (rte_mp == NULL)
+ rte_exit(EXIT_FAILURE,
+ "Get mbuf pool for socket %u failed: %s\n",
+ socket_id, rte_strerror(rte_errno));
+ return rte_mp;
+ }
+
TESTPMD_LOG(INFO,
"create a new mbuf pool <%s>: n=%u, size=%u, socket=%u\n",
pool_name, nb_mbuf, mbuf_seg_size, socket_id);
@@ -2008,6 +2073,11 @@ flush_fwd_rx_queues(void)
uint64_t prev_tsc = 0, diff_tsc, cur_tsc, timer_tsc = 0;
uint64_t timer_period;
+ if (num_procs > 1) {
+ printf("multi-process not support for flushing fwd Rx queues, skip the below lines and return.\n");
+ return;
+ }
+
/* convert to number of cycles */
timer_period = rte_get_timer_hz(); /* 1 second timeout */
@@ -2495,21 +2565,24 @@ start_port(portid_t pid)
return -1;
}
/* configure port */
- diag = rte_eth_dev_configure(pi, nb_rxq + nb_hairpinq,
- nb_txq + nb_hairpinq,
- &(port->dev_conf));
+ diag = eth_dev_configure_mp(pi,
+ nb_rxq + nb_hairpinq,
+ nb_txq + nb_hairpinq,
+ &(port->dev_conf));
if (diag != 0) {
- if (rte_atomic16_cmpset(&(port->port_status),
- RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
- printf("Port %d can not be set back "
- "to stopped\n", pi);
+ if (rte_atomic16_cmpset(
+ &(port->port_status),
+ RTE_PORT_HANDLING,
+ RTE_PORT_STOPPED) == 0)
+ printf("Port %d cannot be set back to stopped\n",
+ pi);
printf("Fail to configure port %d\n", pi);
/* try to reconfigure port next time */
port->need_reconfig = 1;
return -1;
}
}
- if (port->need_reconfig_queues > 0) {
+ if (port->need_reconfig_queues > 0 && is_proc_primary()) {
port->need_reconfig_queues = 0;
/* setup tx queues */
for (qi = 0; qi < nb_txq; qi++) {
@@ -2532,8 +2605,8 @@ start_port(portid_t pid)
if (rte_atomic16_cmpset(&(port->port_status),
RTE_PORT_HANDLING,
RTE_PORT_STOPPED) == 0)
- printf("Port %d can not be set back "
- "to stopped\n", pi);
+ printf("Port %d cannot be set back to stopped\n",
+ pi);
printf("Fail to configure port %d tx queues\n",
pi);
/* try to reconfigure queues next time */
@@ -2610,16 +2683,16 @@ start_port(portid_t pid)
cnt_pi++;
/* start port */
- diag = rte_eth_dev_start(pi);
+ diag = eth_dev_start_mp(pi);
if (diag < 0) {
printf("Fail to start port %d: %s\n", pi,
rte_strerror(-diag));
/* Fail to setup rx queue, return */
if (rte_atomic16_cmpset(&(port->port_status),
- RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
- printf("Port %d can not be set back to "
- "stopped\n", pi);
+ RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
+ printf("Port %d cannot be set back to stopped\n",
+ pi);
continue;
}
@@ -2744,7 +2817,7 @@ stop_port(portid_t pid)
if (port->flow_list)
port_flow_flush(pi);
- if (rte_eth_dev_stop(pi) != 0)
+ if (eth_dev_stop_mp(pi) != 0)
RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for port %u\n",
pi);
@@ -2813,8 +2886,10 @@ close_port(portid_t pid)
continue;
}
- port_flow_flush(pi);
- rte_eth_dev_close(pi);
+ if (is_proc_primary()) {
+ port_flow_flush(pi);
+ rte_eth_dev_close(pi);
+ }
}
remove_invalid_ports();
@@ -3082,7 +3157,7 @@ pmd_test_exit(void)
}
for (i = 0 ; i < RTE_DIM(mempools) ; i++) {
if (mempools[i])
- rte_mempool_free(mempools[i]);
+ mempool_free_mp(mempools[i]);
}
printf("\nBye...\n");
@@ -3413,7 +3488,7 @@ update_jumbo_frame_offload(portid_t portid)
* if unset do it here
*/
if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
- ret = rte_eth_dev_set_mtu(portid,
+ ret = eth_dev_set_mtu_mp(portid,
port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
if (ret)
printf("Failed to set MTU to %u for port %u\n",
@@ -3603,6 +3678,10 @@ init_port_dcb_config(portid_t pid,
int retval;
uint16_t i;
+ if (num_procs > 1) {
+ printf("The multi-process feature doesn't support dcb.\n");
+ return -ENOTSUP;
+ }
rte_port = &ports[pid];
memset(&port_conf, 0, sizeof(struct rte_eth_conf));
@@ -3771,10 +3850,6 @@ main(int argc, char** argv)
rte_exit(EXIT_FAILURE, "Cannot init EAL: %s\n",
rte_strerror(rte_errno));
- if (rte_eal_process_type() == RTE_PROC_SECONDARY)
- rte_exit(EXIT_FAILURE,
- "Secondary process type not supported.\n");
-
ret = register_eth_event_callback();
if (ret != 0)
rte_exit(EXIT_FAILURE, "Cannot register for ethdev events");
@@ -632,6 +632,15 @@ extern enum rte_eth_rx_mq_mode rx_mq_mode;
extern struct rte_flow_action_conntrack conntrack_context;
+extern int proc_id;
+extern unsigned int num_procs;
+
+static inline bool
+is_proc_primary(void)
+{
+ return rte_eal_process_type() == RTE_PROC_PRIMARY;
+}
+
static inline unsigned int
lcore_num(void)
{
@@ -55,6 +55,11 @@ New Features
Also, make sure to start the actual text at the margin.
=======================================================
+ * **Added multi-process support to testpmd.**
+
+ Added command-line options to specify total number of processes and
+ current process ID. Each process owns subset of Rx and Tx queues.
+
Removed Items
-------------
@@ -545,3 +545,85 @@ The command line options are:
bit 0 - two hairpin ports loop
The default value is 0. Hairpin will use single port mode and implicit Tx flow mode.
+
+
+Testpmd Multi-Process Command-line Options
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The following are the command-line options for testpmd multi-process support:
+
+* primary process:
+
+.. code-block:: console
+
+ sudo ./dpdk-testpmd --proc-type=auto -l 0-1 -- -i --rxq=4 --txq=4 \
+ --num-procs=2 --proc-id=0
+
+* secondary process:
+
+.. code-block:: console
+
+ sudo ./dpdk-testpmd --proc-type=auto -l 2-3 -- -i --rxq=4 --txq=4 \
+ --num-procs=2 --proc-id=1
+
+The command line options are:
+
+* ``--num-procs=N``
+
+ The number of processes which will be used.
+
+* ``--proc-id=ID``
+
+ The ID of the current process (ID < num-procs). ID should be different in
+ primary process and secondary process, which starts from '0'.
+
+Calculation rule for queue:
+All queues are allocated to different processes based on ``proc_num`` and
+``proc_id``.
+Calculation rule for the testpmd to allocate queues to each process:
+
+* start(queue start id) = proc_id * nb_q / num_procs
+
+* end(queue end id) = start + nb_q / num_procs
+
+For example, if testpmd is configured to have 4 Tx and Rx queues,
+queues 0 and 1 will be used by the primary process and
+queues 2 and 3 will be used by the secondary process.
+
+The number of queues should be a multiple of the number of processes. If not,
+redundant queues will exist after queues are allocated to processes. If RSS
+is enabled, packet loss occurs when traffic is sent to all processes at the same
+time. Some traffic goes to redundant queues and cannot be forwarded.
+
+All the dev ops is supported in primary process. While secondary process is
+not permitted to allocate or release shared memory, so some ops are not supported
+as follows:
+
+- ``dev_configure``
+- ``dev_start``
+- ``dev_stop``
+- ``rx_queue_setup``
+- ``tx_queue_setup``
+- ``rx_queue_release``
+- ``tx_queue_release``
+
+So, any command from testpmd which calls those APIs will not be supported in
+secondary process, like:
+
+.. code-block:: console
+
+ port config all rxq|txq|rxd|txd <value>
+ port config <port_id> rx_offload xxx on/off
+ port config <port_id> tx_offload xxx on/off
+
+etc.
+
+Stats is supported, stats will not change when one quits and starts, as they
+share the same buffer to store the stats. Flow rules are maintained in process
+level: primary and secondary has its own flow list (but one flow list in HW).
+The two can see all the queues, so setting the flow rules for the other is OK.
+But in the testpmd primary process receiving or transmitting packets from the
+queue allocated for secondary process is not permitted, and same for secondary
+process.
+
+Flow API and RSS are supported.