[RFC] ethdev: support symmetric hash function
Checks
Commit Message
From: Simei Su <simei.su@intel.com>
Currently, there are DEFAULT,TOEPLITZ and SIMPLE_XOR hash funtion.
To support symmetric hash by rte_flow RSS action, this RFC introduces
SYMMETRIC_TOEPLITZ to rte_eth_hash_function.
Signed-off-by: Simei Su <simei.su@intel.com>
---
lib/librte_ethdev/rte_flow.h | 1 +
1 file changed, 1 insertion(+)
Comments
On Thu, Jul 04, 2019 at 12:46:07PM +0800, simei wrote:
> From: Simei Su <simei.su@intel.com>
>
> Currently, there are DEFAULT,TOEPLITZ and SIMPLE_XOR hash funtion.
> To support symmetric hash by rte_flow RSS action, this RFC introduces
> SYMMETRIC_TOEPLITZ to rte_eth_hash_function.
>
> Signed-off-by: Simei Su <simei.su@intel.com>
> ---
> lib/librte_ethdev/rte_flow.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index f3a8fb1..e3c4fe5 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1744,6 +1744,7 @@ enum rte_eth_hash_function {
> RTE_ETH_HASH_FUNCTION_DEFAULT = 0,
> RTE_ETH_HASH_FUNCTION_TOEPLITZ, /**< Toeplitz */
> RTE_ETH_HASH_FUNCTION_SIMPLE_XOR, /**< Simple XOR */
> + RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ, /**< Symmetric TOEPLITZ */
"Symmetric TOEPLITZ" => "Symmetric Toeplitz."
> RTE_ETH_HASH_FUNCTION_MAX,
> };
Other than that, no problem with this change (no ABI impact, no need for
deprecation). Please update testpmd a part of the same patch:
- Wherever "toeplitz" is mentioned in test-pmd/cmdline.c.
- Ditto for flow command, i.e. add ACTION_RSS_FUNC_SYMMETRIC_TOEPLITZ to
test-pmd/cmdline_flow.c.
- Update "set_hash_global_config" documentation section in
testpmd_app_ug/testpmd_funcs.rst.
Note to Shahaf/Yongseok, since mlx5 supports both but defaults to symmetric
Toeplitz on vanilla Linux and standard Toeplitz when using OFED, how about
using this chance to make the algorithm configurable as well?
Thanks.
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, July 4, 2019 5:07 PM
> To: Su, Simei <simei.su@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Yongseok Koh
> <yskoh@mellanox.com>
> Subject: Re: [dpdk-dev] [RFC] ethdev: support symmetric hash function
>
> On Thu, Jul 04, 2019 at 12:46:07PM +0800, simei wrote:
> > From: Simei Su <simei.su@intel.com>
> >
> > Currently, there are DEFAULT,TOEPLITZ and SIMPLE_XOR hash funtion.
> > To support symmetric hash by rte_flow RSS action, this RFC introduces
> > SYMMETRIC_TOEPLITZ to rte_eth_hash_function.
> >
> > Signed-off-by: Simei Su <simei.su@intel.com>
> > ---
> > lib/librte_ethdev/rte_flow.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index f3a8fb1..e3c4fe5 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -1744,6 +1744,7 @@ enum rte_eth_hash_function {
> > RTE_ETH_HASH_FUNCTION_DEFAULT = 0,
> > RTE_ETH_HASH_FUNCTION_TOEPLITZ, /**< Toeplitz */
> > RTE_ETH_HASH_FUNCTION_SIMPLE_XOR, /**< Simple XOR */
> > + RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ, /**< Symmetric
> TOEPLITZ */
>
> "Symmetric TOEPLITZ" => "Symmetric Toeplitz."
>
> > RTE_ETH_HASH_FUNCTION_MAX,
> > };
>
> Other than that, no problem with this change (no ABI impact, no need for
> deprecation). Please update testpmd a part of the same patch:
Is it still ABI break but just with little risk?
RTE_ETH_HASH_FUNCTION_MAX's value is changed anyway.
Should we just remove it, if no one use it?
Regards
Qi
>
> - Wherever "toeplitz" is mentioned in test-pmd/cmdline.c.
>
> - Ditto for flow command, i.e. add ACTION_RSS_FUNC_SYMMETRIC_TOEPLITZ
> to
> test-pmd/cmdline_flow.c.
>
> - Update "set_hash_global_config" documentation section in
> testpmd_app_ug/testpmd_funcs.rst.
>
> Note to Shahaf/Yongseok, since mlx5 supports both but defaults to symmetric
> Toeplitz on vanilla Linux and standard Toeplitz when using OFED, how about
> using this chance to make the algorithm configurable as well?
>
> Thanks.
>
> --
> Adrien Mazarguil
> 6WIND
On Thu, Jul 04, 2019 at 12:48:25PM +0000, Zhang, Qi Z wrote:
>
>
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Thursday, July 4, 2019 5:07 PM
> > To: Su, Simei <simei.su@intel.com>
> > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> > Xing, Beilei <beilei.xing@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> > dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Yongseok Koh
> > <yskoh@mellanox.com>
> > Subject: Re: [dpdk-dev] [RFC] ethdev: support symmetric hash function
> >
> > On Thu, Jul 04, 2019 at 12:46:07PM +0800, simei wrote:
> > > From: Simei Su <simei.su@intel.com>
> > >
> > > Currently, there are DEFAULT,TOEPLITZ and SIMPLE_XOR hash funtion.
> > > To support symmetric hash by rte_flow RSS action, this RFC introduces
> > > SYMMETRIC_TOEPLITZ to rte_eth_hash_function.
> > >
> > > Signed-off-by: Simei Su <simei.su@intel.com>
> > > ---
> > > lib/librte_ethdev/rte_flow.h | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > b/lib/librte_ethdev/rte_flow.h index f3a8fb1..e3c4fe5 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -1744,6 +1744,7 @@ enum rte_eth_hash_function {
> > > RTE_ETH_HASH_FUNCTION_DEFAULT = 0,
> > > RTE_ETH_HASH_FUNCTION_TOEPLITZ, /**< Toeplitz */
> > > RTE_ETH_HASH_FUNCTION_SIMPLE_XOR, /**< Simple XOR */
> > > + RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ, /**< Symmetric
> > TOEPLITZ */
> >
> > "Symmetric TOEPLITZ" => "Symmetric Toeplitz."
> >
> > > RTE_ETH_HASH_FUNCTION_MAX,
> > > };
> >
> > Other than that, no problem with this change (no ABI impact, no need for
> > deprecation). Please update testpmd a part of the same patch:
>
> Is it still ABI break but just with little risk?
> RTE_ETH_HASH_FUNCTION_MAX's value is changed anyway.
> Should we just remove it, if no one use it?
Indeed, it will update RTE_ETH_HASH_FUNCTION_MAX so you're technically
right, and the fact it's unused in DPDK is doesn't mean applications are not
using it for something.
However for this specific case, the intent behind RTE_ETH_HASH_FUNCTION_MAX
is clearly to give out the number of enum entries, applications are not
supposed to use it for anything other than determining if an arbitrary
integer value corresponds to a valid hash function.
And this is the reason we could say it's OK ABI-wise to increase it (not
ideal but acceptable): a binary application has a fixed idea of
RTE_ETH_HASH_FUNCTION_MAX, it doesn't know the entries you're about to add
yet. To such an application, those will exceed RTE_ETH_HASH_FUNCTION_MAX and
should be rejected accordingly.
A more conservative approach would be to mark RTE_ETH_HASH_FUNCTION_MAX as
deprecated (in a separate patch) and schedule it for removal while adding
new entries after it. Its position in the enum could be recycled once
removed.
If you want to remove RTE_ETH_HASH_FUNCTION_MAX directly, do it in a
separate RFC/patch as it will otherwise block the rest of your submission
for something like 2 releases after deprecation.
It's up to you. I'm fine with any of these approaches.
@@ -1744,6 +1744,7 @@ enum rte_eth_hash_function {
RTE_ETH_HASH_FUNCTION_DEFAULT = 0,
RTE_ETH_HASH_FUNCTION_TOEPLITZ, /**< Toeplitz */
RTE_ETH_HASH_FUNCTION_SIMPLE_XOR, /**< Simple XOR */
+ RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ, /**< Symmetric TOEPLITZ */
RTE_ETH_HASH_FUNCTION_MAX,
};