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 <shah...@mellanox.com>
> Signed-off-by: Viacheslav Ovsiienko <viachesl...@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?
> +/**
> + * 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.
> +
> +__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.
> +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()?
> + }
> + 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.
> __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
>
