Re: [PATCH v4 2/7] ethdev: support setting and querying RSS algorithm

[Ajit Khaparde <ajit.khaparde@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 3924 bytes --]

On Wed, Sep 20, 2023 at 9:39 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 9/8/2023 9:00 AM, Jie Hai wrote:
> > Currently, rte_eth_rss_conf supports configuring and querying
> > RSS hash functions, rss key and it's length, but not RSS hash
> > algorithm.
> >
> > The structure ``rte_eth_rss_conf`` is extended by adding a new
> > field "func". This represents the RSS algorithms to apply. The
> > following API will be affected:
> >       - rte_eth_dev_configure
> >       - rte_eth_dev_rss_hash_update
> >       - rte_eth_dev_rss_hash_conf_get
> >
>
> The problem with adding new configuration fields is, none of the drivers
> are using it.
>
> I can see you are updating hns3 driver in next patch, but what about
> others, application will set this field and almost all drivers will
> ignore it for now.
> And in near future, some will be supporting it and some won't, still
> frustrating for the application perspective.
>
> In the past I was gathering list of these items and follow
> implementation of them with drivers, but as the number of drivers
> increased this became very hard to manage.
>
> Another way to manage this is go and update all drivers, and if the
> configuration requests anything other than
> 'RTE_ETH_HASH_FUNCTION_DEFAULT', return error. So that application can
> know this is not supported by this driver.
I think it is a good option. And then a driver can modify it when it
adds support for
other algorithms.

> Do you have better solution for this?
>
:::: snip ::::
>
> > +};
> > +
> >  /**
> >   * A structure used to configure the Receive Side Scaling (RSS) feature
> >   * of an Ethernet port.
> > @@ -464,6 +484,7 @@ struct rte_eth_rss_conf {
> >        * Supplying an *rss_hf* equal to zero disables the RSS feature.
> >        */
> >       uint64_t rss_hf;
> > +     enum rte_eth_hash_function func;        /**< Hash algorithm. */
>
> Can we use 'algorithm' as the field name?
>
> I know it won't exactly match type name (rte_eth_hash_function) but I
> think this will be more accurate also less confusing for "rss_hf (rss
> hash function)" and "func (function)"?
+1


>
> >  };
> >
> >  /*
> > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> > index 271d854f7807..d3f2d466d841 100644
> > --- a/lib/ethdev/rte_flow.c
> > +++ b/lib/ethdev/rte_flow.c
> > @@ -12,7 +12,6 @@
> >  #include <rte_branch_prediction.h>
> >  #include <rte_string_fns.h>
> >  #include <rte_mbuf_dyn.h>
> > -#include "rte_ethdev.h"
> >  #include "rte_flow_driver.h"
> >  #include "rte_flow.h"
> >
> > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > index 3bd0dc64fbe2..a7578b1d2eff 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -40,6 +40,8 @@
> >  #include <rte_macsec.h>
> >  #include <rte_ib.h>
> >
> > +#include "rte_ethdev.h"
> > +
> >  #ifdef __cplusplus
> >  extern "C" {
> >  #endif
> > @@ -3183,26 +3185,6 @@ struct rte_flow_query_count {
> >       uint64_t bytes; /**< Number of bytes through this rule [out]. */
> >  };
> >
> > -/**
> > - * Hash function types.
> > - */
> > -enum rte_eth_hash_function {
> > -     /**
> > -      * DEFAULT means that conformance to a specific hash algorithm is
> > -      * uncared to the caller. The driver can pick the one it deems optimal.
> > -      */
> > -     RTE_ETH_HASH_FUNCTION_DEFAULT = 0,
> > -     RTE_ETH_HASH_FUNCTION_TOEPLITZ, /**< Toeplitz */
> > -     RTE_ETH_HASH_FUNCTION_SIMPLE_XOR, /**< Simple XOR */
> > -     /**
> > -      * Symmetric Toeplitz: src, dst will be replaced by
> > -      * xor(src, dst). For the case with src/dst only,
> > -      * src or dst address will xor with zero pair.
> > -      */
> > -     RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ,
> > -     RTE_ETH_HASH_FUNCTION_MAX,
> > -};
> > -
> >  /**
> >   * RTE_FLOW_ACTION_TYPE_RSS
> >   *
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]