DPDK patches and discussions
 help / color / mirror / Atom feed
From: Slava Ovsiienko <viacheslavo@mellanox.com>
To: Olivier Matz <olivier.matz@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Matan Azrad <matan@mellanox.com>,
	Raslan Darawsheh <rasland@mellanox.com>,
	Ori Kam <orika@mellanox.com>,
	Shahaf Shuler <shahafs@mellanox.com>,
	"stephen@networkplumber.org" <stephen@networkplumber.org>
Subject: Re: [dpdk-dev] [PATCH v3 1/4] mbuf: detach mbuf with pinned external buffer
Date: Wed, 15 Jan 2020 12:52:13 +0000	[thread overview]
Message-ID: <AM4PR05MB32654D9B4F6E3B01CAAFF3B0D2370@AM4PR05MB3265.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20200114152713.GS22738@platinum>

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Tuesday, January 14, 2020 17:27
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; stephen@networkplumber.org
> Subject: Re: [PATCH v3 1/4] mbuf: detach mbuf with pinned external buffer
> 
> Hi Viacheslav,
> 
> Please see some comments below.
> 
> On Tue, Jan 14, 2020 at 09:15:02AM +0000, Viacheslav Ovsiienko wrote:
> > Update detach routine to check the mbuf pool type.
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 64
> > +++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 60 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 219b110..8f486af 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -306,6 +306,46 @@ struct rte_pktmbuf_pool_private {
> >  	uint32_t flags; /**< reserved for future use. */  };
> >
> > +/**
> > + * When set pktmbuf mempool will hold only mbufs with pinned external
> > + * buffer. The external buffer will be attached on the mbuf at the
> > + * memory pool creation and will never be detached by the mbuf free calls.
> > + * mbuf should not contain any room for data after the mbuf structure.
> > + */
> > +#define RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF (1 << 0)
> 
> Out of curiosity, why using a pool flag instead of flagging the mbufs?
> The reason I see is that adding a new m->flag would impact places where we
> copy or reset the flags (it should be excluded). Is there another reason?
> 
Can we introduce the new static flag for mbuf?
Yes, there is some problem - there are many places in DPDK where the flags
in new allocated mbufs are disregarded (ol_flags field is just set to zero).
So, any flag set on allocation (even static, dynamic one is not possible to handle at all)
would get lost. We could fix it in new application (this feature is addressed to the
new ones) and PMDs supporting this, the question is whether we are allowed to
define the new mbuf static (in meaning not dynamic) flag.

> > +/**
> > + * Returns TRUE if given mbuf has an pinned external buffer, or FALSE
> > + * otherwise. The pinned external buffer is allocated at pool
> > +creation
> > + * time and should not be freed.
> > + *
> > + * External buffer is a user-provided anonymous buffer.
> > + */
> > +#ifdef ALLOW_EXPERIMENTAL_API
> > +#define RTE_MBUF_HAS_PINNED_EXTBUF(mb)
> rte_mbuf_has_pinned_extbuf(mb)
> > +#else #define RTE_MBUF_HAS_PINNED_EXTBUF(mb) false #endif
> 
> I suppose you added these lines because the compilation was broken after
> introducing the new __rte_experimental API, which is called from detach().
> 
> I find a bit strange that we require to do this. I don't see what would be
> broken without the ifdef: an application compiled for 19.11 cannot use the
> pinned-ext-buf feature (because it did not exist), so the modification looks
> safe to me.
Without ifdef compilation fails if there is no experimental API usage configured.

> 
> > +
> > +__rte_experimental
> > +static inline uint32_t
> 
> I don't think uint32_t is really better than uint64_t. I agree with Stephen that
> bool is the preferred choice, however if it breaks compilation in some cases, I
> think int is better.
Yes, bool causes compilation issues (just including stdbool.h causes build failure),
and, for example bool  is reserved gcc keyword if AltiVec support is enabled.
uint32_t is the type of priv->flags. 

