[RFC] config: make queues per port a meson config option

The default number of ethernet queues per port is currently set to
1k which is more than enough for most applications, but still is lower
than the total number of queues which may be available on modern NICs.
Rather than increasing the max queues further, which will increase
the memory footprint (since the value is used in array dimensioning),
we can instead make the value a meson tunable option - and reduce the
default value to 256 in the process. This means that:

* most apps which don't need hundreds of queues will see lower mem use.
* apps which do need to use thousands of queues can configure DPDK to
  allow this, without having to modify DPDK files (i.e. rte_config.h)

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 config/meson.build  | 1 +
 config/rte_config.h | 1 -
 meson_options.txt   | 2 ++
 3 files changed, 3 insertions(+), 1 deletion(-)
  

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> 
> The default number of ethernet queues per port is currently set to
> 1k which is more than enough for most applications, but still is lower
> than the total number of queues which may be available on modern NICs.
> Rather than increasing the max queues further, which will increase
> the memory footprint (since the value is used in array dimensioning),
> we can instead make the value a meson tunable option - and reduce the
> default value to 256 in the process.

Overall, I agree that this tunable is not very exotic, and can be exposed as suggested.
The reduction of the default value must be mentioned in the release notes.


>  # set other values pulled from the build options
>  dpdk_conf.set('RTE_MAX_ETHPORTS', get_option('max_ethports'))
> +dpdk_conf.set('RTE_MAX_QUEUES_PER_PORT',
> get_option('max_queues_per_ethport'))

Please rename RTE_MAX_QUEUES_PER_PORT to _PER_ETHPORT, so it resembles MAX_ETHPORTS. For API backwards compatibility, you can add:
#define RTE_MAX_QUEUES_PER_PORT RTE_MAX_QUEUES_PER_ETHPORT


I wonder if it would be possible to have separate max sizes for RX and TX queues? If it can save a non-negligible amount of memory, it might be useful for some applications.


With suggested changes (splitting RX/TX maximums not required),
Acked-by: Morten Brørup <mb@smartsharesystems.com>
  

On Mon, Aug 12, 2024 at 04:10:49PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > 
> > The default number of ethernet queues per port is currently set to
> > 1k which is more than enough for most applications, but still is lower
> > than the total number of queues which may be available on modern NICs.
> > Rather than increasing the max queues further, which will increase
> > the memory footprint (since the value is used in array dimensioning),
> > we can instead make the value a meson tunable option - and reduce the
> > default value to 256 in the process.
> 
> Overall, I agree that this tunable is not very exotic, and can be exposed as suggested.
> The reduction of the default value must be mentioned in the release notes.
> 

Yes, good point. I'll add that in any next version.

> 
> >  # set other values pulled from the build options
> >  dpdk_conf.set('RTE_MAX_ETHPORTS', get_option('max_ethports'))
> > +dpdk_conf.set('RTE_MAX_QUEUES_PER_PORT',
> > get_option('max_queues_per_ethport'))
> 
> Please rename RTE_MAX_QUEUES_PER_PORT to _PER_ETHPORT, so it resembles MAX_ETHPORTS. For API backwards compatibility, you can add:
> #define RTE_MAX_QUEUES_PER_PORT RTE_MAX_QUEUES_PER_ETHPORT
> 

Agree that would more consistent. That would probably best be a separate
patch, since we'd want to convert all internal use over. Will make this a
two-patch set in next version.

> 
> I wonder if it would be possible to have separate max sizes for RX and TX queues? If it can save a non-negligible amount of memory, it might be useful for some applications.
> 

That is an interesting idea. It would certainly allow saving memory for
use-cases where you want a large number of rx queues only, or tx queues
only. However, the defaults are still likely to remain the same. The main
issue I would have with it, is that it would mean having two build time
options rather than 1, and I'm a bit concerned at the number of options we
seem to be accumulating in DPDK.

Overall, I'm tending towards suggesting that we not do that split, but I'm
open to being convinced on it.

