Re: [PATCH v2 1/5] ethdev: support setting and querying RSS algorithm

[Thomas Monjalon <thomas@monjalon.net>]

09/09/2023 02:01, Ajit Khaparde:
> On Fri, Sep 8, 2023 at 1:44 AM Jie Hai <haijie1@huawei.com> wrote:
> >
> > Hi, Thomas
> > Thanks for your review.
> >
> > On 2023/9/4 15:45, Thomas Monjalon wrote:
> > > 04/09/2023 09:10, Jie Hai:
> > >> On 2023/8/30 19:46, Thomas Monjalon wrote:
> > >>> 26/08/2023 09:46, Jie Hai:
> > >   >> + * The *func* field of the *rss_conf* structure indicates the hash algorithm
> > >>>> + * applied by the RSS hashing. Passing RTE_ETH_HASH_FUNCTION_DEFAULT allows
> > >>>> + * the PMD to use its best-effort algorithm rather than a specific one.
> > >>>>     */
> > >>>
> > >>> I don't like commenting a field on top of the structure.
> > >>> By the way, the first sentence does not look helpful.
> > >>> RTE_ETH_HASH_FUNCTION_DEFAULT may be explained in the enum.
> > >>>
> > >> Other fields  above the structure 'rte_eth_rss_conf' have comments.
> > >> If the new fields 'func' do not have comments, it may be misleading.
> > >> Unless all the comments above are removed. I'm not sure whether to
> > >> delete them or not.
> > >
> > > You should explain RTE_ETH_HASH_FUNCTION_DEFAULT in its enum.
> > > The rest of the explanation can be on the struct field.
> > > I'm OK to have another patch moving old explanations from the struct
> > > to the fields.
> > Fixed in V4, please check it.
> > >
> > >>>>    struct rte_eth_rss_conf {
> > >>>>            uint8_t *rss_key;    /**< If not NULL, 40-byte hash key. */
> > >>>>            uint8_t rss_key_len; /**< hash key length in bytes. */
> > >>>>            uint64_t rss_hf;     /**< Hash functions to apply - see below. */
> > >>>> +  enum rte_eth_hash_function func;        /**< Hash algorithm to apply. */
> > >>>
> > >>> You can drop "to apply" words.
> > >> Fixed in V3.
> > >>>
> > >>> How the algorithms support combinations in rss_hf?
> > >>>
> > >> The rss_hf defines the input of the RSS hash algorithms.
> > >> For example, rss_hf = RTE_ETH_RSS_NONFRAG_IPV4_TCP | RTE_ETH_RSS_IPV4,
> > >> these two kinds of packets can be dispatched to different queues
> > >> according to their tuple field value.
> > >> For ipv4-tcp packets, src-ip, dst-ip, src-port, dst-port are used as
> > >> parameters of RSS hash algorithms to compute hash value.
> > >> For ipv4 packets src-ip, dst-ip are used.
> > >>
> > >> If rss_hf = RTE_ETH_RSS_NONFRAG_IPV4_TCP | RTE_ETH_RSS_L4_SRC_ONLY, for
> > >> ipv4-tcp packets, src-port is used as parameters of RSS hash algorithms
> > >> to compute hash value.
> > >
> > > I know what is rss_hf.
> > > My question is about the algorithms.
> > > Do they all support any combination in rss_hf?
> > >
> > >
> > I don't know about all vendors' hardware implementations,
> > so here's just my simple opinion.
> >
> > Theoretically, I think that all algorithms should support all
> > combinations in rss_hf, The reasons are as follows.
> 
> Leave it to the driver and hardware to decide what combinations can be
> supported.
> 
> >
> > 1. The rss_hf and algorithms are independent. For different algorithms
> > and the same packets and hash key, the input parameters for different
> > algorithms should be the same, which depends on implemetation of hardware.
> >
> > 2. As long as hardware and driver support, all packet types defined by
> > rss_hf could generate the the input parameters for the algorithm to compute.

How the application can know a hash is not supported with an algo?
I guess this is an error code on configure?