On Mon, Sep 11, 2023 at 3:09 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 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?
Yes. We may need a way to differentiate errors because of RSS vs others.

>
>