Re: [dpdk-dev] [RFC] ethdev: support symmetric hash function

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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.

-- 
Adrien Mazarguil
6WIND