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

["Ouyang, Changchun" <changchun.ouyang@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Friday, October 24, 2014 11:58 PM
> To: Richardson, Bruce
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep
> EXTERNAL_MBUF flag
> 
> 
> 
> > -----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)) {
> 

Currently seems good to me, too. But need more practice on it.

> 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.
> 
Not fully understand this. Konstantin, would you explain more?
Thanks
Changchun