From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 306B1A2EEB for ; Thu, 12 Sep 2019 19:58:51 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A70ED1EFD2; Thu, 12 Sep 2019 19:58:50 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 9C2781EFE1 for ; Thu, 12 Sep 2019 19:58:48 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Sep 2019 10:58:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,492,1559545200"; d="scan'208";a="269164213" Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by orsmga001.jf.intel.com with ESMTP; 12 Sep 2019 10:58:46 -0700 Received: from irsmsx155.ger.corp.intel.com (163.33.192.3) by irsmsx105.ger.corp.intel.com (163.33.3.28) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 12 Sep 2019 18:58:45 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.164]) by irsmsx155.ger.corp.intel.com ([169.254.14.139]) with mapi id 14.03.0439.000; Thu, 12 Sep 2019 18:58:45 +0100 From: "Ananyev, Konstantin" To: "Medvedkin, Vladimir" , "dev@dpdk.org" CC: "Iremonger, Bernard" , "akhil.goyal@nxp.com" Thread-Topic: [PATCH v1 3/5] ipsec: add SAD add/delete/lookup implementation Thread-Index: AQHVYniEwHREyesyh067j8EXcazm7qcoWwsQ Date: Thu, 12 Sep 2019 17:58:44 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580191964A5F@irsmsx105.ger.corp.intel.com> References: <1565709186-273340-1-git-send-email-vladimir.medvedkin@intel.com> <35869a61ec4294e0c991eb145c385b05f2db1e0d.1567529480.git.vladimir.medvedkin@intel.com> In-Reply-To: <35869a61ec4294e0c991eb145c385b05f2db1e0d.1567529480.git.vladimir.medvedkin@intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNDhmYzhkNGYtYjEwZS00ZWRhLTgwMTUtZTU5NDU2NzFjMWE3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiYmZQQXFKRzFHK1VwUFpCSWxRNk42WGJrRm1OYU54Mjd6c3h2bFMwOW85REt2VkMyVGhkaGlWZUJkamJiUjhBMSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v1 3/5] ipsec: add SAD add/delete/lookup implementation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Vladimir, >=20 > Replace rte_ipsec_sad_add(), rte_ipsec_sad_del() and > rte_ipsec_sad_lookup() stubs with actual implementation. >=20 > It uses three librte_hash tables each of which contains > an entries for a specific SA type (either it is addressed by SPI only > or SPI+DIP or SPI+DIP+SIP) >=20 > Signed-off-by: Vladimir Medvedkin > --- > lib/librte_ipsec/ipsec_sad.c | 245 +++++++++++++++++++++++++++++++++++++= +++--- > 1 file changed, 233 insertions(+), 12 deletions(-) >=20 > diff --git a/lib/librte_ipsec/ipsec_sad.c b/lib/librte_ipsec/ipsec_sad.c > index 7797628..4bf2206 100644 > --- a/lib/librte_ipsec/ipsec_sad.c > +++ b/lib/librte_ipsec/ipsec_sad.c > @@ -13,6 +13,13 @@ >=20 > #include "rte_ipsec_sad.h" >=20 > +/* > + * Rules are stored in three hash tables depending on key_type. > + * Each rule will also be stored in SPI_ONLY table. > + * for each data entry within this table last two bits are reserved to > + * indicate presence of entries with the same SPI in DIP and DIP+SIP tab= les. > + */ > + > #define IPSEC_SAD_NAMESIZE 64 > #define SAD_PREFIX "SAD_" > /* "SAD_" */ > @@ -37,20 +44,158 @@ static struct rte_tailq_elem rte_ipsec_sad_tailq =3D= { > }; > EAL_REGISTER_TAILQ(rte_ipsec_sad_tailq) >=20 > +#define SET_BIT(ptr, bit) (void *)((uintptr_t)(ptr) | (uintptr_t)(bit)) > +#define CLEAR_BIT(ptr, bit) (void *)((uintptr_t)(ptr) & ~(uintptr_t)(bit= )) > +#define GET_BIT(ptr, bit) (void *)((uintptr_t)(ptr) & (uintptr_t)(bit)) > + > +/* > + * @internal helper function > + * Add a rule of type SPI_DIP or SPI_DIP_SIP. > + * Inserts a rule into an appropriate hash table, > + * updates the value for a given SPI in SPI_ONLY hash table > + * reflecting presence of more specific rule type in two LSBs. > + * Updates a counter that reflects the number of rules whith the same SP= I. > + */ > +static inline int > +add_specific(struct rte_ipsec_sad *sad, void *key, > + int key_type, void *sa) > +{ > + void *tmp_val; > + int ret, notexist; > + > + ret =3D rte_hash_lookup(sad->hash[key_type], key); > + notexist =3D (ret =3D=3D -ENOENT); > + ret =3D rte_hash_add_key_data(sad->hash[key_type], key, sa); > + if (ret !=3D 0) > + return ret; > + ret =3D rte_hash_lookup_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY], > + key, &tmp_val); > + if (ret < 0) > + tmp_val =3D NULL; > + tmp_val =3D SET_BIT(tmp_val, key_type); > + ret =3D rte_hash_add_key_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY], > + key, tmp_val); > + if (ret !=3D 0) > + return ret; > + ret =3D rte_hash_lookup(sad->hash[RTE_IPSEC_SAD_SPI_ONLY], key); > + if (key_type =3D=3D RTE_IPSEC_SAD_SPI_DIP) > + sad->cnt_arr[ret].cnt_2 +=3D notexist; > + else > + sad->cnt_arr[ret].cnt_3 +=3D notexist; > + > + return 0; > +} > + > int > -rte_ipsec_sad_add(__rte_unused struct rte_ipsec_sad *sad, > - __rte_unused union rte_ipsec_sad_key *key, > - __rte_unused int key_type, __rte_unused void *sa) > +rte_ipsec_sad_add(struct rte_ipsec_sad *sad, union rte_ipsec_sad_key *ke= y, > + int key_type, void *sa) > +{ > + void *tmp_val; > + int ret; > + > + if ((sad =3D=3D NULL) || (key =3D=3D NULL) || (sa =3D=3D NULL) || > + /* sa must be 4 byte aligned */ > + (GET_BIT(sa, RTE_IPSEC_SAD_KEY_TYPE_MASK) !=3D 0)) > + return -EINVAL; > + > + /* > + * Rules are stored in three hash tables depending on key_type. > + * All rules will also have an entry in SPI_ONLY table, with entry > + * value's two LSB's also indicating presence of rule with this SPI > + * in other tables. > + */ > + switch (key_type) { > + case(RTE_IPSEC_SAD_SPI_ONLY): > + ret =3D rte_hash_lookup_data(sad->hash[key_type], > + key, &tmp_val); > + if (ret >=3D 0) > + tmp_val =3D SET_BIT(sa, GET_BIT(tmp_val, > + RTE_IPSEC_SAD_KEY_TYPE_MASK)); > + else > + tmp_val =3D sa; > + ret =3D rte_hash_add_key_data(sad->hash[key_type], > + key, tmp_val); > + return ret; > + case(RTE_IPSEC_SAD_SPI_DIP): > + case(RTE_IPSEC_SAD_SPI_DIP_SIP): > + return add_specific(sad, key, key_type, sa); > + default: > + return -EINVAL; > + } > +} > + > +/* > + * @internal helper function > + * Delete a rule of type SPI_DIP or SPI_DIP_SIP. > + * Deletes an entry from an appropriate hash table and decrements > + * an entry counter for given SPI. > + * If entry to remove is the last one with given SPI within the table, > + * then it will also update related entry in SPI_ONLY table. > + * Removes an entry from SPI_ONLY hash table if there no rule left > + * for this SPI in any table. > + */ > +static inline int > +del_specific(struct rte_ipsec_sad *sad, void *key, int key_type) > { const void *key ?=20 > - return -ENOTSUP; > + void *tmp_val; > + int ret; > + uint32_t *cnt; Few extra comments inside that function and add_specific() wouldn't hurt. > + > + ret =3D rte_hash_del_key(sad->hash[key_type], key); > + if (ret < 0) > + return ret; > + ret =3D rte_hash_lookup_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY], > + key, &tmp_val); > + if (ret < 0) > + return ret; > + cnt =3D (key_type =3D=3D RTE_IPSEC_SAD_SPI_DIP) ? &sad->cnt_arr[ret].cn= t_2 : > + &sad->cnt_arr[ret].cnt_3; > + if (--(*cnt) !=3D 0) > + return 0; > + > + tmp_val =3D CLEAR_BIT(tmp_val, key_type); > + if (tmp_val =3D=3D NULL) > + ret =3D rte_hash_del_key(sad->hash[RTE_IPSEC_SAD_SPI_ONLY], key); > + else > + ret =3D rte_hash_add_key_data(sad->hash[RTE_IPSEC_SAD_SPI_ONLY], > + key, tmp_val); > + if (ret < 0) > + return ret; > + return 0; > } >=20 > int > -rte_ipsec_sad_del(__rte_unused struct rte_ipsec_sad *sad, > - __rte_unused union rte_ipsec_sad_key *key, > - __rte_unused int key_type) > +rte_ipsec_sad_del(struct rte_ipsec_sad *sad, union rte_ipsec_sad_key *ke= y, const union rte_ipsec_sad_key *key ? > + int key_type) > { > - return -ENOTSUP; > + void *tmp_val; > + int ret; > + > + if ((sad =3D=3D NULL) || (key =3D=3D NULL)) > + return -EINVAL; > + switch (key_type) { > + case(RTE_IPSEC_SAD_SPI_ONLY): > + ret =3D rte_hash_lookup_data(sad->hash[key_type], > + key, &tmp_val); > + if (ret < 0) > + return ret; > + if (GET_BIT(tmp_val, RTE_IPSEC_SAD_KEY_TYPE_MASK) =3D=3D 0) { > + RTE_ASSERT((cnt_2 =3D=3D 0) && (cnt_3 =3D=3D 0)); > + ret =3D rte_hash_del_key(sad->hash[key_type], > + key); I think something like: ret =3D ret < 0 ? ret : 0; is needed here. > + } else { > + tmp_val =3D GET_BIT(tmp_val, > + RTE_IPSEC_SAD_KEY_TYPE_MASK); > + ret =3D rte_hash_add_key_data(sad->hash[key_type], > + key, tmp_val); > + } > + return ret; > + case(RTE_IPSEC_SAD_SPI_DIP): > + case(RTE_IPSEC_SAD_SPI_DIP_SIP): > + return del_specific(sad, key, key_type); > + default: > + return -EINVAL; > + } > } >=20 > struct rte_ipsec_sad * > @@ -248,10 +393,86 @@ rte_ipsec_sad_free(struct rte_ipsec_sad *sad) > rte_free(te); > } >=20 Pls add a comment for that one and other internal function. Even if you do remember what exactly it does now, you won't a year later :) > +static int > +__ipsec_sad_lookup(const struct rte_ipsec_sad *sad, > + const union rte_ipsec_sad_key *keys[], uint32_t n, void *sa[]) > +{ > + const void *keys_2[RTE_HASH_LOOKUP_BULK_MAX]; > + const void *keys_3[RTE_HASH_LOOKUP_BULK_MAX]; > + void *vals_2[RTE_HASH_LOOKUP_BULK_MAX] =3D {NULL}; > + void *vals_3[RTE_HASH_LOOKUP_BULK_MAX] =3D {NULL}; > + uint32_t idx_2[RTE_HASH_LOOKUP_BULK_MAX]; > + uint32_t idx_3[RTE_HASH_LOOKUP_BULK_MAX]; > + uint64_t mask_1, mask_2, mask_3; > + uint64_t map, map_spec; > + uint32_t n_2 =3D 0; > + uint32_t n_3 =3D 0; > + uint32_t i; > + int n_pkts =3D 0; s/n_pkts/found/ ? > + > + for (i =3D 0; i < n; i++) > + sa[i] =3D NULL; > + > + /* > + * 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); > + for (map =3D mask_1; map; map &=3D (map - 1)) { > + i =3D rte_bsf64(map); > + /* > + * if returned value indicates presence of a rule in other > + * tables save a key for further lookup. > + */ > + if ((uintptr_t)sa[i] & RTE_IPSEC_SAD_SPI_DIP_SIP) { > + idx_3[n_3] =3D i; > + keys_3[n_3++] =3D keys[i]; > + } > + if ((uintptr_t)sa[i] & RTE_IPSEC_SAD_SPI_DIP) { > + idx_2[n_2] =3D i; > + keys_2[n_2++] =3D keys[i]; > + } > + sa[i] =3D CLEAR_BIT(sa[i], RTE_IPSEC_SAD_KEY_TYPE_MASK); > + } Just as a thought - instead of setting all sa[] to NULL first, and then going though only found in the loop above, wouldn't it be a bit faster - after lookup bulk go through all sa[] and set them depending on mask value? Then first(zero sa[] loop) can be removed. > + > + if (n_2 !=3D 0) { > + rte_hash_lookup_bulk_data(sad->hash[RTE_IPSEC_SAD_SPI_DIP], > + keys_2, n_2, &mask_2, vals_2); > + for (map_spec =3D mask_2; map_spec; map_spec &=3D (map_spec - 1)) { > + i =3D rte_bsf64(map_spec); > + sa[idx_2[i]] =3D vals_2[i]; > + } > + } > + if (n_3 !=3D 0) { > + rte_hash_lookup_bulk_data(sad->hash[RTE_IPSEC_SAD_SPI_DIP_SIP], > + keys_3, n_3, &mask_3, vals_3); > + for (map_spec =3D mask_3; map_spec; map_spec &=3D (map_spec - 1)) { > + i =3D rte_bsf64(map_spec); > + sa[idx_3[i]] =3D vals_3[i]; > + } > + } > + for (i =3D 0; i < n; i++) > + n_pkts +=3D (sa[i] !=3D NULL); > + > + return n_pkts; > +} > + > int > -rte_ipsec_sad_lookup(__rte_unused const struct rte_ipsec_sad *sad, > - __rte_unused const union rte_ipsec_sad_key *keys[], > - __rte_unused uint32_t n, __rte_unused void *sa[]) > +rte_ipsec_sad_lookup(const struct rte_ipsec_sad *sad, > + const union rte_ipsec_sad_key *keys[], uint32_t n, void *sa[]) Better to follow usual parameter convention and move 'n' after pointers, i.= e.: int rte_ipsec_sad_lookup(const struct rte_ipsec_sad *sad, const union rte_i= psec_sad_key *keys[], void *sa[], uint32_t n) Or provably even better: int rte_ipsec_sad_lookup(const struct rte_ipsec_sad *sad, const union rte_i= psec_sad_key *keys[], const void *sa[], uint32_t n) > { > - return -ENOTSUP; > + uint32_t num, i =3D 0; > + int n_pkts =3D 0; > + > + if (unlikely((sad =3D=3D NULL) || (keys =3D=3D NULL) || (sa =3D=3D NULL= ))) > + return -EINVAL; > + > + do { > + num =3D RTE_MIN(n - i, (uint32_t)RTE_HASH_LOOKUP_BULK_MAX); > + n_pkts +=3D __ipsec_sad_lookup(sad, > + &keys[i], num, &sa[i]); > + i +=3D num; > + } while (i !=3D n); > + > + return n_pkts; > } > -- > 2.7.4