Hi,
On Mon, Jan 20, 2020 at 03:41:10PM +0000, Slava Ovsiienko wrote:
> Hi, Olivier
>
> Thanks a lot for the thorough review.
> There are some answers to comments, please, see below.
>
> > >
> > > /**
> > > + * @internal version of rte_pktmbuf_detach() to be used on mbuf freeing.
> >
> > -version
> > +Version
> >
> > > + * For indirect and regular (not pinned) external mbufs the standard
> > > + * rte_pktmbuf is involved, for pinned external buffer mbufs the
> > > + special
> > > + * handling is performed:
> >
> > Sorry, it is not very clear to me, especially what "the standard
> > rte_pktmbuf is
> > involved" means.
>
> Sorry, it is mistype, should be read as "rte_pktmbuf_detach is invoked".
> >
> > > + *
> > > + * - return zero if reference counter in shinfo is one. It means
> > > + there is
> > > + * no more references to this pinned buffer and mbuf can be returned
> > > + to
> >
> > -references
> > +reference
> >
> > > + * the pool
> > > + *
> > > + * - otherwise (if reference counter is not one), decrement
> > > +reference
> > > + * counter and return non-zero value to prevent freeing the backing
> > > mbuf.
> > > + *
> > > + * Returns non zero if mbuf should not be freed.
> > > + */
> > > +static inline uint16_t __rte_pktmbuf_detach_on_free(struct rte_mbuf
> > > +*m)
> >
> > I think int would be better than uint16_t
> >
> > > +{
> > > + if (RTE_MBUF_HAS_EXTBUF(m)) {
> > > + uint32_t flags = rte_pktmbuf_priv_flags(m->pool);
> > > +
> > > + if (flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) {
> > > + struct rte_mbuf_ext_shared_info *shinfo;
> > > +
> > > + /* Clear flags, mbuf is being freed. */
> > > + m->ol_flags = EXT_ATTACHED_MBUF;
> > > + shinfo = m->shinfo;
> > > + /* Optimize for performance - do not dec/reinit */
> > > + if (likely(rte_mbuf_ext_refcnt_read(shinfo) == 1))
> > > + return 0;
> > > + /*
> > > + * Direct usage of add primitive to avoid
> > > + * duplication of comparing with one.
> > > + */
> > > + if (likely(rte_atomic16_add_return
> > > + (&shinfo->refcnt_atomic, -1)))
> > > + return 1;
> > > + /* Reinitialize counter before mbuf freeing. */
> > > + rte_mbuf_ext_refcnt_set(shinfo, 1);
> > > + return 0;
> > > + }
> > > + }
> > > + rte_pktmbuf_detach(m);
> > > + return 0;
> > > +}
> >
> > I don't think the API comment really reflects what is done in this
> > function. In
> > my understanding, the detach() operation does nothing on an extmem
> > pinned mbuf. So detach() is probably not the proper name.
> >
> > What about something like this instead:
> >
> > /* [...].
> > * assume m is pinned to external memory */ static inline int
> > __rte_pktmbuf_pinned_ext_buf_decref(struct rte_mbuf *m) {
> > struct rte_mbuf_ext_shared_info *shinfo;
> >
> > /* Clear flags, mbuf is being freed. */
> > m->ol_flags = EXT_ATTACHED_MBUF;
> > shinfo = m->shinfo;
> >
> > /* Optimize for performance - do not dec/reinit */
> > if (likely(rte_mbuf_ext_refcnt_read(shinfo) == 1))
> > return 0;
> >
> > /*
> > * Direct usage of add primitive to avoid
> > * duplication of comparing with one.
> > */
> > if (likely(rte_atomic16_add_return
> > (&shinfo->refcnt_atomic, -1)))
> > return 1;
> >
> > /* Reinitialize counter before mbuf freeing. */
> > rte_mbuf_ext_refcnt_set(shinfo, 1);
> > return 0;
> > }
> >
> > static __rte_always_inline struct rte_mbuf * rte_pktmbuf_prefree_seg(struct
> > rte_mbuf *m) {
> > __rte_mbuf_sanity_check(m, 0);
> >
> > if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> >
> > if (!RTE_MBUF_DIRECT(m))
> > if (!RTE_MBUF_HAS_PINNED_EXTBUF(m))
> > rte_pktmbuf_detach(m);
> > else if (__rte_pktmbuf_pinned_ext_buf_decref(m))
> > return NULL;
> > }
> > ...
> > ... (and same below) ...
> >
> >
> > (just quickly tested)
> >
> > The other advantage is that we don't call rte_pktmbuf_detach() where not
> > needed.
> Your proposal fetches the private flags for all indirect packets, including
> the ones
> with IND_ATTACHED_MBUF flags (not external), this extra fetch and check might
> affect
> the performance for indirect packets (and it does not matter for packets with
> external
> buffers). My approach updates the prefree routine for the packets with
> external buffers only, keeping intact the handling for all other mbuf types.
maybe just change the test to this?
if (!RTE_MBUF_HAS_EXTBUF(m) || !RTE_MBUF_HAS_PINNED_EXTBUF(m))
if you prefer, test can be moved in __rte_pktmbuf_pinned_ext_buf_decref():
if (!RTE_MBUF_HAS_EXTBUF(m) || !RTE_MBUF_HAS_PINNED_EXTBUF(m))
return 0;
But my preference would go to the 1st one.
The root of my comment was more about the naming, I don't think the
function should be something_detach() because it would not detach
anything in case of ext mem pinned buffer.
>
> >
> > > +
> > > +/**
> > > * Decrease reference counter and unlink a mbuf segment
> > > *
> > > * This function does the same than a free, except that it does not
> > > @@ -1198,7 +1277,8 @@ static inline void rte_pktmbuf_detach(struct
> > rte_mbuf *m)
> > > if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> > >
> > > if (!RTE_MBUF_DIRECT(m))
> > > - rte_pktmbuf_detach(m);
> > > + if (__rte_pktmbuf_detach_on_free(m))
> > > + return NULL;
> > >
> > > if (m->next != NULL) {
> > > m->next = NULL;
> > > @@ -1210,7 +1290,8 @@ static inline void rte_pktmbuf_detach(struct
> > rte_mbuf *m)
> > > } else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
> > >
> > > if (!RTE_MBUF_DIRECT(m))
> > > - rte_pktmbuf_detach(m);
> > > + if (__rte_pktmbuf_detach_on_free(m))
> > > + return NULL;
> > >
> > > if (m->next != NULL) {
> > > m->next = NULL;
> > > --
> > > 1.8.3.1
> > >
>