> 
> > +rte_mbuf_has_pinned_extbuf(const struct rte_mbuf *m) {
> > +	if (RTE_MBUF_HAS_EXTBUF(m)) {
> > +		/*
> > +		 * The mbuf has the external attached buffer,
> > +		 * we should check the type of the memory pool where
> > +		 * the mbuf was allocated from.
> > +		 */
> > +		struct rte_pktmbuf_pool_private *priv =
> > +			(struct rte_pktmbuf_pool_private *)
> > +				rte_mempool_get_priv(m->pool);
> > +
> > +		return priv->flags &
> RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF;
> 
> What about introducing a rte_pktmbuf_priv_flags() function, on the same
> model than rte_pktmbuf_priv_size() or rte_pktmbuf_data_room_size()?

Nice idea, thanks. I think this routine can be not experimental and we would
get rid of ifdef and other stuff.

> 
> 
> > +	}
> > +	return 0;
> > +}
> > +
> >  #ifdef RTE_LIBRTE_MBUF_DEBUG
> >
> >  /**  check mbuf type in debug mode */ @@ -571,7 +611,8 @@ static
> > inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
> > static __rte_always_inline void  rte_mbuf_raw_free(struct rte_mbuf *m)
> > {
> > -	RTE_ASSERT(RTE_MBUF_DIRECT(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);
> > @@ -1141,11 +1182,26 @@ static inline void rte_pktmbuf_detach(struct
> rte_mbuf *m)
> >  	uint32_t mbuf_size, buf_len;
> >  	uint16_t priv_size;
> >
> > -	if (RTE_MBUF_HAS_EXTBUF(m))
> > +	if (RTE_MBUF_HAS_EXTBUF(m)) {
> > +		/*
> > +		 * The mbuf has the external attached buffed,
> > +		 * we should check the type of the memory pool where
> > +		 * the mbuf was allocated from to detect the pinned
> > +		 * external buffer.
> > +		 */
> > +		struct rte_pktmbuf_pool_private *priv =
> > +			(struct rte_pktmbuf_pool_private *)
> > +				rte_mempool_get_priv(mp);
> > +
> 
> It could be rte_pktmbuf_priv_flags() as said above.
> 
> > +		if (priv->flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) {
> > +			RTE_ASSERT(m->shinfo == NULL);
> > +			m->ol_flags = EXT_ATTACHED_MBUF;
> > +			return;
> > +		}
> 
> I think it is not possible to have m->shinfo == NULL (this comment is also
> related to next patch, because shinfo init is done there). If you try to clone a
> mbuf that comes from an ext-pinned pool, it will crash. Here is the code from
> attach():
> 
> 	static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct
> rte_mbuf *m)
> 	{
> 	        RTE_ASSERT(RTE_MBUF_DIRECT(mi) &&
> 	            rte_mbuf_refcnt_read(mi) == 1);
> 
> 	        if (RTE_MBUF_HAS_EXTBUF(m)) {
> 	                rte_mbuf_ext_refcnt_update(m->shinfo, 1); << HERE
> 	                mi->ol_flags = m->ol_flags;
> 	                mi->shinfo = m->shinfo;
> 		...
> 
> The 2 alternatives I see are:
> 
> - do not allow to clone these mbufs, but today there is no return value
>   to attach() functions, so there is no way to know if it failed or not
> 
> - manage shinfo to support clones
> 
> I think just ignoring the rte_mbuf_ext_refcnt_update() if shinfo == NULL is not
> an option, because if could result in recycling the extbuf while a clone still
> references it.
> 
> 
The clone for the mbufs with pinned buffers is not supposed at all, this was
chosen as development precondition. We can't touch the buf_adr/iova_addr fields in mbuf,
because there is no way to restore these pointers (nomore fixed offset between mbuf and data  buffer),
so rte_mbuf_detach() would be not operational at all.

Also, we can't deduce the mbuf base address from data buffer address. 
We could add RTE_ASSERT to prevent attaching (to) mbufs with pinned data, what do
you think?

> >  		__rte_pktmbuf_free_extbuf(m);
> > -	else
> > +	} else {
> >  		__rte_pktmbuf_free_direct(m);
> > -
> > +	}
> >  	priv_size = rte_pktmbuf_priv_size(mp);
> >  	mbuf_size = (uint32_t)(sizeof(struct rte_mbuf) + priv_size);
> >  	buf_len = rte_pktmbuf_data_room_size(mp);
> > --
> > 1.8.3.1
> >

  reply	other threads:[~2020-01-15 12:52 UTC|newest]

Thread overview: 77+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-18  9:50 [dpdk-dev] [RFC v20.20] mbuf: introduce pktmbuf pool with pinned external buffers Shahaf Shuler
2019-11-18 16:09 ` Stephen Hemminger
2020-01-10 17:56 ` [dpdk-dev] [PATCH 0/4] " Viacheslav Ovsiienko
2020-01-10 17:56   ` [dpdk-dev] [PATCH 1/4] mbuf: detach mbuf with pinned external buffer Viacheslav Ovsiienko
2020-01-10 18:23     ` Stephen Hemminger
2020-01-13 17:07       ` Slava Ovsiienko
2020-01-14  7:19       ` Slava Ovsiienko
2020-01-10 17:57   ` [dpdk-dev] [PATCH 2/4] mbuf: create packet pool with external memory buffers Viacheslav Ovsiienko
2020-01-10 17:57   ` [dpdk-dev] [PATCH 3/4] app/testpmd: add mempool with external data buffers Viacheslav Ovsiienko
2020-01-10 17:57   ` [dpdk-dev] [PATCH 4/4] net/mlx5: allow use allocated mbuf with external buffer Viacheslav Ovsiienko
2020-01-14  7:49 ` [dpdk-dev] [PATCH v2 0/4] mbuf: introduce pktmbuf pool with pinned external buffers Viacheslav Ovsiienko
2020-01-14  7:49   ` [dpdk-dev] [PATCH v2 1/4] mbuf: detach mbuf with pinned external buffer Viacheslav Ovsiienko
2020-01-14  7:49   ` [dpdk-dev] [PATCH v2 2/4] mbuf: create packet pool with external memory buffers Viacheslav Ovsiienko
2020-01-14  7:49   ` [dpdk-dev] [PATCH v2 3/4] app/testpmd: add mempool with external data buffers Viacheslav Ovsiienko
2020-01-14  7:49   ` [dpdk-dev] [PATCH v2 4/4] net/mlx5: allow use allocated mbuf with external buffer Viacheslav Ovsiienko
2020-01-14  9:15 ` [dpdk-dev] [PATCH v3 0/4] mbuf: detach mbuf with pinned " Viacheslav Ovsiienko
2020-01-14  9:15   ` [dpdk-dev] [PATCH v3 1/4] " Viacheslav Ovsiienko
2020-01-14 15:27     ` Olivier Matz
2020-01-15 12:52       ` Slava Ovsiienko [this message]
2020-01-14 15:50     ` Stephen Hemminger
2020-01-14  9:15   ` [dpdk-dev] [PATCH v3 2/4] mbuf: create packet pool with external memory buffers Viacheslav Ovsiienko
2020-01-14 16:04     ` Olivier Matz
2020-01-15 18:13       ` Slava Ovsiienko
2020-01-14  9:15   ` [dpdk-dev] [PATCH v3 3/4] app/testpmd: add mempool with external data buffers Viacheslav Ovsiienko
2020-01-14  9:15   ` [dpdk-dev] [PATCH v3 4/4] net/mlx5: allow use allocated mbuf with external buffer Viacheslav Ovsiienko
2020-01-16 13:04 ` [dpdk-dev] [PATCH v4 0/5] mbuf: detach mbuf with pinned " Viacheslav Ovsiienko
2020-01-16 13:04   ` [dpdk-dev] [PATCH v4 1/5] mbuf: introduce routine to get private mbuf pool flags Viacheslav Ovsiienko
2020-01-20 12:16     ` Olivier Matz
2020-01-16 13:04   ` [dpdk-dev] [PATCH v4 2/5] mbuf: detach mbuf with pinned external buffer Viacheslav Ovsiienko
2020-01-20 13:56     ` Olivier Matz
2020-01-20 15:41       ` Slava Ovsiienko
2020-01-20 16:17         ` Olivier Matz
2020-01-16 13:04   ` [dpdk-dev] [PATCH v4 3/5] mbuf: create packet pool with external memory buffers Viacheslav Ovsiienko
2020-01-20 13:59     ` Olivier Matz
2020-01-20 17:33       ` Slava Ovsiienko
2020-01-16 13:04   ` [dpdk-dev] [PATCH v4 4/5] app/testpmd: add mempool with external data buffers Viacheslav Ovsiienko
2020-01-20 14:11     ` Olivier Matz
2020-01-16 13:04   ` [dpdk-dev] [PATCH v4 5/5] net/mlx5: allow use allocated mbuf with external buffer Viacheslav Ovsiienko
2020-01-20 17:23 ` [dpdk-dev] [PATCH v5 0/5] mbuf: detach mbuf with pinned " Viacheslav Ovsiienko
2020-01-20 17:23   ` [dpdk-dev] [PATCH v5 1/5] mbuf: introduce routine to get private mbuf pool flags Viacheslav Ovsiienko
2020-01-20 20:43     ` Stephen Hemminger
2020-01-20 22:52       ` Thomas Monjalon
2020-01-21  6:48       ` Slava Ovsiienko
2020-01-21  8:00       ` Slava Ovsiienko
2020-01-21  8:14         ` Olivier Matz
2020-01-21  8:23           ` Slava Ovsiienko
2020-01-21  9:13             ` Slava Ovsiienko
2020-01-21 14:01               ` Olivier Matz
2020-01-21 16:21                 ` Stephen Hemminger
2020-01-20 17:23   ` [dpdk-dev] [PATCH v5 2/5] mbuf: detach mbuf with pinned external buffer Viacheslav Ovsiienko
2020-01-20 17:40     ` Olivier Matz
2020-01-20 17:23   ` [dpdk-dev] [PATCH v5 3/5] mbuf: create packet pool with external memory buffers Viacheslav Ovsiienko
2020-01-20 17:46     ` Olivier Matz
2020-01-20 17:23   ` [dpdk-dev] [PATCH v5 4/5] app/testpmd: add mempool with external data buffers Viacheslav Ovsiienko
2020-01-20 17:23   ` [dpdk-dev] [PATCH v5 5/5] net/mlx5: allow use allocated mbuf with external buffer Viacheslav Ovsiienko
2020-01-20 17:30   ` [dpdk-dev] [PATCH v5 0/5] mbuf: detach mbuf with pinned " Slava Ovsiienko
2020-01-20 17:41     ` Olivier Matz
2020-01-20 19:16 ` [dpdk-dev] [PATCH v6 " Viacheslav Ovsiienko
2020-01-20 19:16   ` [dpdk-dev] [PATCH v6 1/5] mbuf: introduce routine to get private mbuf pool flags Viacheslav Ovsiienko
2020-01-20 19:16   ` [dpdk-dev] [PATCH v6 2/5] mbuf: detach mbuf with pinned external buffer Viacheslav Ovsiienko
2023-12-06 10:55     ` [dpdk-dev] [PATCH v6 2/5] mbuf: detach mbuf with pinned externalbuffer Morten Brørup
2020-01-20 19:16   ` [dpdk-dev] [PATCH v6 3/5] mbuf: create packet pool with external memory buffers Viacheslav Ovsiienko
2020-01-20 20:48     ` Stephen Hemminger
2020-01-21  7:04       ` Slava Ovsiienko
2020-01-20 19:16   ` [dpdk-dev] [PATCH v6 4/5] app/testpmd: add mempool with external data buffers Viacheslav Ovsiienko
2020-01-20 19:16   ` [dpdk-dev] [PATCH v6 5/5] net/mlx5: allow use allocated mbuf with external buffer Viacheslav Ovsiienko
2020-01-20 22:55   ` [dpdk-dev] [PATCH v6 0/5] mbuf: detach mbuf with pinned " Thomas Monjalon
2020-01-22  8:50 ` [dpdk-dev] [PATCH] mbuf: fix pinned memory free routine style issue Viacheslav Ovsiienko
2020-02-06  9:46   ` Olivier Matz
2020-02-06 14:26     ` Thomas Monjalon
2020-01-24 20:25 ` [dpdk-dev] [PATCH] app/test: add test for mbuf with pinned external buffer Viacheslav Ovsiienko
2020-01-26 10:53   ` Slava Ovsiienko
2020-02-06  8:17   ` Olivier Matz
2020-02-06  8:24     ` Slava Ovsiienko
2020-02-06  9:51       ` Slava Ovsiienko
2020-02-06  9:49   ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
2020-02-06 14:43     ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM4PR05MB32654D9B4F6E3B01CAAFF3B0D2370@AM4PR05MB3265.eurprd05.prod.outlook.com \
    --to=viacheslavo@mellanox.com \
    --cc=dev@dpdk.org \
    --cc=matan@mellanox.com \
    --cc=olivier.matz@6wind.com \
    --cc=orika@mellanox.com \
    --cc=rasland@mellanox.com \
    --cc=shahafs@mellanox.com \
    --cc=stephen@networkplumber.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).