Re: [dpdk-dev] [PATCH] ring: relax alignment constraint on ring structure

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Wed, 11 Apr 2018 00:33:14 +0000
> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: Olivier Matz <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>  "Richardson, Bruce" <bruce.richardson@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] ring: relax alignment constraint on ring
>  structure
> 

Hi Konstantin,

> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Friday, April 6, 2018 2:26 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Cc: Olivier Matz <olivier.matz@6wind.com>; dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] ring: relax alignment constraint on ring structure
> > 
> > -----Original Message-----
> > 
> > Hi Konstantin,
> > 
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > > Sent: Thursday, April 5, 2018 9:02 AM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > > Cc: Olivier Matz <olivier.matz@6wind.com>; dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
> > > > Subject: Re: [dpdk-dev] [PATCH] ring: relax alignment constraint on ring structure
> > > >
> > > > -----Original Message-----
> > > > > Date: Wed, 4 Apr 2018 23:38:41 +0000
> > > > > From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> > > > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Olivier Matz
> > > > >  <olivier.matz@6wind.com>
> > > > > CC: "dev@dpdk.org" <dev@dpdk.org>, "Richardson, Bruce"
> > > > >  <bruce.richardson@intel.com>
> > > > > Subject: RE: [dpdk-dev] [PATCH] ring: relax alignment constraint on ring
> > > > >  structure
> > > > >
> > > > > Hi lads,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > > > > Sent: Tuesday, April 3, 2018 5:43 PM
> > > > > > To: Olivier Matz <olivier.matz@6wind.com>
> > > > > > Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> > > > > > Subject: Re: [dpdk-dev] [PATCH] ring: relax alignment constraint on ring structure
> > > > > >
> > > > > > -----Original Message-----
> > > > > > > Date: Tue, 3 Apr 2018 17:56:01 +0200
> > > > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > > > CC: dev@dpdk.org, konstantin.ananyev@intel.com, bruce.richardson@intel.com
> > > > > > > Subject: Re: [dpdk-dev] [PATCH] ring: relax alignment constraint on ring
> > > > > > >  structure
> > > > > > > User-Agent: NeoMutt/20170113 (1.7.2)
> > > > > > >
> > > > > > > On Tue, Apr 03, 2018 at 09:07:04PM +0530, Jerin Jacob wrote:
> > > > > > > > -----Original Message-----
> > > > > > > > > Date: Tue, 3 Apr 2018 17:25:17 +0200
> > > > > > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > > > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > > > > > CC: dev@dpdk.org, konstantin.ananyev@intel.com, bruce.richardson@intel.com
> > > > > > > > > Subject: Re: [dpdk-dev] [PATCH] ring: relax alignment constraint on ring
> > > > > > > > >  structure
> > > > > > > > > User-Agent: NeoMutt/20170113 (1.7.2)
> > > > > > > > >
> > > > > > > > > On Tue, Apr 03, 2018 at 08:37:23PM +0530, Jerin Jacob wrote:
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > > Date: Tue, 3 Apr 2018 15:26:44 +0200
> > > > > > > > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > > > > > > To: dev@dpdk.org
> > > > > > > > > > > Subject: [dpdk-dev] [PATCH] ring: relax alignment constraint on ring
> > > > > > > > > > >  structure
> > > > > > > > > > > X-Mailer: git-send-email 2.11.0
> > > > > > > > > > >
> > > > > > > > > > > The initial objective of
> > > > > > > > > > > commit d9f0d3a1ffd4 ("ring: remove split cacheline build setting")
> > > > > > > > > > > was to add an empty cache line betwee, the producer and consumer
> > > > > > > > > > > data (on platform with cache line size = 64B), preventing from
> > > > > > > > > > > having them on adjacent cache lines.
> > > > > > > > > > >
> > > > > > > > > > > Following discussion on the mailing list, it appears that this
> > > > > > > > > > > also imposes an alignment constraint that is not required.
> > > > > > > > > > >
> > > > > > > > > > > This patch removes the extra alignment constraint and adds the
> > > > > > > > > > > empty cache lines using padding fields in the structure. The
> > > > > > > > > > > size of rte_ring structure and the offset of the fields remain
> > > > > > > > > > > the same on platforms with cache line size = 64B:
> > > > > > > > > > >
> > > > > > > > > > >   rte_ring = 384
> > > > > > > > > > >   rte_ring.name = 0
> > > > > > > > > > >   rte_ring.flags = 32
> > > > > > > > > > >   rte_ring.memzone = 40
> > > > > > > > > > >   rte_ring.size = 48
> > > > > > > > > > >   rte_ring.mask = 52
> > > > > > > > > > >   rte_ring.prod = 128
> > > > > > > > > > >   rte_ring.cons = 256
> > > > > > > > > > >
> > > > > > > > > > > But it has an impact on platform where cache line size is 128B:
> > > > > > > > > > >
> > > > > > > > > > >   rte_ring = 384        -> 768
> > > > > > > > > > >   rte_ring.name = 0
> > > > > > > > > > >   rte_ring.flags = 32
> > > > > > > > > > >   rte_ring.memzone = 40
> > > > > > > > > > >   rte_ring.size = 48
> > > > > > > > > > >   rte_ring.mask = 52
> > > > > > > > > > >   rte_ring.prod = 128   -> 256
> > > > > > > > > > >   rte_ring.cons = 256   -> 512
> > > > > > > > > >
> > > > > > > > > > Are we leaving TWO cacheline to make sure, HW prefetch don't load
> > > > > > > > > > the adjust cacheline(consumer)?
> > > > > > > > > >
> > > > > > > > > > If so, Will it have impact on those machine where it is 128B Cache line
> > > > > > > > > > and the HW prefetcher is not loading the next caching explicitly. Right?
> > > > > > > > >
> > > > > > > > > The impact on machines that have a 128B cache line is that an unused
> > > > > > > > > cache line will be added between the producer and consumer data. I
> > > > > > > > > expect that the impact is positive in case there is a hw prefetcher, and
> > > > > > > > > null in case there is no such prefetcher.
> > > > > > > >
> > > > > > > > It is not NULL, Right? You are loosing 256B for each ring.
> > > > > > >
> > > > > > > Is it really that important?
> > > > > >
> > > > > > Pipeline or eventdev SW cases there could more rings in the system.
> > > > > > I don't see any downside of having config option which is enabled
> > > > > > default.
> > > > > >
> > > > > > In my view, such config options are good, as in embedded usecases, customers
> > > > > > can really fine tune the target for the need. In server usecases, let the default
> > > > > > of option be enabled, no harm.
> > > > >
> > > > > But that would mean we have to maintain two layouts for the rte_ring structure.
> > > >
> > > > Is there any downside of having two configurable layout? meaning, we are not
> > > > transferring rte_ring structure over network etc(ie no interoperability
> > > > issue). Does it really matter? May I am missing something here.
> > >
> > > My concern about potential compatibility problems we are introducing -
> > > library build with 'y', while app wit 'n', or visa-versa.
> > 
> > Got it.
> > 
> > > I wonder are there really a lot of users who would be interested in such savings?
> > > Could it happen that this new option would sit here unused and untested?
> > 
> > OK. Fair enough. I have no objections for Olivier patch.
> > 
> > As a suggestion, may be we can move "char name[RTE_MEMZONE_NAMESIZE]" in the
> > struct rte_ring in place of " empty cacheline" to save 32B. No strong option
> > though.
> 
> That sounds like a good idea to me...
> But I suppose in that case we need to move to that empty cacheline all fields that precede prod?

Even though those fields are read only in fastpath,I suppose moving all
the fields(used in fast path) after prod, prefetch _cons_ cache line in cross
CPU case.

I think, following comment can be addressed in code as it is an ABI change.
        /*
         * Note: this field kept the RTE_MEMZONE_NAMESIZE size due to
         * ABI
         * compatibility requirements, it could be changed to
         * RTE_RING_NAMESIZE
         * next time the ABI changes
         */
        char name[RTE_MEMZONE_NAMESIZE] __rte_cache_aligned; /**< Name of the ring. */


> Otherwise there will be not much advantage in such move.
> 
>