Re: [dpdk-dev] [PATCH v6 3/3] ixgbe: Add LRO support

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

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Friday, March 13, 2015 9:08 AM
> To: Vlad Zolotarov; Ananyev, Konstantin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 3/3] ixgbe: Add LRO support
> 
> Hi Vlad,
> 
> On 03/11/2015 05:54 PM, Vlad Zolotarov wrote:
> >>>> About the existing RX/TX functions and PPC support:
> >>>> Note that all of them were created before PPC support for DPDK was
> >>>> introduced.
> >>>> At that moment only IA was supported.
> >>>> That's why in some places where you would expect to see 'mb()' there
> >>>> are 'volatile' and/or ' rte_compiler_barrier' instead.
> >>>> Why all that places wasn't updated when PPC support was added -
> >>>> that's another question.
> >>>>   From my understanding - with current implementation some of DPDK
> >>>> PMDs RX/TX functions and  rte_ring wouldn't work correctly
> >>> on PPC.
> >>>> So, I suppose we need to decide for ourselves - do we really want to
> >>>> support PPC and other architectures with non-IA memory
> >>> model or not?
> >>>> If not, then I think we don't need any mb()s inside recv_pkts_lro()
> >>>> - just rte_compiler_barrier seems enough, and no point to
> >>> complain about
> >>>> it in comments.
> >>>> If yes - then why to introduce a new function with a known potential
> >>>> bug?
> >>> In order to introduce a new function with the proper implementation or
> >>> to fix any other places with the similar weakness I would need a proper
> >>> tools like a proper platform-dependent barrier-macros similar to
> >>> smp_Xmb() Linux macros that reduce to a compiler barrier where
> >>> appropriate or to a proper memory fence where needed.
> >> I understand that.
> >> Let's add new macro for that: rte_smp_Xmb() or something,
> >> so it would be just rte_compiler_barrier() for x86 and a proper mb()
> >> for PPC.
> >
> > There was an idea to use the C11 built-in memory barriers. I suggest we
> > open a separate discussion about that and add these and the appropriate
> > fixes in a separate series. There are quite a few places to fix anyway,
> > which are currently broken on PPC so this patch doesn't make things any
> > worse. However adding a new memory barrier doesn't belong to an LRO
> > functionality and thus to this series.
> 
> This is an interesting discussion. Just for reference, I submitted a
> patch on this topic but it was probably too early as only Intel
> architecture was supported at that time.
> 
> See http://dpdk.org/ml/archives/dev/2014-May/002597.html

I do remember that conversation :)
At that moment, as nothing except IA wasn't supported, I feel it was not needed.
Though now, if we do want to support PPC and other architectures with weak memory model,
I think we do need to introduce some platform dependent set of Xmb() macros.
See http://dpdk.org/ml/archives/dev/2014-October/006729.html

Actually while thinking about it once again:
Is there any good use for rte_compiler_barrier() for PPC memory model?
I can't think about any.
So I wonder can't we just make for PPC:
 #define rte_compiler_barrier    rte_mb
While keeping it as it is for IA.
Would save us from searching/replacing though all the code.

 Konstantin



> 
> Regards,
> Olivier