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 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 ; Tue, 27 Oct 2020 11:05:42 +0100 (CET) Received: by mail-wr1-f66.google.com with SMTP id i1so1207396wro.1 for ; 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 To: Thomas Monjalon 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 , Ankur Dwivedi , Anoob Joseph , Jeff Guo , Haiyue Wang , Jerin Jacob , Nithin Dabilpuram , Kiran Kumar K , Radu Nicolau , Ray Kinsella , Neil Horman 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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 > #include > #include > +#include > #include > #include > > @@ -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