Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Friday, October 24, 2014 4:43 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag
> 
> On Fri, Oct 24, 2014 at 01:34:58PM +0100, Bruce Richardson wrote:
> > On Fri, Oct 24, 2014 at 10:46:06AM +0000, Ananyev, Konstantin wrote:
> > > Hi Changchun,
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang Changchun
> > > > Sent: Friday, October 24, 2014 9:10 AM
> > > > To: dev@dpdk.org
> > > > Subject: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag
> > > >
> > > > Every pmd RX function need keep the EXTERNAL_MBUF flag
> > > > in mbuf.ol_flags, and can't overwrite it when filling ol_flags from
> > > > descriptor to mbuf, otherwise, it probably cause to crash when freeing a mbuf
> > > > and trying to freeing its attached external buffer, say, from guest space.
> > > >
> > >
> > > Don't really like the idea to put:
> > > mb->ol_flags = pkt_flags | (mb->ol_flags & EXTERNAL_MBUF);
> > > in each and every PMD from now on...
> > >
> > > From other side, it is probably not very good that RX functions update whole ol_flags, not only RX related part.
> > > Wonder can we reserve low 32bits of ol_flags for RX, and high 32bits for TX and generic stuff.
> > > So our ol_flags will look something like that:
> > >
> > > union {
> > > 	uint64_t ol_raw_flags;
> > > 	struct {
> > > 		uint32_t rx;
> > > 		uint32_t gen_tx;
> > > 	} ol_flags
> > > };
> > >
> > > And make all PMD RX functions to operate on rx part of the flags only:
> > > mb->ol_flags.rx = pkt_flags;
> > > ?
> > >
> > > Konstantin
> > >
> > I would tend to agree with this. Changchun, did you get to assess the
> > performance impact of making this change to the PMDs? I suspect that making
> > the changes to each PMD would impact performance, while Konstantin's
> > suggestion should eliminate that impact.
> > The downside there is that we are limiting the flexibility we have in
> > expanding beyond 32 RX flags and 24 TX flags. :-(
> >
> > /Bruce
> >
> 
> How about switching things about in terms of the flag. Instead of having to
> manage a flag across the baord to indicate if an mbuf is pointing to
> external memory, I think we should use the flag to indicate that an mbuf is
> attached to the memory space of another mbuf.
> 
> My reasons for suggesting this are:
> 1. Mbufs pointing to externally managed memory are not really the problem to
> be dealt with on free, since they can be handled the same as mbufs with the
> data pointer pointing internally, it's mbufs attached to other mbufs which
> are - so that's what we need to track using a flag.
> 2. Setting the flag to indicate an indirect mbuf should have no impact on
> the driver, as an mbuf that has just been allocated from mempool cannot be
> an indirect one.
> 3. The only place we would need to worry about such a flag is in the attach,
> detach and free mbuf functions - and on free we would simply need to replace
> the existing check for "md != m" with a new check for the new flag. It would
> be a contained change.
> 

Sounds good to me.
That's' definitely much better than my proposal.
Plus, if we'll stop to rely on:

  md = RTE_MBUF_FROM_BADDR(m->buf_addr);
  if (unlikely (md != m)) {

That will allow us to set  buf_addr to some other valid offset inside mbuf
and that fix an old problem with mbufs extra metadata (userdata) stored in the packet's headroom. 

Konstantin

> Thoughts?
> /Bruce