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 0709FA04DC; Tue, 20 Oct 2020 13:56:03 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EFBCDAD63; Tue, 20 Oct 2020 13:56:00 +0200 (CEST) Received: from wout4-smtp.messagingengine.com (wout4-smtp.messagingengine.com [64.147.123.20]) by dpdk.org (Postfix) with ESMTP id 1CFDEAD61 for ; Tue, 20 Oct 2020 13:55:58 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.west.internal (Postfix) with ESMTP id 169C8D92; Tue, 20 Oct 2020 07:55:56 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Tue, 20 Oct 2020 07:55:56 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm2; bh= kYzBYgvvLFWgFlPI4LAtqvsFdupITRZPd17fC1DGBXE=; b=ALzhpZViIIZcz+Zt 9a1ji0Y2R7Uowl5gR4Tzgc+QnBeJF93MaCixqx5sktkCH877FwripYR/LS4vV1nW CfjtROassIQfWVuVeourFFQzm0Q7VSsjsi32LXIA51B1T4MysjHR1Ws7rRfuX2Iz 7TF4jj//G323IRt92HumOKB1YZh1WjdtwiOLKR8iE6ya30HIHycpAT1m7FxhEhT8 QRq8XWiDjaHes5fB5JW9wOI5LDaHF2WSUwAjwUxpczpFV42Gigt5r93Anymvy5fj dFchmP1R2K2QmlEg06s7zdt2a/N64sk8mB0vrP91mFD6TQXW/K45lukXroUSSRNy fuHRKg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=kYzBYgvvLFWgFlPI4LAtqvsFdupITRZPd17fC1DGB XE=; b=F18rd+uRflXSbb0FBH935PzTP3RiK6ytY1s6soy2mJWBVE6L5K7kufFYJ bGeVIfg5n498uFd/ekXTEKXDXR0P08UJyODyusQTxKpwhVxRR3eJ51hjCbp5L8jb sSTznlqMfO3zbCdAIECOHnWbiCoedvKa8BRxYv9XZbK2/+7Z0sEYN5eRZHC924CI KbIL+Wb84iy7rT2nQf765weY2OhKEEcaEdEWV9ozvp3InmEPwF7DZ+d4iZv/Biq5 TvvbBkjo8YLjMxNAiGHuc/K4Jy5/6Uuu79hjcq6OLr0Yb9tOFfMT7vtq2OPzm4yU 2CgZgxm0AXSn0DQwsEscbnkEtw4Vw== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrjeefgdeggecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthhqredttddtudenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf frrghtthgvrhhnpeefffegiedtfeetffeuueffleeuuddvleetkeelgfevudekvdeuueeg ieekhfelgfenucffohhmrghinhepughpughkrdhorhhgnecukfhppeejjedrudefgedrvd dtfedrudekgeenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhr ohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 697523280059; Tue, 20 Oct 2020 07:55:54 -0400 (EDT) From: Thomas Monjalon To: Morten =?ISO-8859-1?Q?Br=F8rup?= Cc: dev@dpdk.org, Olivier Matz Date: Tue, 20 Oct 2020 13:55:53 +0200 Message-ID: <4466910.c3LTmThTiu@thomas> In-Reply-To: <20201007091614.GO21395@platinum> References: <20200916104013.126362-1-mb@smartsharesystems.com> <20201007091614.GO21395@platinum> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" Subject: Re: [dpdk-dev] [PATCH v3] mbuf: minor cleanup 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 Morten, Any update about this patch please? 07/10/2020 11:16, Olivier Matz: > Hi Morten, >=20 > Thanks for this cleanup. Please see some comments below. >=20 > On Wed, Sep 16, 2020 at 12:40:13PM +0200, Morten Br=F8rup wrote: > > The mbuf header files had some commenting style errors that affected the > > API documentation. > > Also, the RTE_ prefix was missing on a macro and a definition. > >=20 > > Note: This patch does not touch the offload and attachment flags that a= re > > also missing the RTE_ prefix. > >=20 > > Changes only affecting documentation: > > * Removed the MBUF_INVALID_PORT definition from rte_mbuf.h; it is > > already defined in rte_mbuf_core.h. > > This removal also reestablished the description of the > > rte_pktmbuf_reset() function. > > * Corrected the comment related to RTE_MBUF_MAX_NB_SEGS. > > * Corrected the comment related to PKT_TX_QINQ_PKT. > >=20 > > Changes regarding missing RTE_ prefix: > > * Converted the MBUF_RAW_ALLOC_CHECK() macro to an > > __rte_mbuf_raw_sanity_check() inline function. > > Added backwards compatible macro with the original name. > > * Renamed the MBUF_INVALID_PORT definition to RTE_MBUF_PORT_INVALID. > > Added backwards compatible definition with the original name. > >=20 > > v2: > > * Use RTE_MBUF_PORT_INVALID instead of MBUF_INVALID_PORT in rte_mbuf.c. > >=20 > > v3: > > * The functions/macros used in __rte_mbuf_raw_sanity_check() require > > RTE_ENABLE_ASSERT or RTE_LIBRTE_MBUF_DEBUG, or they don't use the mbuf > > parameter, which generates a compiler waning. So mark the mbuf parame= ter > > __rte_unused if none of them are defined. > >=20 > > Signed-off-by: Morten Br=F8rup > > --- > > doc/guides/rel_notes/deprecation.rst | 7 ---- > > lib/librte_mbuf/rte_mbuf.c | 4 +- > > lib/librte_mbuf/rte_mbuf.h | 55 +++++++++++++++++++--------- > > lib/librte_mbuf/rte_mbuf_core.h | 9 +++-- > > 4 files changed, 45 insertions(+), 30 deletions(-) > >=20 > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_note= s/deprecation.rst > > index 279eccb04..88d7d0761 100644 > > --- a/doc/guides/rel_notes/deprecation.rst > > +++ b/doc/guides/rel_notes/deprecation.rst > > @@ -294,13 +294,6 @@ Deprecation Notices > > - https://patches.dpdk.org/patch/71457/ > > - https://patches.dpdk.org/patch/71456/ > > =20 > > -* rawdev: The rawdev APIs which take a device-specific structure as > > - parameter directly, or indirectly via a "private" pointer inside ano= ther > > - structure, will be modified to take an additional parameter of the > > - structure size. The affected APIs will include ``rte_rawdev_info_get= ``, > > - ``rte_rawdev_configure``, ``rte_rawdev_queue_conf_get`` and > > - ``rte_rawdev_queue_setup``. > > - > > * acl: ``RTE_ACL_CLASSIFY_NUM`` enum value will be removed. > > This enum value is not used inside DPDK, while it prevents to add new > > classify algorithms without causing an ABI breakage. >=20 > I think this change is not related. >=20 > This makes me think that a deprecation notice could be done for the > old names without the RTE_ prefix, to be removed in 21.11. >=20 >=20 > > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c > > index 8a456e5e6..53a015311 100644 > > --- a/lib/librte_mbuf/rte_mbuf.c > > +++ b/lib/librte_mbuf/rte_mbuf.c > > @@ -104,7 +104,7 @@ rte_pktmbuf_init(struct rte_mempool *mp, > > /* init some constant fields */ > > m->pool =3D mp; > > m->nb_segs =3D 1; > > - m->port =3D MBUF_INVALID_PORT; > > + m->port =3D RTE_MBUF_PORT_INVALID; > > rte_mbuf_refcnt_set(m, 1); > > m->next =3D NULL; > > } > > @@ -207,7 +207,7 @@ __rte_pktmbuf_init_extmem(struct rte_mempool *mp, > > /* init some constant fields */ > > m->pool =3D mp; > > m->nb_segs =3D 1; > > - m->port =3D MBUF_INVALID_PORT; > > + m->port =3D RTE_MBUF_PORT_INVALID; > > m->ol_flags =3D EXT_ATTACHED_MBUF; > > rte_mbuf_refcnt_set(m, 1); > > m->next =3D NULL; > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > > index 7259575a7..406d3abb2 100644 > > --- a/lib/librte_mbuf/rte_mbuf.h > > +++ b/lib/librte_mbuf/rte_mbuf.h > > @@ -554,12 +554,36 @@ __rte_experimental > > int rte_mbuf_check(const struct rte_mbuf *m, int is_header, > > const char **reason); > > =20 > > -#define MBUF_RAW_ALLOC_CHECK(m) do { \ > > - RTE_ASSERT(rte_mbuf_refcnt_read(m) =3D=3D 1); \ > > - RTE_ASSERT((m)->next =3D=3D NULL); \ > > - RTE_ASSERT((m)->nb_segs =3D=3D 1); \ > > - __rte_mbuf_sanity_check(m, 0); \ > > -} while (0) > > +#if defined(RTE_ENABLE_ASSERT) || defined(RTE_LIBRTE_MBUF_DEBUG) >=20 > I don't see why this #if is needed. Wouldn't it work to have only > one function definition with the __rte_unused attribute? >=20 > > +/** > > + * Sanity checks on a reinitialized mbuf. > > + * > > + * Check the consistency of the given reinitialized mbuf. > > + * The function will cause a panic if corruption is detected. > > + * > > + * Check that the mbuf is properly reinitialized (refcnt=3D1, next=3DN= ULL, > > + * nb_segs=3D1), as done by rte_pktmbuf_prefree_seg(). > > + * >=20 > Maybe indicate that these checks are only done when debug is on. >=20 > > + * @param m > > + * The mbuf to be checked. > > + */ > > +static __rte_always_inline void > > +__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m) > > +{ > > + RTE_ASSERT(rte_mbuf_refcnt_read(m) =3D=3D 1); > > + RTE_ASSERT(m->next =3D=3D NULL); > > + RTE_ASSERT(m->nb_segs =3D=3D 1); > > + __rte_mbuf_sanity_check(m, 0); > > +} > > +#else > > +static __rte_always_inline void > > +__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m __rte_unused) > > +{ > > + /* Nothing here. */ > > +} > > +#endif > > +/** For backwards compatibility. */ > > +#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check(m) >=20 > It looks that MBUF_RAW_ALLOC_CHECK() is also used in drivers/net/sfc, > I think it should be updated too. >=20 > > =20 > > /** > > * Allocate an uninitialized mbuf from mempool *mp*. > > @@ -586,7 +610,7 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(s= truct rte_mempool *mp) > > =20 > > if (rte_mempool_get(mp, (void **)&m) < 0) > > return NULL; > > - MBUF_RAW_ALLOC_CHECK(m); > > + __rte_mbuf_raw_sanity_check(m); > > return m; > > } > > =20 > > @@ -609,10 +633,7 @@ rte_mbuf_raw_free(struct rte_mbuf *m) > > { > > RTE_ASSERT(!RTE_MBUF_CLONED(m) && > > (!RTE_MBUF_HAS_EXTBUF(m) || RTE_MBUF_HAS_PINNED_EXTBUF(m))); > > - RTE_ASSERT(rte_mbuf_refcnt_read(m) =3D=3D 1); > > - RTE_ASSERT(m->next =3D=3D NULL); > > - RTE_ASSERT(m->nb_segs =3D=3D 1); > > - __rte_mbuf_sanity_check(m, 0); > > + __rte_mbuf_raw_sanity_check(m); > > rte_mempool_put(m->pool, m); > > } > > =20 > > @@ -858,8 +879,6 @@ static inline void rte_pktmbuf_reset_headroom(struc= t rte_mbuf *m) > > * @param m > > * The packet mbuf to be reset. > > */ > > -#define MBUF_INVALID_PORT UINT16_MAX > > - > > static inline void rte_pktmbuf_reset(struct rte_mbuf *m) > > { > > m->next =3D NULL; > > @@ -868,7 +887,7 @@ static inline void rte_pktmbuf_reset(struct rte_mbu= f *m) > > m->vlan_tci =3D 0; > > m->vlan_tci_outer =3D 0; > > m->nb_segs =3D 1; > > - m->port =3D MBUF_INVALID_PORT; > > + m->port =3D RTE_MBUF_PORT_INVALID; > > =20 > > m->ol_flags &=3D EXT_ATTACHED_MBUF; > > m->packet_type =3D 0; > > @@ -931,22 +950,22 @@ static inline int rte_pktmbuf_alloc_bulk(struct r= te_mempool *pool, > > switch (count % 4) { > > case 0: > > while (idx !=3D count) { > > - MBUF_RAW_ALLOC_CHECK(mbufs[idx]); > > + __rte_mbuf_raw_sanity_check(mbufs[idx]); > > rte_pktmbuf_reset(mbufs[idx]); > > idx++; > > /* fall-through */ > > case 3: > > - MBUF_RAW_ALLOC_CHECK(mbufs[idx]); > > + __rte_mbuf_raw_sanity_check(mbufs[idx]); > > rte_pktmbuf_reset(mbufs[idx]); > > idx++; > > /* fall-through */ > > case 2: > > - MBUF_RAW_ALLOC_CHECK(mbufs[idx]); > > + __rte_mbuf_raw_sanity_check(mbufs[idx]); > > rte_pktmbuf_reset(mbufs[idx]); > > idx++; > > /* fall-through */ > > case 1: > > - MBUF_RAW_ALLOC_CHECK(mbufs[idx]); > > + __rte_mbuf_raw_sanity_check(mbufs[idx]); > > rte_pktmbuf_reset(mbufs[idx]); > > idx++; > > /* fall-through */ > > diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf= _core.h > > index 8cd7137ac..4ac5609e3 100644 > > --- a/lib/librte_mbuf/rte_mbuf_core.h > > +++ b/lib/librte_mbuf/rte_mbuf_core.h > > @@ -272,7 +272,7 @@ extern "C" { > > * mbuf 'vlan_tci' & 'vlan_tci_outer' must be valid when this flag is = set. > > */ > > #define PKT_TX_QINQ (1ULL << 49) > > -/* this old name is deprecated */ > > +/** This old name is deprecated. */ > > #define PKT_TX_QINQ_PKT PKT_TX_QINQ > > =20 > > /** > > @@ -686,7 +686,7 @@ struct rte_mbuf_ext_shared_info { > > }; > > }; > > =20 > > -/**< Maximum number of nb_segs allowed. */ > > +/** Maximum number of nb_segs allowed. */ > > #define RTE_MBUF_MAX_NB_SEGS UINT16_MAX > > =20 > > /** > > @@ -714,7 +714,10 @@ struct rte_mbuf_ext_shared_info { > > #define RTE_MBUF_DIRECT(mb) \ > > (!((mb)->ol_flags & (IND_ATTACHED_MBUF | EXT_ATTACHED_MBUF))) > > =20 > > -#define MBUF_INVALID_PORT UINT16_MAX > > +/** NULL value for the uint16_t port type. */ > > +#define RTE_MBUF_PORT_INVALID UINT16_MAX >=20 > I don't really like talking about "NULL". What do you think instead of > this wording? >=20 > /** Uninitialized or unspecified port */ >=20 > > +/** For backwards compatibility. */ > > +#define MBUF_INVALID_PORT RTE_MBUF_PORT_INVALID > > =20 > > /** > > * A macro that points to an offset into the data in the mbuf. >=20 > Thanks, > Olivier >=20