DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: "Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: "akhil.goyal@nxp.com" <akhil.goyal@nxp.com>
Subject: Re: [dpdk-dev] [PATCH] ipsec: use hash lookup with hash sigs in sad lookup
Date: Tue, 24 Mar 2020 19:57:35 +0000	[thread overview]
Message-ID: <SN6PR11MB255864E60991F4DC95404C6F9AF10@SN6PR11MB2558.namprd11.prod.outlook.com> (raw)
In-Reply-To: <1583758545-375405-1-git-send-email-vladimir.medvedkin@intel.com>

Hi Vladimir,

> 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

Looks good in general.
Few thoughts below, nothing major.
 
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
> ---
> This patch depends on https://patches.dpdk.org/patch/66460/
> 
>  lib/librte_ipsec/ipsec_sad.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/librte_ipsec/ipsec_sad.c b/lib/librte_ipsec/ipsec_sad.c
> index 2c994ed..3292968 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,9 @@ 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 spi_dip_keysize;
> +	uint32_t spi_dip_sip_keysize;

Pure stylish thing: might be
uint32_t keysize[RTE_IPSEC_SAD_KEY_TYPE_MASK];
here?

> +	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.
> @@ -272,6 +277,7 @@ rte_ipsec_sad_create(const char *name, const struct rte_ipsec_sad_conf *conf)
> 
>  	hash_params.hash_func = DEFAULT_HASH_FUNC;

If we doing lookup_with_hash now, might be it makes sense to
do add_with_hash and del_with_hash too?
For code consistency.

>  	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)
> @@ -295,6 +301,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->spi_dip_keysize = 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 +318,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->spi_dip_sip_keysize = 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 +448,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 +471,14 @@ __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->spi_dip_sip_keysize, 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->spi_dip_keysize, sad->init_val);
>  			keys_2[n_2++] = keys[i];
>  		}
>  		/* clear 2 LSB's which indicate the presence
> @@ -471,8 +489,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 +499,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];
> --
> 2.7.4


  reply	other threads:[~2020-03-24 19:57 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-09 12:55 Vladimir Medvedkin
2020-03-24 19:57 ` Ananyev, Konstantin [this message]
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
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=SN6PR11MB255864E60991F4DC95404C6F9AF10@SN6PR11MB2558.namprd11.prod.outlook.com \
    --to=konstantin.ananyev@intel.com \
    --cc=akhil.goyal@nxp.com \
    --cc=dev@dpdk.org \
    --cc=vladimir.medvedkin@intel.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).