On Thu, Jul 04, 2019 at 12:48:25PM +0000, Zhang, Qi Z wrote: > > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > > Sent: Thursday, July 4, 2019 5:07 PM > > To: Su, Simei <simei...@intel.com> > > Cc: Zhang, Qi Z <qi.z.zh...@intel.com>; Wu, Jingjing > > <jingjing...@intel.com>; > > Xing, Beilei <beilei.x...@intel.com>; Yang, Qiming <qiming.y...@intel.com>; > > dev@dpdk.org; Shahaf Shuler <shah...@mellanox.com>; Yongseok Koh > > <ys...@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...@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...@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
