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 679D8A04BA; Wed, 7 Oct 2020 11:16:21 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B4E7C1B3C8; Wed, 7 Oct 2020 11:16:19 +0200 (CEST) Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id CBAC72B8C for ; Wed, 7 Oct 2020 11:16:16 +0200 (CEST) Received: by mail-wr1-f66.google.com with SMTP id w5so1270060wrp.8 for ; Wed, 07 Oct 2020 02:16:16 -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:content-transfer-encoding:in-reply-to :user-agent; bh=Ph6ydhu4w2T7AV1rBC9MWAR4gpximzn21wkYOqFYrE4=; b=GYEE7u7CoBNyezbxBM65EgohbOVDt+z742d+ORDZaoF8KWqCWqCduoG5Px+PDG6Wdc vLJhkCdI894Tp492d2RLJJc8I4wzvh1uHuTmaqxdWXx7UbNrzwR5Yf1zeu37RVoIDijp 0EHrap/luBIB59Ne+kocuayBGfJz8KxnKYPFYXCctrh0Yj4rgFaseNk927ebJLc0gJu6 pxvIG0wrn6oikB8UsJy8L14ED7oqaoKu7d5myVNa2GZUSuALQyhRlcUX5lQUHUozWDZp CrJQMuTJ73uRFF31VxaKvSzoRzRoso29Q5PN00D2T0dVTW+7SCaBlJlOZYE2SbewG92Q PSZQ== 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:content-transfer-encoding :in-reply-to:user-agent; bh=Ph6ydhu4w2T7AV1rBC9MWAR4gpximzn21wkYOqFYrE4=; b=V8R1gBIqQfhwFeKphX0E8WpVKDlacqmJDPSLfpE5xt4ThSRxnYDWAt2r6iPLFq+gRE giUrekWnRihy6s4TMpeLWmVEOjZ056n0c5PD9ylRTuMd4VwA7lFuO5dnR8Ois6BMr/4k yfhO+e8OGYuG+pZlGMddiTSAcTNnEsg7wMR+tzap45kMrBuAfS1ckvEpS+9gW0FQH+5e LoZWcNDJH6hbr0v81uycBVBryu1kx4bZ4tvW7pVi2AdHyfYxCpS/d436IJ6trhu1PZ5o ZjNi7soydWsKpKBHAsWtum9YWXNvGY9v75yrc+F6eV46y4xourwRBihHEA4fpQtP1Bbn 5xAg== X-Gm-Message-State: AOAM532W8/WyyK+h3dr+hN+tP5BJiS4N+CYRf3j5DwnmcPg6pOADxvNb hBi6enPOfYBnY1nzME2NFATSCg== X-Google-Smtp-Source: ABdhPJwKo8+adYA4wyT0zOoGpYprZJ/3PjLCVudtyU2McFH6puqbhOWeWYiSe2ddpm17UVAt32yOqg== X-Received: by 2002:adf:f3c4:: with SMTP id g4mr2549797wrp.207.1602062176295; Wed, 07 Oct 2020 02:16:16 -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 z67sm1894647wme.41.2020.10.07.02.16.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Oct 2020 02:16:15 -0700 (PDT) Date: Wed, 7 Oct 2020 11:16:14 +0200 From: Olivier Matz To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: thomas@monjalon.net, dev@dpdk.org Message-ID: <20201007091614.GO21395@platinum> References: <20200916104013.126362-1-mb@smartsharesystems.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200916104013.126362-1-mb@smartsharesystems.com> User-Agent: Mutt/1.10.1 (2018-07-13) 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, Thanks for this cleanup. Please see some comments below. On Wed, Sep 16, 2020 at 12:40:13PM +0200, Morten Brørup 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. > > Note: This patch does not touch the offload and attachment flags that are > also missing the RTE_ prefix. > > 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. > > 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. > > v2: > * Use RTE_MBUF_PORT_INVALID instead of MBUF_INVALID_PORT in rte_mbuf.c. > > 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 parameter > __rte_unused if none of them are defined. > > Signed-off-by: Morten Brørup > --- > 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(-) > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/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/ > > -* rawdev: The rawdev APIs which take a device-specific structure as > - parameter directly, or indirectly via a "private" pointer inside another > - 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. I think this change is not related. 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. > 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 = mp; > m->nb_segs = 1; > - m->port = MBUF_INVALID_PORT; > + m->port = RTE_MBUF_PORT_INVALID; > rte_mbuf_refcnt_set(m, 1); > m->next = NULL; > } > @@ -207,7 +207,7 @@ __rte_pktmbuf_init_extmem(struct rte_mempool *mp, > /* init some constant fields */ > m->pool = mp; > m->nb_segs = 1; > - m->port = MBUF_INVALID_PORT; > + m->port = RTE_MBUF_PORT_INVALID; > m->ol_flags = EXT_ATTACHED_MBUF; > rte_mbuf_refcnt_set(m, 1); > m->next = 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); > > -#define MBUF_RAW_ALLOC_CHECK(m) do { \ > - RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1); \ > - RTE_ASSERT((m)->next == NULL); \ > - RTE_ASSERT((m)->nb_segs == 1); \ > - __rte_mbuf_sanity_check(m, 0); \ > -} while (0) > +#if defined(RTE_ENABLE_ASSERT) || defined(RTE_LIBRTE_MBUF_DEBUG) I don't see why this #if is needed. Wouldn't it work to have only one function definition with the __rte_unused attribute? > +/** > + * 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=1, next=NULL, > + * nb_segs=1), as done by rte_pktmbuf_prefree_seg(). > + * Maybe indicate that these checks are only done when debug is on. > + * @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) == 1); > + RTE_ASSERT(m->next == NULL); > + RTE_ASSERT(m->nb_segs == 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) It looks that MBUF_RAW_ALLOC_CHECK() is also used in drivers/net/sfc, I think it should be updated too. > > /** > * Allocate an uninitialized mbuf from mempool *mp*. > @@ -586,7 +610,7 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp) > > if (rte_mempool_get(mp, (void **)&m) < 0) > return NULL; > - MBUF_RAW_ALLOC_CHECK(m); > + __rte_mbuf_raw_sanity_check(m); > return m; > } > > @@ -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) == 1); > - RTE_ASSERT(m->next == NULL); > - RTE_ASSERT(m->nb_segs == 1); > - __rte_mbuf_sanity_check(m, 0); > + __rte_mbuf_raw_sanity_check(m); > rte_mempool_put(m->pool, m); > } > > @@ -858,8 +879,6 @@ static inline void rte_pktmbuf_reset_headroom(struct 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 = NULL; > @@ -868,7 +887,7 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m) > m->vlan_tci = 0; > m->vlan_tci_outer = 0; > m->nb_segs = 1; > - m->port = MBUF_INVALID_PORT; > + m->port = RTE_MBUF_PORT_INVALID; > > m->ol_flags &= EXT_ATTACHED_MBUF; > m->packet_type = 0; > @@ -931,22 +950,22 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool, > switch (count % 4) { > case 0: > while (idx != 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 > > /** > @@ -686,7 +686,7 @@ struct rte_mbuf_ext_shared_info { > }; > }; > > -/**< Maximum number of nb_segs allowed. */ > +/** Maximum number of nb_segs allowed. */ > #define RTE_MBUF_MAX_NB_SEGS UINT16_MAX > > /** > @@ -714,7 +714,10 @@ struct rte_mbuf_ext_shared_info { > #define RTE_MBUF_DIRECT(mb) \ > (!((mb)->ol_flags & (IND_ATTACHED_MBUF | EXT_ATTACHED_MBUF))) > > -#define MBUF_INVALID_PORT UINT16_MAX > +/** NULL value for the uint16_t port type. */ > +#define RTE_MBUF_PORT_INVALID UINT16_MAX I don't really like talking about "NULL". What do you think instead of this wording? /** Uninitialized or unspecified port */ > +/** For backwards compatibility. */ > +#define MBUF_INVALID_PORT RTE_MBUF_PORT_INVALID > > /** > * A macro that points to an offset into the data in the mbuf. > -- > 2.17.1 > Thanks, Olivier