Re: [dpdk-dev] [PATCH 2/2] Remove RTE_MBUF_REFCNT references

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Feb 18, 2015 at 11:14:42AM +0100, Olivier MATZ wrote:
> On 02/18/2015 11:00 AM, Bruce Richardson wrote:
> >On Wed, Feb 18, 2015 at 09:48:58AM +0000, Ananyev, Konstantin wrote:
> >>Hi lads,
> >>
> >>>-----Original Message-----
> >>>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> >>>Sent: Wednesday, February 18, 2015 9:36 AM
> >>>To: Olivier MATZ
> >>>Cc: dev@dpdk.org
> >>>Subject: Re: [dpdk-dev] [PATCH 2/2] Remove RTE_MBUF_REFCNT references
> >>>
> >>>On Wed, Feb 18, 2015 at 10:16:56AM +0100, Olivier MATZ wrote:
> >>>>Hi Sergio,
> >>>>
> >>>>On 02/16/2015 05:08 PM, Sergio Gonzalez Monroy wrote:
> >>>>>This patch removes all references to RTE_MBUF_REFCNT, setting the refcnt
> >>>>>field in the mbuf struct permanently.
> >>>>>
> >>>>>Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> >>>>
> >>>>I think removing the refcount compile option goes in the right
> >>>>direction. However, activating the refcount will break the applications
> >>>>that reserve a private zone in mbufs. This is due to the macros
> >>>>RTE_MBUF_TO_BADDR() and RTE_MBUF_FROM_BADDR() that suppose that
> >>>>the beginning of the mbuf is 128 bytes (sizeof mbuf) before the
> >>>>data buffer.
> >>>>
> >>>
> >>>While I understand how the macros make certain assumptions, how does activating
> >>>the refcnt specifically lead to the problems you describe? Could you explain
> >>>that part in a bit more detail?
> >>>
> >>>Thanks,
> >>>/Bruce
> >>>
> >>
> >>Olivier, I also don't understand your concern here.
> >>As I can see, that patch has nothing to do with RTE_MBUF_FROM_BADDR/ RTE_MBUF_FROM_BADDR macros.
> >>They are still there, for example rte_pktmbuf_detach() still uses it to restore mbuf's buf_addr.
> >>The only principal change here, is that we don't rely more  on RTE_MBUF_FROM_BADDR to determine,
> >>Is that indirect mbuf or not.
> >>Instead we use a special falg for that purpose:
> >>
> >>-#define RTE_MBUF_INDIRECT(mb)   (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb))
> >>+#define RTE_MBUF_INDIRECT(mb)   (mb->ol_flags & IND_ATTACHED_MBUF)
> >>
> >>BTW, Sergio as I said before, I think it should be:
> >>#define RTE_MBUF_INDIRECT(mb)   ((mb)->ol_flags & IND_ATTACHED_MBUF)
> >>
> >>Konstantin
> >>
> >>
> >>>>For RTE_MBUF_TO_BADDR(), it's relatively easy to replace it. The
> >>>>mbuf pool could store the size of the private size like it's done
> >>>>for mbp_priv->mbuf_data_room_size. Using rte_mempool_from_obj(m)
> >>>>or m->pool, we can retrieve the mbuf pool and this value, then
> >>>>compute the buffer address.
> >
> >Agreed, that makes sense.
> >
> >>>>
> >>>>For RTE_MBUF_FROM_BADDR(), it's more complex. We could ensure that
> >>>>a backpointer to the mbuf is always located before the data buffer,
> >>>>but it looks difficult to do.
> >
> >On the other hand, with the proposed refcnt change Sergio proposes, we no
> >longer use this macro in any of the built-in mbuf handling for freeing mbufs.
> >Does this need to be solved at anything other than the application level?
> 
> It's still used in __rte_pktmbuf_prefree_seg() to retrieve the
> parent mbuf (direct) from the indirect mbuf beeing freed.
>
Yes, my bad.
How was this managed before, since refcnt field seems to be necessary in order
to effectively manage indirect mbufs? Is this just the case that this is something
that never worked and that needs to be solved, or is it something that was 
working that this patch will now break?

Thanks,
/Bruce