Re: [PATCH v3 00/26] add meson config options for queues per port

[David Marchand <david.marchand@redhat.com>]

On Wed, Aug 14, 2024 at 12:50 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> There are a number of issues with the current RTE_MAX_QUEUES_PER_PORT
> setting in DPDK that are addressed by this patchset:
>
> * The name does not make it clear that this is intended as an
>   ethdev-only setting
> * A number of other libraries are using this define rather than having
>   more relevant defines for the particular usecase.
> * The define is hard-coded in DPDK source code and is not adjustable via
>   a build-time/meson option
> * Because of the lack of configurability, the max is therefore set to a
>   conservatively-high value, wasting memory.
> * There is an assumption that the number of Rx queues and Tx queues
>   should have the same maximum value. Depending on application, it may
>   be desirable to have fan-in with multiple Rx queues e.g. for
>   classification/filtering, feed a single Tx queue, or the opposite
>   where, e.g. for QoS Tx scheduling, a few Rx queues feeds a very large
>   number of Tx queues.
>
> This patchset therefore addresses these by:
>
> * replacing the single define for max queues with independent defines
>   for Rx and Tx queues.
> * adjusts the name to ensure that it is clear the defines are for
>   ethports only. [ethports being used in the RTE_MAX_ETHPORTS setting].
> * replaces occurances of RTE_MAX_QUEUES_PER_PORT with appropriate
>   defines for non-ethdev use cases
> * replaces all other internal occurances of the define with the new
>   per-Rx and per-Tx definitions.
> * adds meson config options to allow build-time configuration of the max
>   Rx and Tx queue values.
>
> Naming Note:
> * The new meson config options are called "max_ethport_rx_queues" and
>   "max_ethport_tx_queues" so that in the meson options list they appear
>   alphabetically beside the existing "max_ethports" option.
> * For naming consistency, the new C defines are therefore
>   RTE_MAX_ETHPORT_RX_QUEUES and RTE_MAX_ETHPORT_TX_QUEUES.
>
> V3:
> * Resend of v2 with correct author email, to avoid reply bounces
> * drop "rfc" prefix from patches
>
> V2:
> * What was a single patch with "3 insertions(+), 1 deletion(-)" has now
>   become a 26-patch set! :-)
> * Created separate Rx and Tx defines
> * Ensured that the name makes it clear that the define is for ethdev
> * When updating internal use, created one patch per component for easier
>   maintainer review. In most cases it was obvious whether Rx or Tx
>   define should be used, but a few cases were less clear.
> * Added documentation updates for the changes (release notes and
>   deprecation notice), spread across 3 of the patches.
>
> Bruce Richardson (26):
>   cryptodev: remove use of ethdev max queues definition
>   eventdev: remove use of ethev queues define
>   app/test-bbdev: remove use of ethdev queue count value
>   config: add separate defines for max Rx and Tx queues
>   ethdev: use separate Rx and Tx queue limits
>   bpf: use separate Rx and Tx queue limits
>   latencystats: use separate Rx and Tx queue limits
>   pdump: use separate Rx and Tx queue limits
>   power: use separate Rx and Tx queue limits
>   net/af_xdp: use separate Rx and Tx queue limits
>   net/cnxk: use separate Rx and Tx queue limits
>   net/failsafe: use separate Rx and Tx queue limits
>   net/hns3: use separate Rx and Tx queue limits
>   net/mlx5: use separate Rx and Tx queue limits
>   net/null: use separate Rx and Tx queue limits
>   net/sfc: use separate Rx and Tx queue limits
>   net/thunderx: use separate Rx and Tx queue limits
>   net/vhost: use separate Rx and Tx queue limits
>   app/dumpcap: use separate Rx and Tx queue limits
>   app/test-pmd: use separate Rx and Tx queue limits
>   examples/ipsec-secgw: use separate Rx and Tx queue limits
>   examples/l3fwd-power: use separate Rx and Tx queue limits
>   examples/l3fwd: use separate Rx and Tx queue limits
>   examples/vhost: use separate Rx and Tx queue limits
>   config: make queues per port a meson config option
>   config: add computed max queues define for compatibility
>
>  app/dumpcap/main.c                     |  2 +-
>  app/test-bbdev/test_bbdev.c            |  4 ++--
>  app/test-pmd/testpmd.c                 |  7 ++++---
>  app/test-pmd/testpmd.h                 | 16 ++++++++--------
>  config/meson.build                     | 10 ++++++++++
>  config/rte_config.h                    |  2 +-
>  doc/guides/rel_notes/deprecation.rst   | 11 +++++++++++
>  doc/guides/rel_notes/release_24_11.rst | 21 +++++++++++++++++++++
>  drivers/net/af_xdp/rte_eth_af_xdp.c    |  2 +-
>  drivers/net/cnxk/cnxk_ethdev_ops.c     |  4 ++--
>  drivers/net/failsafe/failsafe_ops.c    |  4 ++--
>  drivers/net/hns3/hns3_tm.c             |  2 +-
>  drivers/net/mlx5/mlx5_flow.c           |  2 +-
>  drivers/net/mlx5/mlx5_flow_hw.c        |  2 +-
>  drivers/net/null/rte_eth_null.c        |  4 ++--
>  drivers/net/sfc/sfc_sw_stats.c         |  6 ++++--
>  drivers/net/thunderx/nicvf_ethdev.c    |  2 +-
>  drivers/net/vhost/rte_eth_vhost.c      |  7 ++++---
>  examples/ipsec-secgw/ipsec-secgw.c     |  2 +-
>  examples/ipsec-secgw/ipsec.c           |  2 +-
>  examples/l3fwd-power/main.c            |  2 +-
>  examples/l3fwd-power/perf_core.c       |  2 +-
>  examples/l3fwd/main.c                  |  2 +-
>  examples/vhost/main.c                  |  2 +-
>  examples/vhost/main.h                  |  2 +-
>  lib/bpf/bpf_pkt.c                      |  3 ++-
>  lib/cryptodev/cryptodev_pmd.c          |  4 ++--
>  lib/ethdev/ethdev_driver.h             |  8 ++++----
>  lib/ethdev/ethdev_private.c            | 24 ++++++++++++++----------
>  lib/ethdev/rte_ethdev.c                | 16 +++++++---------
>  lib/ethdev/rte_ethdev.h                | 18 +++++++++---------
>  lib/eventdev/eventdev_private.c        |  2 +-
>  lib/latencystats/rte_latencystats.c    |  4 ++--
>  lib/pdump/rte_pdump.c                  | 18 +++++++++---------
>  lib/power/rte_power_pmd_mgmt.c         |  4 ++--
>  meson_options.txt                      |  4 ++++
>  36 files changed, 140 insertions(+), 87 deletions(-)
>

I sent some comments.

Patch 2 has a typo "ethev" in its title.

I would squash the drivers changes into a single patch, as those are mechanical.

The last two patches may have to be squashed as I suspect compilation
is broken for applications relying on RTE_MAX_QUEUES_PER_PORT if we
stop between the two changes.

Otherwise, lgtm.


-- 
David Marchand