> 
> With suggested changes (splitting RX/TX maximums not required),
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>
  

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> 
> On Mon, Aug 12, 2024 at 04:10:49PM +0200, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > >
> > > The default number of ethernet queues per port is currently set to
> > > 1k which is more than enough for most applications, but still is lower
> > > than the total number of queues which may be available on modern NICs.
> > > Rather than increasing the max queues further, which will increase
> > > the memory footprint (since the value is used in array dimensioning),
> > > we can instead make the value a meson tunable option - and reduce the
> > > default value to 256 in the process.
> >
> > Overall, I agree that this tunable is not very exotic, and can be exposed as
> suggested.
> > The reduction of the default value must be mentioned in the release notes.
> >
> 
> Yes, good point. I'll add that in any next version.

ACK.

> 
> >
> > >  # set other values pulled from the build options
> > >  dpdk_conf.set('RTE_MAX_ETHPORTS', get_option('max_ethports'))
> > > +dpdk_conf.set('RTE_MAX_QUEUES_PER_PORT',
> > > get_option('max_queues_per_ethport'))
> >
> > Please rename RTE_MAX_QUEUES_PER_PORT to _PER_ETHPORT, so it resembles
> MAX_ETHPORTS. For API backwards compatibility, you can add:
> > #define RTE_MAX_QUEUES_PER_PORT RTE_MAX_QUEUES_PER_ETHPORT
> >
> 
> Agree that would more consistent. That would probably best be a separate
> patch, since we'd want to convert all internal use over. Will make this a
> two-patch set in next version.

ACK. And agree about two-patch series.

> 
> >
> > I wonder if it would be possible to have separate max sizes for RX and TX
> queues? If it can save a non-negligible amount of memory, it might be useful
> for some applications.
> >
> 
> That is an interesting idea. It would certainly allow saving memory for
> use-cases where you want a large number of rx queues only, or tx queues
> only. However, the defaults are still likely to remain the same. The main
> issue I would have with it, is that it would mean having two build time
> options rather than 1, and I'm a bit concerned at the number of options we
> seem to be accumulating in DPDK.
> 
> Overall, I'm tending towards suggesting that we not do that split, but I'm
> open to being convinced on it.

I would guess that many applications have an asymmetrical split of number of RX/TX queues. So I would argue that:
The reason to make this configurable in meson is to conserve memory, so why only go half the way if there is more memory to be conserved?

The distros will use oversize maximums anyway, but custom built applications might benefit.

Here's a weird thought:
Perhaps RX and TX maximums can be controlled individually by changing rte_config.h, and they can be overridden via one meson configuration parameter to set both (to the same value).

> 
> >
> > With suggested changes (splitting RX/TX maximums not required),
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >

My ACK remains; splitting RX/TX maximums is not Must Have, it is Nice To Have.
  

