DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>
To: "Lukas Bartosik [C]" <lbartosik@marvell.com>
Cc: "konstantin.ananyev@intel.com" <konstantin.ananyev@intel.com>,
	"akhil.goyal@nxp.com" <akhil.goyal@nxp.com>,
	Anoob Joseph <anoobj@marvell.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3] ipsec: use hash lookup with hash sigs in sad lookup
Date: Tue, 21 Apr 2020 15:25:18 +0100	[thread overview]
Message-ID: <a5673123-c6bf-9a7a-d26d-5a753ff2b499@intel.com> (raw)
In-Reply-To: <347a1212-f3cb-4aa9-e551-c7779b7ac17c@marvell.com>

Hi Lukas,

http://patches.dpdk.org/patch/66715/ is suspended due to new hash library it relates to will not be applied in 20.05.
This patch replaces lookup in __ipsec_sad_lookup only for SPI_ONLY table.
So yes, in the future both solutions will coexist (new rte_dwk_hash_lookup for SPI_ONLY and rte_hash_lookup_with_hash_bulk_data
  for SPI_DIP/SPI_DIP_SIP).

On 21/04/2020 14:28, Lukas Bartosik [C] wrote:
> Hi Vladimir,
>
> How does this patch relate to http://patches.dpdk.org/patch/66715/.
> Both patches replace lookup function rte_hash_lookup_bulk_data used in __ipsec_sad_lookup with
> rte_hash_lookup_with_hash_bulk_data or rte_dwk_hash_lookup.
>
> Is the plan for both solutions to coexist or will only one be selected ?
>
> Thanks,
> Lukasz
>
> On 20.04.2020 20:27, Vladimir Medvedkin wrote:
>> Change hash function from jhash to crc.
>> Precalculate hash signatures for a bulk of keys and then
>> use rte_hash_lookup_with_hash_bulk_data() to speed up sad lookup
>> Also use rte_hash_add_key_with_hash and _del_key_with_hash with
>> precalculated hash signature for a key in rte_ipsec_sad_add and
>> rte_ipsec_sad_del
>>
>> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> ---
>> This patch depends on https://urldefense.proofpoint.com/v2/url?u=https-3A__patches.dpdk.org_patch_68700_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=SchRHhE7GLCjEY4i2a1byjC_FpWgRLtq4-kLvKp3_84&m=NIgAtdq8iGRrs8haKYwwa0eyciO0TMFWnUYisMoGD5o&s=IpnJ270RsaE8LIU06649hZvkyKeAaTwamVbIMX9AyUY&e=
>>
>>   lib/librte_ipsec/ipsec_sad.c | 106 ++++++++++++++++++++++++++++++-------------
>>   1 file changed, 74 insertions(+), 32 deletions(-)
>>
>> diff --git a/lib/librte_ipsec/ipsec_sad.c b/lib/librte_ipsec/ipsec_sad.c
>> index 2c994ed..6c95240 100644
>> --- a/lib/librte_ipsec/ipsec_sad.c
>> +++ b/lib/librte_ipsec/ipsec_sad.c
>> @@ -2,10 +2,12 @@
>>    * Copyright(c) 2019 Intel Corporation
>>    */
>>   
>> +#include <string.h>
>> +
>>   #include <rte_eal_memconfig.h>
>>   #include <rte_errno.h>
>>   #include <rte_hash.h>
>> -#include <rte_jhash.h>
>> +#include <rte_hash_crc.h>
>>   #include <rte_malloc.h>
>>   #include <rte_random.h>
>>   #include <rte_rwlock.h>
>> @@ -24,7 +26,7 @@
>>   /* "SAD_<name>" */
>>   #define SAD_FORMAT		SAD_PREFIX "%s"
>>   
>> -#define DEFAULT_HASH_FUNC	rte_jhash
>> +#define DEFAULT_HASH_FUNC	rte_hash_crc
>>   #define MIN_HASH_ENTRIES	8U /* From rte_cuckoo_hash.h */
>>   
>>   struct hash_cnt {
>> @@ -35,6 +37,8 @@ struct hash_cnt {
>>   struct rte_ipsec_sad {
>>   	char name[RTE_IPSEC_SAD_NAMESIZE];
>>   	struct rte_hash	*hash[RTE_IPSEC_SAD_KEY_TYPE_MASK];
>> +	uint32_t keysize[RTE_IPSEC_SAD_KEY_TYPE_MASK];
>> +	uint32_t init_val;
>>   	/* Array to track number of more specific rules
>>   	 * (spi_dip or spi_dip_sip). Used only in add/delete
>>   	 * as a helper struct.
>> @@ -70,29 +74,36 @@ add_specific(struct rte_ipsec_sad *sad, const void *key,
>>   	/* Check if the key is present in the table.
>>   	 * Need for further accaunting in cnt_arr
>>   	 */
>> -	ret = rte_hash_lookup(sad->hash[key_type], key);
>> +	ret = rte_hash_lookup_with_hash(sad->hash[key_type], key,
>> +		rte_hash_crc(key, sad->keysize[key_type], sad->init_val));
>>   	notexist = (ret == -ENOENT);
>>   
>>   	/* Add an SA to the corresponding table.*/
>> -	ret = rte_hash_add_key_data(sad->hash[key_type], key, sa);
>> +	ret = rte_hash_add_key_with_hash_data(sad->hash[key_type], key,
>> +		rte_hash_crc(key, sad->keysize[key_type], sad->init_val), sa);
>>   	if (ret != 0)
>>   		return ret;
>>   
>>   	/* Check if there is an entry in SPI only table with the same SPI */
>> -	ret = rte_hash_lookup_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY],
>> -		key, &tmp_val);
>> +	ret = rte_hash_lookup_with_hash_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY],
>> +		key, rte_hash_crc(key, sad->keysize[RTE_IPSEC_SAD_SPI_ONLY],
>> +		sad->init_val), &tmp_val);
>>   	if (ret < 0)
>>   		tmp_val = NULL;
>>   	tmp_val = SET_BIT(tmp_val, key_type);
>>   
>>   	/* Add an entry into SPI only table */
>> -	ret = rte_hash_add_key_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY],
>> -		key, tmp_val);
>> +	ret = rte_hash_add_key_with_hash_data(
>> +		sad->hash[RTE_IPSEC_SAD_SPI_ONLY], key,
>> +		rte_hash_crc(key, sad->keysize[RTE_IPSEC_SAD_SPI_ONLY],
>> +		sad->init_val), tmp_val);
>>   	if (ret != 0)
>>   		return ret;
>>   
>>   	/* Update a counter for a given SPI */
>> -	ret = rte_hash_lookup(sad->hash[RTE_IPSEC_SAD_SPI_ONLY], key);
>> +	ret = rte_hash_lookup_with_hash(sad->hash[RTE_IPSEC_SAD_SPI_ONLY], key,
>> +		rte_hash_crc(key, sad->keysize[RTE_IPSEC_SAD_SPI_ONLY],
>> +		sad->init_val));
>>   	if (key_type == RTE_IPSEC_SAD_SPI_DIP)
>>   		sad->cnt_arr[ret].cnt_dip += notexist;
>>   	else
>> @@ -122,15 +133,17 @@ rte_ipsec_sad_add(struct rte_ipsec_sad *sad,
>>   	 */
>>   	switch (key_type) {
>>   	case(RTE_IPSEC_SAD_SPI_ONLY):
>> -		ret = rte_hash_lookup_data(sad->hash[key_type],
>> -			key, &tmp_val);
>> +		ret = rte_hash_lookup_with_hash_data(sad->hash[key_type],
>> +			key, rte_hash_crc(key, sad->keysize[key_type],
>> +			sad->init_val), &tmp_val);
>>   		if (ret >= 0)
>>   			tmp_val = SET_BIT(sa, GET_BIT(tmp_val,
>>   				RTE_IPSEC_SAD_KEY_TYPE_MASK));
>>   		else
>>   			tmp_val = sa;
>> -		ret = rte_hash_add_key_data(sad->hash[key_type],
>> -			key, tmp_val);
>> +		ret = rte_hash_add_key_with_hash_data(sad->hash[key_type],
>> +			key, rte_hash_crc(key, sad->keysize[key_type],
>> +			sad->init_val), tmp_val);
>>   		return ret;
>>   	case(RTE_IPSEC_SAD_SPI_DIP):
>>   	case(RTE_IPSEC_SAD_SPI_DIP_SIP):
>> @@ -158,13 +171,15 @@ del_specific(struct rte_ipsec_sad *sad, const void *key, int key_type)
>>   	uint32_t *cnt;
>>   
>>   	/* Remove an SA from the corresponding table.*/
>> -	ret = rte_hash_del_key(sad->hash[key_type], key);
>> +	ret = rte_hash_del_key_with_hash(sad->hash[key_type], key,
>> +		rte_hash_crc(key, sad->keysize[key_type], sad->init_val));
>>   	if (ret < 0)
>>   		return ret;
>>   
>>   	/* Get an index of cnt_arr entry for a given SPI */
>> -	ret = rte_hash_lookup_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY],
>> -		key, &tmp_val);
>> +	ret = rte_hash_lookup_with_hash_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY],
>> +		key, rte_hash_crc(key, sad->keysize[RTE_IPSEC_SAD_SPI_ONLY],
>> +		sad->init_val), &tmp_val);
>>   	if (ret < 0)
>>   		return ret;
>>   	cnt = (key_type == RTE_IPSEC_SAD_SPI_DIP) ?
>> @@ -182,10 +197,15 @@ del_specific(struct rte_ipsec_sad *sad, const void *key, int key_type)
>>   	 * remove an entry from SPI_only table
>>   	 */
>>   	if (tmp_val == NULL)
>> -		ret = rte_hash_del_key(sad->hash[RTE_IPSEC_SAD_SPI_ONLY], key);
>> +		ret = rte_hash_del_key_with_hash(
>> +			sad->hash[RTE_IPSEC_SAD_SPI_ONLY], key,
>> +			rte_hash_crc(key, sad->keysize[RTE_IPSEC_SAD_SPI_ONLY],
>> +			sad->init_val));
>>   	else
>> -		ret = rte_hash_add_key_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY],
>> -			key, tmp_val);
>> +		ret = rte_hash_add_key_with_hash_data(
>> +			sad->hash[RTE_IPSEC_SAD_SPI_ONLY], key,
>> +			rte_hash_crc(key, sad->keysize[RTE_IPSEC_SAD_SPI_ONLY],
>> +			sad->init_val), tmp_val);
>>   	if (ret < 0)
>>   		return ret;
>>   	return 0;
>> @@ -203,19 +223,23 @@ rte_ipsec_sad_del(struct rte_ipsec_sad *sad,
>>   		return -EINVAL;
>>   	switch (key_type) {
>>   	case(RTE_IPSEC_SAD_SPI_ONLY):
>> -		ret = rte_hash_lookup_data(sad->hash[key_type],
>> -			key, &tmp_val);
>> +		ret = rte_hash_lookup_with_hash_data(sad->hash[key_type],
>> +			key, rte_hash_crc(key, sad->keysize[key_type],
>> +			sad->init_val), &tmp_val);
>>   		if (ret < 0)
>>   			return ret;
>>   		if (GET_BIT(tmp_val, RTE_IPSEC_SAD_KEY_TYPE_MASK) == 0) {
>> -			ret = rte_hash_del_key(sad->hash[key_type],
>> -				key);
>> +			ret = rte_hash_del_key_with_hash(sad->hash[key_type],
>> +				key, rte_hash_crc(key, sad->keysize[key_type],
>> +				sad->init_val));
>>   			ret = ret < 0 ? ret : 0;
>>   		} else {
>>   			tmp_val = GET_BIT(tmp_val,
>>   				RTE_IPSEC_SAD_KEY_TYPE_MASK);
>> -			ret = rte_hash_add_key_data(sad->hash[key_type],
>> -				key, tmp_val);
>> +			ret = rte_hash_add_key_with_hash_data(
>> +				sad->hash[key_type], key,
>> +				rte_hash_crc(key, sad->keysize[key_type],
>> +				sad->init_val), tmp_val);
>>   		}
>>   		return ret;
>>   	case(RTE_IPSEC_SAD_SPI_DIP):
>> @@ -272,6 +296,7 @@ rte_ipsec_sad_create(const char *name, const struct rte_ipsec_sad_conf *conf)
>>   
>>   	hash_params.hash_func = DEFAULT_HASH_FUNC;
>>   	hash_params.hash_func_init_val = rte_rand();
>> +	sad->init_val = hash_params.hash_func_init_val;
>>   	hash_params.socket_id = conf->socket_id;
>>   	hash_params.name = hash_name;
>>   	if (conf->flags & RTE_IPSEC_SAD_FLAG_RW_CONCURRENCY)
>> @@ -280,6 +305,7 @@ rte_ipsec_sad_create(const char *name, const struct rte_ipsec_sad_conf *conf)
>>   	/** Init hash[RTE_IPSEC_SAD_SPI_ONLY] for SPI only */
>>   	snprintf(hash_name, sizeof(hash_name), "sad_1_%p", sad);
>>   	hash_params.key_len = sizeof(((struct rte_ipsec_sadv4_key *)0)->spi);
>> +	sad->keysize[RTE_IPSEC_SAD_SPI_ONLY] = hash_params.key_len;
>>   	hash_params.entries = sa_sum;
>>   	sad->hash[RTE_IPSEC_SAD_SPI_ONLY] = rte_hash_create(&hash_params);
>>   	if (sad->hash[RTE_IPSEC_SAD_SPI_ONLY] == NULL) {
>> @@ -295,6 +321,7 @@ rte_ipsec_sad_create(const char *name, const struct rte_ipsec_sad_conf *conf)
>>   	else
>>   		hash_params.key_len +=
>>   			sizeof(((struct rte_ipsec_sadv4_key *)0)->dip);
>> +	sad->keysize[RTE_IPSEC_SAD_SPI_DIP] = hash_params.key_len;
>>   	hash_params.entries = RTE_MAX(MIN_HASH_ENTRIES,
>>   			conf->max_sa[RTE_IPSEC_SAD_SPI_DIP]);
>>   	sad->hash[RTE_IPSEC_SAD_SPI_DIP] = rte_hash_create(&hash_params);
>> @@ -311,6 +338,7 @@ rte_ipsec_sad_create(const char *name, const struct rte_ipsec_sad_conf *conf)
>>   	else
>>   		hash_params.key_len +=
>>   			sizeof(((struct rte_ipsec_sadv4_key *)0)->sip);
>> +	sad->keysize[RTE_IPSEC_SAD_SPI_DIP_SIP] = hash_params.key_len;
>>   	hash_params.entries = RTE_MAX(MIN_HASH_ENTRIES,
>>   			conf->max_sa[RTE_IPSEC_SAD_SPI_DIP_SIP]);
>>   	sad->hash[RTE_IPSEC_SAD_SPI_DIP_SIP] = rte_hash_create(&hash_params);
>> @@ -440,15 +468,21 @@ __ipsec_sad_lookup(const struct rte_ipsec_sad *sad,
>>   	uint32_t n_3 = 0;
>>   	uint32_t i;
>>   	int found = 0;
>> +	hash_sig_t hash_sig[RTE_HASH_LOOKUP_BULK_MAX];
>> +	hash_sig_t hash_sig_2[RTE_HASH_LOOKUP_BULK_MAX];
>> +	hash_sig_t hash_sig_3[RTE_HASH_LOOKUP_BULK_MAX];
>>   
>> -	for (i = 0; i < n; i++)
>> +	for (i = 0; i < n; i++) {
>>   		sa[i] = NULL;
>> +		hash_sig[i] = rte_hash_crc_4byte(keys[i]->v4.spi,
>> +			sad->init_val);
>> +	}
>>   
>>   	/*
>>   	 * Lookup keys in SPI only hash table first.
>>   	 */
>> -	rte_hash_lookup_bulk_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY],
>> -		(const void **)keys, n, &mask_1, sa);
>> +	rte_hash_lookup_with_hash_bulk_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY],
>> +		(const void **)keys, hash_sig, n, &mask_1, sa);
>>   	for (map = mask_1; map; map &= (map - 1)) {
>>   		i = rte_bsf64(map);
>>   		/*
>> @@ -457,10 +491,16 @@ __ipsec_sad_lookup(const struct rte_ipsec_sad *sad,
>>   		 */
>>   		if ((uintptr_t)sa[i] & RTE_IPSEC_SAD_SPI_DIP_SIP) {
>>   			idx_3[n_3] = i;
>> +			hash_sig_3[n_3] = rte_hash_crc(keys[i],
>> +				sad->keysize[RTE_IPSEC_SAD_SPI_DIP_SIP],
>> +				sad->init_val);
>>   			keys_3[n_3++] = keys[i];
>>   		}
>>   		if ((uintptr_t)sa[i] & RTE_IPSEC_SAD_SPI_DIP) {
>>   			idx_2[n_2] = i;
>> +			hash_sig_2[n_2] = rte_hash_crc(keys[i],
>> +				sad->keysize[RTE_IPSEC_SAD_SPI_DIP],
>> +				sad->init_val);
>>   			keys_2[n_2++] = keys[i];
>>   		}
>>   		/* clear 2 LSB's which indicate the presence
>> @@ -471,8 +511,9 @@ __ipsec_sad_lookup(const struct rte_ipsec_sad *sad,
>>   
>>   	/* Lookup for more specific rules in SPI_DIP table */
>>   	if (n_2 != 0) {
>> -		rte_hash_lookup_bulk_data(sad->hash[RTE_IPSEC_SAD_SPI_DIP],
>> -			keys_2, n_2, &mask_2, vals_2);
>> +		rte_hash_lookup_with_hash_bulk_data(
>> +			sad->hash[RTE_IPSEC_SAD_SPI_DIP],
>> +			keys_2, hash_sig_2, n_2, &mask_2, vals_2);
>>   		for (map_spec = mask_2; map_spec; map_spec &= (map_spec - 1)) {
>>   			i = rte_bsf64(map_spec);
>>   			sa[idx_2[i]] = vals_2[i];
>> @@ -480,8 +521,9 @@ __ipsec_sad_lookup(const struct rte_ipsec_sad *sad,
>>   	}
>>   	/* Lookup for more specific rules in SPI_DIP_SIP table */
>>   	if (n_3 != 0) {
>> -		rte_hash_lookup_bulk_data(sad->hash[RTE_IPSEC_SAD_SPI_DIP_SIP],
>> -			keys_3, n_3, &mask_3, vals_3);
>> +		rte_hash_lookup_with_hash_bulk_data(
>> +			sad->hash[RTE_IPSEC_SAD_SPI_DIP_SIP],
>> +			keys_3, hash_sig_3, n_3, &mask_3, vals_3);
>>   		for (map_spec = mask_3; map_spec; map_spec &= (map_spec - 1)) {
>>   			i = rte_bsf64(map_spec);
>>   			sa[idx_3[i]] = vals_3[i];

-- 
Regards,
Vladimir


  reply	other threads:[~2020-04-21 14:25 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-09 12:55 [dpdk-dev] [PATCH] " Vladimir Medvedkin
2020-03-24 19:57 ` Ananyev, Konstantin
2020-03-26 12:54   ` Medvedkin, Vladimir
2020-03-26 16:45 ` [dpdk-dev] [PATCH v2 1/2] " Vladimir Medvedkin
2020-04-07 12:58   ` Ananyev, Konstantin
2020-04-20 18:27   ` [dpdk-dev] [PATCH v3] " Vladimir Medvedkin
2020-04-21 13:28     ` Lukas Bartosik [C]
2020-04-21 14:25       ` Medvedkin, Vladimir [this message]
2020-04-25 13:55     ` Thomas Monjalon
2020-03-26 16:45 ` [dpdk-dev] [PATCH v2 2/2] ipsec: use hash add and delete with precalculated signatures Vladimir Medvedkin
2020-04-07 12:59   ` Ananyev, Konstantin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a5673123-c6bf-9a7a-d26d-5a753ff2b499@intel.com \
    --to=vladimir.medvedkin@intel.com \
    --cc=akhil.goyal@nxp.com \
    --cc=anoobj@marvell.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --cc=lbartosik@marvell.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).