Re: [PATCH v5 00/40] support setting and querying RSS algorithms

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 10/17/2023 3:06 PM, Thomas Monjalon wrote:
> Hello,
> 
> 11/10/2023 11:27, Jie Hai:
>>  app/proc-info/main.c                   | 32 ++++++++++-----
>>  app/test-pmd/cmdline.c                 | 29 ++++++++++---
>>  app/test-pmd/config.c                  | 38 ++++++++---------
>>  app/test-pmd/testpmd.h                 |  2 +-
>>  doc/guides/rel_notes/release_23_11.rst |  2 +
>>  drivers/net/atlantic/atl_ethdev.c      |  2 +
>>  drivers/net/axgbe/axgbe_ethdev.c       |  9 +++++
>>  drivers/net/bnx2x/bnx2x_ethdev.c       |  4 ++
>>  drivers/net/bnxt/bnxt_ethdev.c         |  6 +++
>>  drivers/net/bonding/rte_eth_bond_pmd.c |  6 +++
>>  drivers/net/cnxk/cnxk_ethdev.c         |  5 +++
>>  drivers/net/cnxk/cnxk_ethdev_ops.c     |  3 ++
>>  drivers/net/cpfl/cpfl_ethdev.c         |  6 +++
>>  drivers/net/cxgbe/cxgbe_ethdev.c       |  9 ++++-
>>  drivers/net/dpaa/dpaa_ethdev.c         |  7 ++++
>>  drivers/net/dpaa2/dpaa2_ethdev.c       |  7 ++++
>>  drivers/net/ena/ena_rss.c              |  3 ++
>>  drivers/net/enic/enic_ethdev.c         |  1 +
>>  drivers/net/enic/enic_main.c           |  3 ++
>>  drivers/net/fm10k/fm10k_ethdev.c       |  9 ++++-
>>  drivers/net/hinic/hinic_pmd_ethdev.c   |  3 ++
>>  drivers/net/hinic/hinic_pmd_rx.c       |  3 ++
>>  drivers/net/hns3/hns3_rss.c            | 47 ++++++++++++---------
>>  drivers/net/i40e/i40e_ethdev.c         |  7 ++++
>>  drivers/net/iavf/iavf_ethdev.c         |  6 +++
>>  drivers/net/ice/ice_dcf.c              |  3 ++
>>  drivers/net/ice/ice_dcf_ethdev.c       |  3 ++
>>  drivers/net/ice/ice_ethdev.c           |  7 ++++
>>  drivers/net/idpf/idpf_ethdev.c         |  6 +++
>>  drivers/net/igc/igc_ethdev.c           |  4 ++
>>  drivers/net/igc/igc_txrx.c             |  5 +++
>>  drivers/net/ionic/ionic_ethdev.c       |  6 +++
>>  drivers/net/ixgbe/ixgbe_ethdev.c       | 12 +++++-
>>  drivers/net/ixgbe/ixgbe_rxtx.c         |  4 ++
>>  drivers/net/mana/mana.c                | 11 ++++-
>>  drivers/net/mlx5/mlx5_ethdev.c         |  4 ++
>>  drivers/net/mlx5/mlx5_rss.c            |  3 +-
>>  drivers/net/mvpp2/mrvl_ethdev.c        |  3 ++
>>  drivers/net/netvsc/hn_ethdev.c         |  6 +++
>>  drivers/net/nfp/nfp_common.c           |  9 ++++-
>>  drivers/net/ngbe/ngbe_ethdev.c         |  6 ++-
>>  drivers/net/ngbe/ngbe_rxtx.c           |  3 ++
>>  drivers/net/null/rte_eth_null.c        |  8 ++++
>>  drivers/net/qede/qede_ethdev.c         |  9 ++++-
>>  drivers/net/sfc/sfc_ethdev.c           |  3 ++
>>  drivers/net/sfc/sfc_rx.c               |  3 ++
>>  drivers/net/tap/rte_eth_tap.c          |  8 ++++
>>  drivers/net/thunderx/nicvf_ethdev.c    | 10 ++++-
>>  drivers/net/txgbe/txgbe_ethdev.c       |  7 +++-
>>  drivers/net/txgbe/txgbe_ethdev_vf.c    |  7 +++-
>>  drivers/net/txgbe/txgbe_rxtx.c         |  3 ++
>>  lib/ethdev/rte_ethdev.c                | 17 ++++++++
>>  lib/ethdev/rte_ethdev.h                | 56 +++++++++++++++++++-------
>>  lib/ethdev/rte_flow.c                  |  1 -
>>  lib/ethdev/rte_flow.h                  | 25 +-----------
>>  55 files changed, 395 insertions(+), 106 deletions(-)
> 
> Changing all drivers is suspicious.
> It shows that something is missing in ethdev.
> Please can you add the checks in ethdev only?
> 

That is kind of request from me, let me try to summarize what is going on,

there is a new config item added to "struct rte_eth_rss_conf" introduced
in this set, which is RSS hashing algorithm to use.

Problem is none of the existing drivers are taking account this new
config item, so application will request it but drivers silently ignore it.

This is a generic problem when adding a new config item to existing
config struct.

So my request was if drivers not supporting it, and it is requested by
the application, driver should return an error to let application know
that it is not supported, that is why bunch of drivers updated.


One option can be adding a new, specific API and dev_ops for this, for
this case new config item is related to the existing RSS API, so I think
it can be part of the existing API.

Other can be to have some kind of capability reporting by drivers, and
application will detect it and won't request this new config item, I
think Stephen already suggested something like this. This capability
flag is again a generic requirement, and `rte_eth_dev_info_get()`
partially used for this purpose. I think it will be odd from application
perspective to have a capability for just one config item of a feature set.


Anyway, I think updating drivers to report they are not supporting new
config item is best option to me, but also I think we should discuss
this capability reporting in ethdev in a wider context.