On Mon, Aug 12, 2024 at 05:02:11PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > 
> > On Mon, Aug 12, 2024 at 04:10:49PM +0200, Morten Brørup wrote:
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > >
> > > > The default number of ethernet queues per port is currently set to
> > > > 1k which is more than enough for most applications, but still is lower
> > > > than the total number of queues which may be available on modern NICs.
> > > > Rather than increasing the max queues further, which will increase
> > > > the memory footprint (since the value is used in array dimensioning),
> > > > we can instead make the value a meson tunable option - and reduce the
> > > > default value to 256 in the process.
> > >
> > > Overall, I agree that this tunable is not very exotic, and can be exposed as
> > suggested.
> > > The reduction of the default value must be mentioned in the release notes.
> > >
> > 
> > Yes, good point. I'll add that in any next version.
> 
> ACK.
> 
> > 
> > >
> > > >  # set other values pulled from the build options
> > > >  dpdk_conf.set('RTE_MAX_ETHPORTS', get_option('max_ethports'))
> > > > +dpdk_conf.set('RTE_MAX_QUEUES_PER_PORT',
> > > > get_option('max_queues_per_ethport'))
> > >
> > > Please rename RTE_MAX_QUEUES_PER_PORT to _PER_ETHPORT, so it resembles
> > MAX_ETHPORTS. For API backwards compatibility, you can add:
> > > #define RTE_MAX_QUEUES_PER_PORT RTE_MAX_QUEUES_PER_ETHPORT
> > >
> > 
> > Agree that would more consistent. That would probably best be a separate
> > patch, since we'd want to convert all internal use over. Will make this a
> > two-patch set in next version.
> 
> ACK. And agree about two-patch series.
> 
> > 
> > >
> > > I wonder if it would be possible to have separate max sizes for RX and TX
> > queues? If it can save a non-negligible amount of memory, it might be useful
> > for some applications.
> > >
> > 
> > That is an interesting idea. It would certainly allow saving memory for
> > use-cases where you want a large number of rx queues only, or tx queues
> > only. However, the defaults are still likely to remain the same. The main
> > issue I would have with it, is that it would mean having two build time
> > options rather than 1, and I'm a bit concerned at the number of options we
> > seem to be accumulating in DPDK.
> > 
> > Overall, I'm tending towards suggesting that we not do that split, but I'm
> > open to being convinced on it.
> 
> I would guess that many applications have an asymmetrical split of number of RX/TX queues. So I would argue that:
> The reason to make this configurable in meson is to conserve memory, so why only go half the way if there is more memory to be conserved?
> 
> The distros will use oversize maximums anyway, but custom built applications might benefit.
> 
> Here's a weird thought:
> Perhaps RX and TX maximums can be controlled individually by changing rte_config.h, and they can be overridden via one meson configuration parameter to set both (to the same value).
> 
> > 
> > >
> > > With suggested changes (splitting RX/TX maximums not required),
> > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > >
> 
> My ACK remains; splitting RX/TX maximums is not Must Have, it is Nice To Have.
>
Let me see how much in involved in trying to split...
  

> From: Bruce Richardson [mailto:bruce.richards@intel.com]
> 
> 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.
> 
> 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.

Thanks.

For the series,
Acked-by: Morten Brørup <mb@smartsharesystems.com>
  

> From: Bruce Richardson [mailto:bruce.richards@intel.com]
> 
> 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.
> 
> 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.

Thanks.

For the series,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

@Bruce: There's something wrong with your "From" email address; bruce.richards@ bounces.
So I resent this reply to your bruce.richardson@ address.
  

On Wed, Aug 14, 2024 at 09:48:46AM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richards@intel.com]
> > 
> > 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.
> > 
> > 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.
> 
> Thanks.
> 
> For the series,
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 
> @Bruce: There's something wrong with your "From" email address; bruce.richards@ bounces.
> So I resent this reply to your bruce.richardson@ address.
>
Yes, indeed. Something has indeed got messed up - probably in my git
configuration here. I'll resend a v3 to try and correct it, so that others
don't get any bounces.

/Bruce
  

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.
  

On Thu, Sep 19, 2024 at 04:14:28PM +0200, David Marchand wrote:
> 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.

Sure. Will do a new revision in future if others are similarly ok with it.

> 
> I would squash the drivers changes into a single patch, as those are mechanical.
> 
Yep, makes sense. I split them out for easier review. Are you or Thomas ok
to squash those on apply as I'd rather keep them separate for maintainers
while the changes are still in patchwork?

> 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.

Yes, good point that it might be. Will squash in later revisions.

> 
> Otherwise, lgtm.
> 
One open question is whether we are doing the right thing to have separate
defines for Tx queues and Rx queues?

I think it's useful to have the separate defines, but there has been a
comment suggesting that we are better keeping the single define. My own
thinking is that the single-define is appropriate for offload devices since
the queues tend to come in pairs - since everything sent down to the device
comes back up again - but for the NICs, we can have wild assymmetry
depending on use case, for example, for QoS on Tx.

I take it you are ok with the two-define solution then?

/Bruce