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 742ADA04DC; Mon, 26 Oct 2020 16:06:29 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 88CD42BB5; Mon, 26 Oct 2020 16:06:22 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by dpdk.org (Postfix) with ESMTP id AC67E29C6 for ; Mon, 26 Oct 2020 16:06:20 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 5CFB97F4F3; Mon, 26 Oct 2020 18:06:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 5CFB97F4F3 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1603724779; bh=Sgi1CS4EMe1gewbVvB+2MgwEjK9bAxw4+2xtyBeR61w=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=QIT/iTnIRIx9od0Y1I4Fq7RqpawJmfHBazkSa1ztreJoINmew5xlTmAEOY3LkZMUV fcZfnAaF9ELrdgDILWa7hVJO/BDG+ub5FwBuoxXVYhove6MPb1BnF7F6sZyAtzQtfR WcUqwx9vXfhzB2Zspiqs3mikZaWB6Pn1h9mw9CLw= To: Thomas Monjalon , dev@dpdk.org Cc: ferruh.yigit@intel.com, david.marchand@redhat.com, bruce.richardson@intel.com, olivier.matz@6wind.com, 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 References: <20201026052105.1561859-1-thomas@monjalon.net> <20201026052105.1561859-6-thomas@monjalon.net> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <13f99aa6-04d6-553f-0c3a-1d40ee5dbf65@oktetlabs.ru> Date: Mon, 26 Oct 2020 18:06:19 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 MIME-Version: 1.0 In-Reply-To: <20201026052105.1561859-6-thomas@monjalon.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 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 10/26/20 8:20 AM, 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 [snip] > diff --git a/drivers/net/ixgbe/ixgbe_ipsec.c b/drivers/net/ixgbe/ixgbe_ipsec.c > index 48f5082d49..0232db20ed 100644 > --- a/drivers/net/ixgbe/ixgbe_ipsec.c > +++ b/drivers/net/ixgbe/ixgbe_ipsec.c > @@ -484,7 +484,8 @@ ixgbe_crypto_update_mb(void *device __rte_unused, > get_sec_session_private_data(session); > if (ic_session->op == IXGBE_OP_AUTHENTICATED_ENCRYPTION) { > union ixgbe_crypto_tx_desc_md *mdata = > - (union ixgbe_crypto_tx_desc_md *)&m->udata64; > + (union ixgbe_crypto_tx_desc_md *) > + rte_security_dynfield(m); IMHO alignment looks a bit confusing here, may be add one more TAB? > mdata->enc = 1; > mdata->sa_idx = ic_session->sa_index; > mdata->pad_len = ixgbe_crypto_compute_pad_len(m); > @@ -751,5 +752,7 @@ ixgbe_ipsec_ctx_create(struct rte_eth_dev *dev) > return -ENOMEM; > } > } > + if (rte_security_dynfield_register() < 0) > + return -rte_errno; > return 0; > } [snip] > diff --git a/examples/ipsec-secgw/ipsec_worker.c b/examples/ipsec-secgw/ipsec_worker.c > index b6c851f257..72f698893d 100644 > --- a/examples/ipsec-secgw/ipsec_worker.c > +++ b/examples/ipsec-secgw/ipsec_worker.c > @@ -208,7 +208,8 @@ 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 = RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset, > + struct ipsec_sa *); I think it should be de-reference above, i.e. sa = (struct ipsec_sa *)*RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset, RTE_SECURITY_DYNFIELD_TYPE *); or just sa = *RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset, struct ipsec_sa **); and why not rte_security_dynfield()? It all looks very fragile. May be at least add RTE_BUILD_BUG_ON(sizeof(RTE_SECURITY_DYNFIELD_TYPE) == sizeof(ipsec_sa *)); and similar checks when an application or a library does lookup for a dynamic field. In general since lookup should not happen on data path, may be lookup should return size of the field which must be checked by the caller for consistency. > } > > /* Check if we have a match */ > @@ -226,7 +227,8 @@ 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 = RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset, > + struct ipsec_sa *); same > } > > /* Check if we have a match */ > @@ -357,7 +359,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; > + *RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset, > + struct rte_security_session **) = sess->security.ses; rte_security_dynfield() ? Is it really different types of value in one example application? It looks like it should be different dynamic fields. Otherwise, I don't understand how to work with it. In fact may be above is out of scope of the patch series... > > /* Mark the packet for Tx security offload */ > pkt->ol_flags |= PKT_TX_SEC_OFFLOAD; > @@ -465,7 +468,9 @@ ipsec_wrkr_non_burst_int_port_drv_mode(struct eh_event_link_info *links, > } > > /* Save security session */ > - pkt->userdata = sess_tbl[port_id]; > + *RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset, > + struct rte_security_session **) = > + sess_tbl[port_id]; rte_security_dynfield() ? > > /* Mark the packet for Tx security offload */ > pkt->ol_flags |= PKT_TX_SEC_OFFLOAD; [snip]