From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id 90783A04B5;
Tue, 27 Oct 2020 11:05:48 +0100 (CET)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id C421D2BD5;
Tue, 27 Oct 2020 11:05:45 +0100 (CET)
Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com
[209.85.221.66]) by dpdk.org (Postfix) with ESMTP id E8E982BD3
for <dev@dpdk.org>; Tue, 27 Oct 2020 11:05:42 +0100 (CET)
Received: by mail-wr1-f66.google.com with SMTP id i1so1207396wro.1
for <dev@dpdk.org>; Tue, 27 Oct 2020 03:05:42 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google;
h=date:from:to:cc:subject:message-id:references:mime-version
:content-disposition:in-reply-to:user-agent;
bh=9PRBCnx3FnzxbB9tvqJRnID8r6Pgf11Npw2D6TeHAes=;
b=J+UBUUQrUHNksCPeqRypJnTzww9TjU/qxYr5hVVB4XqFz3r0rpCRMe6/ZwZfJMuJZ3
IVmYGIk420kGlE4rq8mvwlJusa6oIfW1ORAZDCr+zVHAtxF1QdjvLw5MXHr859LPmUOM
7mi6+X0Qw+cIkOEWbsQcShFi/JXkZposyJrR53V6lK02AzMelht4pHy2eJso90dWpxfz
oWq6Q8oQPqh0XSQU8Rl0qemHYNm0XcraiIYWTjd9U3DaJTn+OC/re24D10igrw0s21xv
d3K2GibEZ0vACoAH7lt6DESA1Mg/Idwo3drDM6L+m/ZGvao9/iUW6S84zRK61zBPHylL
Nu7w==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:references
:mime-version:content-disposition:in-reply-to:user-agent;
bh=9PRBCnx3FnzxbB9tvqJRnID8r6Pgf11Npw2D6TeHAes=;
b=cYiBAjS5BvQGOTyI3kRbT40lnh9N2N/kIsq50iNWyTkxUEu9bsNUTyOX3z3axoEu9r
tmRcsnXfOIF3GwbhOui4JvV8phezxea2gaVM/FWD9PniQD2SJc8zs+sTglI+4CGsOdCg
sgRfpCViAZwdM21mYYUVzSATae5FfEjUPbYJqkFCMxhBRbtJxmDSG+FPlyYt2Dm0dNVA
qYTLBCHijGPHpnw/vkBp4BFSfXEWYnBplspSPiDcjlJnTEVU86vb8SHgCs5SQZvSAzUh
NN2bdY2LpkrQEscDHrKvjIYSmtz228rTEAGgi3cvhTw1egpMaHCx9CZKuu/sWOX+Fys2
QUYg==
X-Gm-Message-State: AOAM531/rDXxRU391VfzWCtGXh/cSvVI/aQPrEHwGZISnrWPfiZsDxrd
MIE4I9dVuFRSrCnsGAwEP1QR7A==
X-Google-Smtp-Source: ABdhPJyIXyAK/6ZvcY9qHfI27E7QfrCj/J2vPw3sI6I/A3o0axQc5KWKedXdfq4kI2/WhGOOG62vew==
X-Received: by 2002:adf:ce12:: with SMTP id p18mr1886700wrn.52.1603793142561;
Tue, 27 Oct 2020 03:05:42 -0700 (PDT)
Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr.
[2a01:cb0c:5:a600:3456:36f7:e65e:d1a0])
by smtp.gmail.com with ESMTPSA id x1sm1319220wrl.41.2020.10.27.03.05.41
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Tue, 27 Oct 2020 03:05:41 -0700 (PDT)
Date: Tue, 27 Oct 2020 11:05:41 +0100
From: Olivier Matz <olivier.matz@6wind.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: dev@dpdk.org, ferruh.yigit@intel.com, david.marchand@redhat.com,
bruce.richardson@intel.com, andrew.rybchenko@oktetlabs.ru,
akhil.goyal@nxp.com, Declan Doherty <declan.doherty@intel.com>,
Ankur Dwivedi <adwivedi@marvell.com>,
Anoob Joseph <anoobj@marvell.com>, Jeff Guo <jia.guo@intel.com>,
Haiyue Wang <haiyue.wang@intel.com>, Jerin Jacob <jerinj@marvell.com>,
Nithin Dabilpuram <ndabilpuram@marvell.com>,
Kiran Kumar K <kirankumark@marvell.com>,
Radu Nicolau <radu.nicolau@intel.com>, Ray Kinsella <mdr@ashroe.eu>,
Neil Horman <nhorman@tuxdriver.com>
Message-ID: <20201027100541.GO1898@platinum>
References: <20201026052105.1561859-1-thomas@monjalon.net>
<20201026222013.2147904-1-thomas@monjalon.net>
<20201026222013.2147904-6-thomas@monjalon.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201026222013.2147904-6-thomas@monjalon.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Subject: Re: [dpdk-dev] [PATCH v2 05/15] security: switch metadata to
dynamic mbuf field
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
On Mon, Oct 26, 2020 at 11:20:03PM +0100, Thomas Monjalon wrote:
> The device-specific metadata was stored in the deprecated field udata64.
> It is moved to a dynamic mbuf field in order to allow removal of udata64.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> doc/guides/prog_guide/rte_security.rst | 9 +++---
> drivers/crypto/octeontx2/otx2_cryptodev_sec.c | 5 ++-
> drivers/net/ixgbe/ixgbe_ipsec.c | 5 ++-
> drivers/net/ixgbe/ixgbe_rxtx.c | 6 ++--
> drivers/net/octeontx2/otx2_ethdev.h | 1 +
> drivers/net/octeontx2/otx2_ethdev_sec.c | 5 ++-
> drivers/net/octeontx2/otx2_ethdev_sec_tx.h | 2 +-
> drivers/net/octeontx2/otx2_rx.h | 2 +-
> examples/ipsec-secgw/ipsec-secgw.c | 9 +++---
> examples/ipsec-secgw/ipsec_worker.c | 12 ++++---
> lib/librte_security/rte_security.c | 22 +++++++++++++
> lib/librte_security/rte_security.h | 32 +++++++++++++++++++
> lib/librte_security/rte_security_driver.h | 3 ++
> lib/librte_security/version.map | 3 ++
> 14 files changed, 96 insertions(+), 20 deletions(-)
>
<...>
> --- a/examples/ipsec-secgw/ipsec_worker.c
> +++ b/examples/ipsec-secgw/ipsec_worker.c
> @@ -208,7 +208,7 @@ process_ipsec_ev_inbound(struct ipsec_ctx *ctx, struct route_table *rt,
> "Inbound security offload failed\n");
> goto drop_pkt_and_exit;
> }
> - sa = pkt->userdata;
> + sa = *(struct ipsec_sa **)rte_security_dynfield(pkt);
> }
>
> /* Check if we have a match */
> @@ -226,7 +226,7 @@ process_ipsec_ev_inbound(struct ipsec_ctx *ctx, struct route_table *rt,
> "Inbound security offload failed\n");
> goto drop_pkt_and_exit;
> }
> - sa = pkt->userdata;
> + sa = *(struct ipsec_sa **)rte_security_dynfield(pkt);
> }
>
> /* Check if we have a match */
> @@ -357,7 +357,8 @@ process_ipsec_ev_outbound(struct ipsec_ctx *ctx, struct route_table *rt,
> }
>
> if (sess->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA)
> - pkt->userdata = sess->security.ses;
> + *(struct rte_security_session **)rte_security_dynfield(pkt) =
> + sess->security.ses;
>
> /* Mark the packet for Tx security offload */
> pkt->ol_flags |= PKT_TX_SEC_OFFLOAD;
> @@ -465,7 +466,10 @@ ipsec_wrkr_non_burst_int_port_drv_mode(struct eh_event_link_info *links,
> }
>
> /* Save security session */
> - pkt->userdata = sess_tbl[port_id];
> + if (rte_security_dynfield_is_registered())
> + *(struct rte_security_session **)
> + rte_security_dynfield(pkt) =
> + sess_tbl[port_id];
>
Maybe the last 2 lines can be on the same line (a bit more than 80,
but less than 100 chars).
This is not clear to me why you need to call
rte_security_dynfield_is_registered() here, and not in other places.
Would it make sense instead to always register the dynfield at some
place in rte_security, so that we are sure that as soon as we use
rte_security, the dynfield is registered. A good place would be an init
function, but I don't see one.
> /* Mark the packet for Tx security offload */
> pkt->ol_flags |= PKT_TX_SEC_OFFLOAD;
> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> index ee4666026a..4fb0b797e9 100644
> --- a/lib/librte_security/rte_security.c
> +++ b/lib/librte_security/rte_security.c
> @@ -23,6 +23,28 @@
> RTE_PTR_OR_ERR_RET(p1->p2->p3, last_retval); \
> } while (0)
>
> +#define RTE_SECURITY_DYNFIELD_NAME "rte_security_dynfield_metadata"
> +int rte_security_dynfield_offset = -1;
> +
> +int
> +rte_security_dynfield_register(void)
> +{
> + static const struct rte_mbuf_dynfield dynfield_desc = {
> + .name = RTE_SECURITY_DYNFIELD_NAME,
> + .size = sizeof(RTE_SECURITY_DYNFIELD_TYPE),
> + .align = __alignof__(RTE_SECURITY_DYNFIELD_TYPE),
> + };
> + rte_security_dynfield_offset =
> + rte_mbuf_dynfield_register(&dynfield_desc);
> + return rte_security_dynfield_offset;
> +}
> +
> +bool
> +rte_security_dynfield_is_registered(void)
> +{
> + return rte_security_dynfield_offset >= 0;
> +}
> +
Wouldn't it be better to have it in a static inline function?
The point is just to check that offset is >= 0, and it is used
in data path.
> struct rte_security_session *
> rte_security_session_create(struct rte_security_ctx *instance,
> struct rte_security_session_conf *conf,
> diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
> index 271531af12..7fbdee99cc 100644
> --- a/lib/librte_security/rte_security.h
> +++ b/lib/librte_security/rte_security.h
> @@ -27,6 +27,7 @@ extern "C" {
> #include <rte_common.h>
> #include <rte_crypto.h>
> #include <rte_mbuf.h>
> +#include <rte_mbuf_dyn.h>
> #include <rte_memory.h>
> #include <rte_mempool.h>
>
> @@ -451,6 +452,37 @@ int
> rte_security_session_destroy(struct rte_security_ctx *instance,
> struct rte_security_session *sess);
>
> +/** Device-specific metadata field type */
> +#define RTE_SECURITY_DYNFIELD_TYPE uint64_t
What about using a typedef instead of a #define?
It could be in lowercase: rte_security_dynfield_t