Re: [dpdk-dev] rte_mbuf.next in 2nd cacheline

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

On Mon, Jun 15, 2015 at 01:47:26PM -0700, Stephen Hemminger wrote:
> On Mon, 15 Jun 2015 18:34:13 +0000
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> 
> > 
> > 
> > > -----Original Message-----
> > > From: Richardson, Bruce
> > > Sent: Monday, June 15, 2015 5:23 PM
> > > To: Ananyev, Konstantin
> > > Cc: Olivier MATZ; dev@dpdk.org; Damjan Marion (damarion)
> > > Subject: Re: [dpdk-dev] rte_mbuf.next in 2nd cacheline
> > > 
> > > On Mon, Jun 15, 2015 at 05:10:44PM +0100, Ananyev, Konstantin wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Richardson, Bruce
> > > > > Sent: Monday, June 15, 2015 5:02 PM
> > > > > To: Ananyev, Konstantin
> > > > > Cc: Olivier MATZ; dev@dpdk.org; Damjan Marion (damarion)
> > > > > Subject: Re: [dpdk-dev] rte_mbuf.next in 2nd cacheline
> > > > >
> > > > > On Mon, Jun 15, 2015 at 04:59:55PM +0100, Ananyev, Konstantin wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > As I can see, vector TX is the only one that calls __rte_pktmbuf_prefree_seg() directly.
> > > > > > All others use rte_pktmbuf_free_seg(), that does ' m->next = NULL' anyway.
> > > > > > For vector TX - yes, need to verify that it would not introduce a slowdown.
> > > > > > Konstantin
> > > > > >
> > > > >
> > > > > But if the function is only directly called from one place, and that doesn't
> > > > > have a problem, why would we bother making any change at all?
> > > >
> > > >
> > > > For future usages?
> > > > But sure, if you believe that we can safely remove 'm->next = NULL' at RX path,
> > > > without any changes in the __rte_pktmbuf_prefree_seg() -
> > > > that seems fine to me.
> > > > Konstantin
> > > >
> > > 
> > > If we find it's not safe, we can add in the change to __rte_pktmbuf_prefree_seg
> > > as you suggest.
> > > 
> > > One other question: based on this, do you think it's safe to also remove the
> > > assignment to NULL from the pktmbuf_alloc function? I suspect it should be safe, and
> > > that should help any traffic-generator type applications that use that function
> > > extensively.
> > 
> > So it will be setup to NULL, either by:
> > - mbuf constructor.
> > - TX full-path free descriptors code.
> > - upper layer code that uses 'next' pointer explicitly.
> > ?
> > 
> > I can't come-up with some breakage scenario off-hand.
> > But that means that we have to should avoid resetting tx_offload in pktmbuf_alloc() too, right?
> > Otherwise there probably wouldn't be any real gain.
> > Konstantin
> > 
> > > 
> > > /Bruce
> > 
> > 
> 
> The issue is when mbuf is used once by something and it sets it to non-NULL
> packet is sent then freed.
> then the free packet is picked up by one of the drivers using "fast path" Rx code.
> 
> IMHO the "fast path" Rx code has to go away. All the special allocation code
> should call rte_pktmuf_reset and then only set the fields that are special
> for the received packet.  The compiler should be able to remove the redundant
> stores, and it would prevent bugs I am seeing where some fields are not set
> in some drivers.

Is the better option here not to just fix the drivers rather than making the
fast-path RX code go away? Can you give us a report out on what fields are not
getting correctly set, and when using what drivers, and we can look and see 
where our gaps in coverage are?

/Bruce