[v2,1/5] ethdev: support setting and querying RSS algorithm
Checks
Commit Message
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 is affected:
- rte_eth_dev_configure
- rte_eth_dev_rss_hash_update
- rte_eth_dev_rss_hash_conf_get
If the value of "func" used for configuration is a gibberish
value, report the error and return. Do the same for
rte_eth_dev_rss_hash_update and rte_eth_dev_configure.
To check whether the drivers report the "func" field, it is set
to default value before querying.
Signed-off-by: Jie Hai <haijie1@huawei.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
doc/guides/rel_notes/release_23_11.rst | 2 ++
lib/ethdev/rte_ethdev.c | 17 +++++++++++++++++
lib/ethdev/rte_ethdev.h | 6 ++++++
3 files changed, 25 insertions(+)
Comments
Hello,
Thanks for bringing a new capability.
26/08/2023 09:46, Jie Hai:
> 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 is affected:
> - rte_eth_dev_configure
> - rte_eth_dev_rss_hash_update
> - rte_eth_dev_rss_hash_conf_get
So far, the RSS algorithm was used only in flow RSS API.
> --- a/doc/guides/rel_notes/release_23_11.rst
> +++ b/doc/guides/rel_notes/release_23_11.rst
> @@ -123,6 +123,8 @@ ABI Changes
> Also, make sure to start the actual text at the margin.
> =======================================================
>
> + * ethdev: Added "func" field to ``rte_eth_rss_conf`` structure for RSS hash
> + algorithm.
As written above, it should start at the margin.
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> +#include "rte_flow.h"
It is strange to include rte_flow.h here.
It would be better to move the enum.
> + * 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.
> 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.
How the algorithms support combinations in rss_hf?
On Wed, Aug 30, 2023 at 4:46 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> Hello,
>
> Thanks for bringing a new capability.
>
> 26/08/2023 09:46, Jie Hai:
> > 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 is affected:
> > - rte_eth_dev_configure
> > - rte_eth_dev_rss_hash_update
> > - rte_eth_dev_rss_hash_conf_get
>
> So far, the RSS algorithm was used only in flow RSS API.
>
> > --- a/doc/guides/rel_notes/release_23_11.rst
> > +++ b/doc/guides/rel_notes/release_23_11.rst
> > @@ -123,6 +123,8 @@ ABI Changes
> > Also, make sure to start the actual text at the margin.
> > =======================================================
> >
> > + * ethdev: Added "func" field to ``rte_eth_rss_conf`` structure for RSS hash
> > + algorithm.
>
> As written above, it should start at the margin.
>
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > +#include "rte_flow.h"
>
> It is strange to include rte_flow.h here.
> It would be better to move the enum.
>
> > + * 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.
>
> > 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.
>
> How the algorithms support combinations in rss_hf?
I will spend a little more time on this tomorrow.
Can you update testpmd also to display the info as a part of show rss.
>
>
On 2023/8/31 8:10, Ajit Khaparde wrote:
> On Wed, Aug 30, 2023 at 4:46 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>> Hello,
>>
>> Thanks for bringing a new capability.
>>
>> 26/08/2023 09:46, Jie Hai:
>>> 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 is affected:
>>> - rte_eth_dev_configure
>>> - rte_eth_dev_rss_hash_update
>>> - rte_eth_dev_rss_hash_conf_get
>>
>> So far, the RSS algorithm was used only in flow RSS API.
>>
>>> --- a/doc/guides/rel_notes/release_23_11.rst
>>> +++ b/doc/guides/rel_notes/release_23_11.rst
>>> @@ -123,6 +123,8 @@ ABI Changes
>>> Also, make sure to start the actual text at the margin.
>>> =======================================================
>>>
>>> + * ethdev: Added "func" field to ``rte_eth_rss_conf`` structure for RSS hash
>>> + algorithm.
>>
>> As written above, it should start at the margin.
>>
>>> --- a/lib/ethdev/rte_ethdev.h
>>> +++ b/lib/ethdev/rte_ethdev.h
>>> +#include "rte_flow.h"
>>
>> It is strange to include rte_flow.h here.
>> It would be better to move the enum.
>>
>>> + * 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.
>>
>>> 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.
>>
>> How the algorithms support combinations in rss_hf?
> I will spend a little more time on this tomorrow.
> Can you update testpmd also to display the info as a part of show rss.
>
Hi, Ajit Khaparde,
Thanks for your advice. I will update testpmd in next version.
Thanks， Jie Hai
>>
>>
On 2023/8/31 8:10, Ajit Khaparde wrote:
> On Wed, Aug 30, 2023 at 4:46 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>> Hello,
>>
>> Thanks for bringing a new capability.
>>
>> 26/08/2023 09:46, Jie Hai:
>>> 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 is affected:
>>> - rte_eth_dev_configure
>>> - rte_eth_dev_rss_hash_update
>>> - rte_eth_dev_rss_hash_conf_get
>>
>> So far, the RSS algorithm was used only in flow RSS API.
>>
>>> --- a/doc/guides/rel_notes/release_23_11.rst
>>> +++ b/doc/guides/rel_notes/release_23_11.rst
>>> @@ -123,6 +123,8 @@ ABI Changes
>>> Also, make sure to start the actual text at the margin.
>>> =======================================================
>>>
>>> + * ethdev: Added "func" field to ``rte_eth_rss_conf`` structure for RSS hash
>>> + algorithm.
>>
>> As written above, it should start at the margin.
>>
>>> --- a/lib/ethdev/rte_ethdev.h
>>> +++ b/lib/ethdev/rte_ethdev.h
>>> +#include "rte_flow.h"
>>
>> It is strange to include rte_flow.h here.
>> It would be better to move the enum.
>>
>>> + * 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.
>>
>>> 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.
>>
>> How the algorithms support combinations in rss_hf?
> I will spend a little more time on this tomorrow.
> Can you update testpmd also to display the info as a part of show rss.
>
Hi, Ajit Khaparde,
Displaying RSS hash algorithms with testpmd is in progress.
However, there are some opinions on the implementation,
whether to add commands or display them in existing commands.
way 1: show port 0 rss-hash func
RSS algorithms:
symmetric_toeplitz
way 2: show port 0 rss-hash
RSS functions:
ipv4 ipv4-frag ipv4-other ipv6 ipv6-frag ipv6-other
RSS algorithms:
symmetric_toeplitz
I hope you can give some comments or suggestions.
Thanks,
Jie Hai
>>
>>
Hi, Thomas
Thanks for your review.
On 2023/8/30 19:46, Thomas Monjalon wrote:
> Hello,
>
> Thanks for bringing a new capability.
>
> 26/08/2023 09:46, Jie Hai:
>> 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 is affected:
>> - rte_eth_dev_configure
>> - rte_eth_dev_rss_hash_update
>> - rte_eth_dev_rss_hash_conf_get
>
> So far, the RSS algorithm was used only in flow RSS API.
>
Fixed in V3, these API will be affected.
>> --- a/doc/guides/rel_notes/release_23_11.rst
>> +++ b/doc/guides/rel_notes/release_23_11.rst
>> @@ -123,6 +123,8 @@ ABI Changes
>> Also, make sure to start the actual text at the margin.
>> =======================================================
>>
>> + * ethdev: Added "func" field to ``rte_eth_rss_conf`` structure for RSS hash
>> + algorithm.
>
> As written above, it should start at the margin.
>
Fixed in V3.
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> +#include "rte_flow.h"
>
> It is strange to include rte_flow.h here.
> It would be better to move the enum.
>
Fixed in V3, the definations are removed to rte_ethdev.h.
>> + * 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.
>> 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.
>
> .
Thanks,
Jie Hai
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.
> >> 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?
> >> How the algorithms support combinations in rss_hf?
> > I will spend a little more time on this tomorrow.
> > Can you update testpmd also to display the info as a part of show rss.
> >
> Hi, Ajit Khaparde,
>
> Displaying RSS hash algorithms with testpmd is in progress.
> However, there are some opinions on the implementation,
> whether to add commands or display them in existing commands.
>
> way 1: show port 0 rss-hash func
> RSS algorithms:
> symmetric_toeplitz
Option 1 looks good enough. Thanks
>
> way 2: show port 0 rss-hash
> RSS functions:
> ipv4 ipv4-frag ipv4-other ipv6 ipv6-frag ipv6-other
> RSS algorithms:
> symmetric_toeplitz
>
> I hope you can give some comments or suggestions.
>
> Thanks,
> Jie Hai
> >>
> >>
On Sat, 26 Aug 2023 15:46:03 +0800
Jie Hai <haijie1@huawei.com> 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 is affected:
> - rte_eth_dev_configure
> - rte_eth_dev_rss_hash_update
> - rte_eth_dev_rss_hash_conf_get
>
> If the value of "func" used for configuration is a gibberish
> value, report the error and return. Do the same for
> rte_eth_dev_rss_hash_update and rte_eth_dev_configure.
>
> To check whether the drivers report the "func" field, it is set
> to default value before querying.
>
> Signed-off-by: Jie Hai <haijie1@huawei.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
This is unannounced API/ABI change.
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.
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.
Thanks,
Jie Hai
> .
Hi, Stephen Hemminger
On 2023/9/6 23:10, Stephen Hemminger wrote:
> On Sat, 26 Aug 2023 15:46:03 +0800
> Jie Hai <haijie1@huawei.com> 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 is affected:
>> - rte_eth_dev_configure
>> - rte_eth_dev_rss_hash_update
>> - rte_eth_dev_rss_hash_conf_get
>>
>> If the value of "func" used for configuration is a gibberish
>> value, report the error and return. Do the same for
>> rte_eth_dev_rss_hash_update and rte_eth_dev_configure.
>>
>> To check whether the drivers report the "func" field, it is set
>> to default value before querying.
>>
>> Signed-off-by: Jie Hai <haijie1@huawei.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>
> This is unannounced API/ABI change.
Thanks for pointing that. I'm sorry I didn't pay attention to this before.
What should I do with this set of patches?
Is it feasible to send annouce now?
>
> .
Thanks,
Jie Hai
On Fri, 8 Sep 2023 17:28:08 +0800
Jie Hai <haijie1@huawei.com> wrote:
> Hi, Stephen Hemminger
>
> On 2023/9/6 23:10, Stephen Hemminger wrote:
> > On Sat, 26 Aug 2023 15:46:03 +0800
> > Jie Hai <haijie1@huawei.com> 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 is affected:
> >> - rte_eth_dev_configure
> >> - rte_eth_dev_rss_hash_update
> >> - rte_eth_dev_rss_hash_conf_get
> >>
> >> If the value of "func" used for configuration is a gibberish
> >> value, report the error and return. Do the same for
> >> rte_eth_dev_rss_hash_update and rte_eth_dev_configure.
> >>
> >> To check whether the drivers report the "func" field, it is set
> >> to default value before querying.
> >>
> >> Signed-off-by: Jie Hai <haijie1@huawei.com>
> >> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> >
> > This is unannounced API/ABI change.
> Thanks for pointing that. I'm sorry I didn't pay attention to this before.
> What should I do with this set of patches?
> Is it feasible to send annouce now?
Since this API/ABI breaking release, I am ok taking it. But will need a release
note. Also you should treat 0 as "no change" case to allow for code that does hash_update
without setting the value.
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.
>
> Thanks,
> Jie Hai
> > .
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?
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.
>
>
On 2023/9/9 4:58, Stephen Hemminger wrote:
> On Fri, 8 Sep 2023 17:28:08 +0800
> Jie Hai <haijie1@huawei.com> wrote:
>
>> Hi, Stephen Hemminger
>>
>> On 2023/9/6 23:10, Stephen Hemminger wrote:
>>> On Sat, 26 Aug 2023 15:46:03 +0800
>>> Jie Hai <haijie1@huawei.com> 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 is affected:
>>>> - rte_eth_dev_configure
>>>> - rte_eth_dev_rss_hash_update
>>>> - rte_eth_dev_rss_hash_conf_get
>>>>
>>>> If the value of "func" used for configuration is a gibberish
>>>> value, report the error and return. Do the same for
>>>> rte_eth_dev_rss_hash_update and rte_eth_dev_configure.
>>>>
>>>> To check whether the drivers report the "func" field, it is set
>>>> to default value before querying.
>>>>
>>>> Signed-off-by: Jie Hai <haijie1@huawei.com>
>>>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>>>
>>> This is unannounced API/ABI change.
>> Thanks for pointing that. I'm sorry I didn't pay attention to this before.
>> What should I do with this set of patches?
>> Is it feasible to send annouce now?
>
> Since this API/ABI breaking release, I am ok taking it. But will need a release
> note. Also you should treat 0 as "no change" case to allow for code that does hash_update
> without setting the value.
>
The release note 'doc/guides/rel_notes/release_23_11.rst' has been
updated in this patch. Please check it.
Whether or not to treat 0 as no change depends on the implemetation of
the driver.
According to the drivers that support algorithm modification through the
flow API, the 0 is regarded as no change.
It is believed that these drivers have the same behavior when updating
the hash_update with setting algorithm.
For the current drivers that do hash_update without setting the 'func'
value, the implemetation does not involve the modification of the
algorithm.
In either case, 0 is regarded as "no change".
>
> .
@@ -123,6 +123,8 @@ ABI Changes
Also, make sure to start the actual text at the margin.
=======================================================
+ * ethdev: Added "func" field to ``rte_eth_rss_conf`` structure for RSS hash
+ algorithm.
Known Issues
------------
@@ -1445,6 +1445,14 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
goto rollback;
}
+ if (dev_conf->rx_adv_conf.rss_conf.func >= RTE_ETH_HASH_FUNCTION_MAX) {
+ RTE_ETHDEV_LOG(ERR,
+ "Ethdev port_id=%u invalid rss hash function (%u)\n",
+ port_id, dev_conf->rx_adv_conf.rss_conf.func);
+ ret = -EINVAL;
+ goto rollback;
+ }
+
/* Check if Rx RSS distribution is disabled but RSS hash is enabled. */
if (((dev_conf->rxmode.mq_mode & RTE_ETH_MQ_RX_RSS_FLAG) == 0) &&
(dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_RSS_HASH)) {
@@ -4630,6 +4638,13 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
return -ENOTSUP;
}
+ if (rss_conf->func >= RTE_ETH_HASH_FUNCTION_MAX) {
+ RTE_ETHDEV_LOG(ERR,
+ "Ethdev port_id=%u invalid rss hash function (%u)\n",
+ port_id, rss_conf->func);
+ return -EINVAL;
+ }
+
if (*dev->dev_ops->rss_hash_update == NULL)
return -ENOTSUP;
ret = eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
@@ -4657,6 +4672,8 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
return -EINVAL;
}
+ rss_conf->func = RTE_ETH_HASH_FUNCTION_DEFAULT;
+
if (*dev->dev_ops->rss_hash_conf_get == NULL)
return -ENOTSUP;
ret = eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
@@ -174,6 +174,7 @@ extern "C" {
#include "rte_ethdev_trace_fp.h"
#include "rte_dev_info.h"
+#include "rte_flow.h"
extern int rte_eth_dev_logtype;
@@ -461,11 +462,16 @@ struct rte_vlan_filter_conf {
* The *rss_hf* field of the *rss_conf* structure indicates the different
* types of IPv4/IPv6 packets to which the RSS hashing must be applied.
* Supplying an *rss_hf* equal to zero disables the RSS feature.
+ *
+ * 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.
*/
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. */
};
/*