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

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

[Olivier Matz]

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

Link: http://dpdk.org/dev/patchwork/patch/25039/
Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---

I'm sending this patch to throw the discussion again, but since it
breaks the ABI on platform with cache lines = 128B, I think we should
follow the usual ABI breakage process.

If everybody agree, I'll send a notice and resend a similar patch after
17.08.

Olivier



 lib/librte_ring/rte_ring.h | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index 1beb781b4..135b83df0 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -116,14 +116,6 @@ enum rte_ring_queue_behavior {
 
 struct rte_memzone; /* forward declaration, so as not to require memzone.h */
 
-#if RTE_CACHE_LINE_SIZE < 128
-#define PROD_ALIGN (RTE_CACHE_LINE_SIZE * 2)
-#define CONS_ALIGN (RTE_CACHE_LINE_SIZE * 2)
-#else
-#define PROD_ALIGN RTE_CACHE_LINE_SIZE
-#define CONS_ALIGN RTE_CACHE_LINE_SIZE
-#endif
-
 /* structure to hold a pair of head/tail values and other metadata */
 struct rte_ring_headtail {
 	volatile uint32_t head;  /**< Prod/consumer head. */
@@ -155,11 +147,15 @@ struct rte_ring {
 	uint32_t mask;           /**< Mask (size-1) of ring. */
 	uint32_t capacity;       /**< Usable size of ring */
 
+	char pad0 __rte_cache_aligned; /**< empty cache line */
+
 	/** Ring producer status. */
-	struct rte_ring_headtail prod __rte_aligned(PROD_ALIGN);
+	struct rte_ring_headtail prod __rte_cache_aligned;
+	char pad1 __rte_cache_aligned; /**< empty cache line */
 
 	/** Ring consumer status. */
-	struct rte_ring_headtail cons __rte_aligned(CONS_ALIGN);
+	struct rte_ring_headtail cons __rte_cache_aligned;
+	char pad2 __rte_cache_aligned; /**< empty cache line */
 };
 
 #define RING_F_SP_ENQ 0x0001 /**< The default enqueue is "single-producer". */
-- 
2.11.0

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

[Olivier Matz]

Hi,

On Fri, 30 Jun 2017 16:26:09 +0200, Olivier Matz <olivier.matz@6wind.com> wrote:
> 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
> 
> Link: http://dpdk.org/dev/patchwork/patch/25039/
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> 
> I'm sending this patch to throw the discussion again, but since it
> breaks the ABI on platform with cache lines = 128B, I think we should
> follow the usual ABI breakage process.
> 
> If everybody agree, I'll send a notice and resend a similar patch after
> 17.08.
> 

If there is no comment, I'll send a deprecation notice in the
coming days.

Thanks,
Olivier

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

[Olivier Matz]

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

Link: http://dpdk.org/dev/patchwork/patch/25039/
Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 doc/guides/rel_notes/deprecation.rst   |  6 ------
 doc/guides/rel_notes/release_18_05.rst |  8 +++++++-
 lib/librte_ring/Makefile               |  2 +-
 lib/librte_ring/rte_ring.h             | 16 ++++++----------
 4 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 40448961a..84e153461 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -139,9 +139,3 @@ Deprecation Notices
   required the previous behavior can be configured using existing flow
   director APIs. There is no ABI/API break. This change will just remove a
   global configuration setting and require explicit configuration.
-
-* ring: The alignment constraints on the ring structure will be relaxed
-  to one cache line instead of two, and an empty cache line padding will
-  be added between the producer and consumer structures. The size of the
-  structure and the offset of the fields will remain the same on
-  platforms with 64B cache line, but will change on other platforms.
diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst
index 9cc77f893..4d0276f1d 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -86,6 +86,12 @@ ABI Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* ring: the alignment constraints on the ring structure has been relaxed
+  to one cache line instead of two, and an empty cache line padding is
+  added between the producer and consumer structures. The size of the
+  structure and the offset of the fields remains the same on platforms
+  with 64B cache line, but changes on other platforms.
+
 
 Removed Items
 -------------
@@ -176,7 +182,7 @@ The libraries prepended with a plus sign were incremented in this version.
      librte_power.so.1
      librte_rawdev.so.1
      librte_reorder.so.1
-     librte_ring.so.1
+   + librte_ring.so.2
      librte_sched.so.1
      librte_security.so.1
      librte_table.so.3
diff --git a/lib/librte_ring/Makefile b/lib/librte_ring/Makefile
index bde8907d6..21a36770d 100644
--- a/lib/librte_ring/Makefile
+++ b/lib/librte_ring/Makefile
@@ -11,7 +11,7 @@ LDLIBS += -lrte_eal
 
 EXPORT_MAP := rte_ring_version.map
 
-LIBABIVER := 1
+LIBABIVER := 2
 
 # all source are stored in SRCS-y
 SRCS-$(CONFIG_RTE_LIBRTE_RING) := rte_ring.c
diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index 253cdc96a..d3d3f7f97 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -62,14 +62,6 @@ enum rte_ring_queue_behavior {
 
 struct rte_memzone; /* forward declaration, so as not to require memzone.h */
 
-#if RTE_CACHE_LINE_SIZE < 128
-#define PROD_ALIGN (RTE_CACHE_LINE_SIZE * 2)
-#define CONS_ALIGN (RTE_CACHE_LINE_SIZE * 2)
-#else
-#define PROD_ALIGN RTE_CACHE_LINE_SIZE
-#define CONS_ALIGN RTE_CACHE_LINE_SIZE
-#endif
-
 /* structure to hold a pair of head/tail values and other metadata */
 struct rte_ring_headtail {
 	volatile uint32_t head;  /**< Prod/consumer head. */
@@ -101,11 +93,15 @@ struct rte_ring {
 	uint32_t mask;           /**< Mask (size-1) of ring. */
 	uint32_t capacity;       /**< Usable size of ring */
 
+	char pad0 __rte_cache_aligned; /**< empty cache line */
+
 	/** Ring producer status. */
-	struct rte_ring_headtail prod __rte_aligned(PROD_ALIGN);
+	struct rte_ring_headtail prod __rte_cache_aligned;
+	char pad1 __rte_cache_aligned; /**< empty cache line */
 
 	/** Ring consumer status. */
-	struct rte_ring_headtail cons __rte_aligned(CONS_ALIGN);
+	struct rte_ring_headtail cons __rte_cache_aligned;
+	char pad2 __rte_cache_aligned; /**< empty cache line */
 };
 
 #define RING_F_SP_ENQ 0x0001 /**< The default enqueue is "single-producer". */
-- 
2.11.0

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

[Jerin Jacob]

-----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

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?

>   rte_ring.cons = 256   -> 512
> 
> Link: http://dpdk.org/dev/patchwork/patch/25039/
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

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

[Olivier Matz]

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.

On machines with 64B cache line, this was already the case. It just
reduces the alignment constraint.

Olivier

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

[Jerin Jacob]

-----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.

> 
> On machines with 64B cache line, this was already the case. It just
> reduces the alignment constraint.

Not all the 64B CL machines will have HW prefetch.

I would recommend to add conditional compilation flags to express HW
prefetch enabled or not? based on that we can decide to reserve
the additional space. By default, in common config, HW prefetch can
be enabled so that it works for almost all cases.


> 
> Olivier

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

[Olivier Matz]

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?


> > On machines with 64B cache line, this was already the case. It just
> > reduces the alignment constraint.
> 
> Not all the 64B CL machines will have HW prefetch.
> 
> I would recommend to add conditional compilation flags to express HW
> prefetch enabled or not? based on that we can decide to reserve
> the additional space. By default, in common config, HW prefetch can
> be enabled so that it works for almost all cases.

The hw prefetcher can be enabled at runtime, so a compilation flag
does not seem to be a good idea. Moreover, changing this compilation
flag would change the ABI.

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

[Jerin Jacob]

-----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.

> 
> 
> > > On machines with 64B cache line, this was already the case. It just
> > > reduces the alignment constraint.
> > 
> > Not all the 64B CL machines will have HW prefetch.
> > 
> > I would recommend to add conditional compilation flags to express HW
> > prefetch enabled or not? based on that we can decide to reserve
> > the additional space. By default, in common config, HW prefetch can
> > be enabled so that it works for almost all cases.
> 
> The hw prefetcher can be enabled at runtime, so a compilation flag
> does not seem to be a good idea. Moreover, changing this compilation

On those Hardwares HW prefetch can be disabled at runtime, it is fine
with default config. I was taking about some low end ARM hardware which
does not have HW prefetch is not present at all.

> flag would change the ABI.

ABI is broken anyway, Right? due to size of the structure change.

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

[Ananyev, Konstantin]

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.
I am agree with Olivier here, might be saving 256B per ring is not worth such hassle.
Konstantin

> 
> >
> >
> > > > On machines with 64B cache line, this was already the case. It just
> > > > reduces the alignment constraint.
> > >
> > > Not all the 64B CL machines will have HW prefetch.
> > >
> > > I would recommend to add conditional compilation flags to express HW
> > > prefetch enabled or not? based on that we can decide to reserve
> > > the additional space. By default, in common config, HW prefetch can
> > > be enabled so that it works for almost all cases.
> >
> > The hw prefetcher can be enabled at runtime, so a compilation flag
> > does not seem to be a good idea. Moreover, changing this compilation
> 
> On those Hardwares HW prefetch can be disabled at runtime, it is fine
> with default config. I was taking about some low end ARM hardware which
> does not have HW prefetch is not present at all.
> 
> > flag would change the ABI.
> 
> ABI is broken anyway, Right? due to size of the structure change.
> 

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

[Jerin Jacob]

-----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.

I was thinking like this:

in config/common_base:
CONFIG_RTE_EAL_HAS_HW_PREFETCH=y

#ifdef RTE_EAL_HAS_HW_PREFETCH
#define EMPTY_CACHE_LINE char pad0 __rte_cache_aligned
#else
#define EMPTY_CACHE_LINE 
#endif

struct rte_ring_headtail {
	..
	..
	char pad0 __rte_cache_aligned; /**< empty cache line */
	struct rte_ring_headtail prod __rte_cache_aligned;
	EMPTY_CACHE_LINE
	struct rte_ring_headtail cons __rte_cache_aligned;
	EMPTY_CACHE_LINE
}

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

[Ananyev, Konstantin]

Hi Jerin,

> -----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.
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?
Konstantin

> 
> I was thinking like this:
> 
> in config/common_base:
> CONFIG_RTE_EAL_HAS_HW_PREFETCH=y
> 
> #ifdef RTE_EAL_HAS_HW_PREFETCH
> #define EMPTY_CACHE_LINE char pad0 __rte_cache_aligned
> #else
> #define EMPTY_CACHE_LINE
> #endif
> 
> struct rte_ring_headtail {
> 	..
> 	..
> 	char pad0 __rte_cache_aligned; /**< empty cache line */
> 	struct rte_ring_headtail prod __rte_cache_aligned;
> 	EMPTY_CACHE_LINE
> 	struct rte_ring_headtail cons __rte_cache_aligned;
> 	EMPTY_CACHE_LINE
> }

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

[Jerin Jacob]

-----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.



> Konstantin
> 
> > 
> > I was thinking like this:
> > 
> > in config/common_base:
> > CONFIG_RTE_EAL_HAS_HW_PREFETCH=y
> > 
> > #ifdef RTE_EAL_HAS_HW_PREFETCH
> > #define EMPTY_CACHE_LINE char pad0 __rte_cache_aligned
> > #else
> > #define EMPTY_CACHE_LINE
> > #endif
> > 
> > struct rte_ring_headtail {
> > 	..
> > 	..
> > 	char pad0 __rte_cache_aligned; /**< empty cache line */
> > 	struct rte_ring_headtail prod __rte_cache_aligned;
> > 	EMPTY_CACHE_LINE
> > 	struct rte_ring_headtail cons __rte_cache_aligned;
> > 	EMPTY_CACHE_LINE
> > }
> 

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

[Ananyev, Konstantin]

Hi Jerin,

> -----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?
Otherwise there will be not much advantage in such move.
Konstantin

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

[Jerin Jacob]

-----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.
> 
> 

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

[Ananyev, Konstantin]

Hi Jerin,

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Wednesday, April 11, 2018 3:49 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, 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.

Ah yes, you right, missed that.
Konstantin

> 
> 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.
> >
> >

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

[Thomas Monjalon]

> > > > > > > > > > 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.

Applied, thanks

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

[Burakov, Anatoly]

On 03-Apr-18 2:26 PM, Olivier Matz wrote:
> 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
> 
> Link: http://dpdk.org/dev/patchwork/patch/25039/
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---

This patch causes eal_flags_autotest to hang on FreeBSD. Root cause at 
this time is unknown, but this is a weird one - the test seems to hang 
while doing read() in bsd/eal_thread.c:59. Reverting this patch on top 
of rc5 results in eal_flags_autotest passing.

-- 
Thanks,
Anatoly

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

[Burakov, Anatoly]

On 25-May-18 11:59 AM, Burakov, Anatoly wrote:
> On 03-Apr-18 2:26 PM, Olivier Matz wrote:
>> 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
>>
>> Link: http://dpdk.org/dev/patchwork/patch/25039/
>> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
> 
> This patch causes eal_flags_autotest to hang on FreeBSD. Root cause at 
> this time is unknown, but this is a weird one - the test seems to hang 
> while doing read() in bsd/eal_thread.c:59. Reverting this patch on top 
> of rc5 results in eal_flags_autotest passing.
> 

Further investigation shows that for some reason, if Enter is pressed 
while the test is seemingly "hung", it continues and passes. Or rather, 
it hangs on one test, and if Enter is pressed, it finishes that test and 
hangs on another, after which pressing Enter again will result in test 
succeeding. Weird...

-- 
Thanks,
Anatoly

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

[Burakov, Anatoly]

On 25-May-18 1:18 PM, Burakov, Anatoly wrote:
> On 25-May-18 11:59 AM, Burakov, Anatoly wrote:
>> On 03-Apr-18 2:26 PM, Olivier Matz wrote:
>>> 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
>>>
>>> Link: http://dpdk.org/dev/patchwork/patch/25039/
>>> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>> ---
>>
>> This patch causes eal_flags_autotest to hang on FreeBSD. Root cause at 
>> this time is unknown, but this is a weird one - the test seems to hang 
>> while doing read() in bsd/eal_thread.c:59. Reverting this patch on top 
>> of rc5 results in eal_flags_autotest passing.
>>
> 
> Further investigation shows that for some reason, if Enter is pressed 
> while the test is seemingly "hung", it continues and passes. Or rather, 
> it hangs on one test, and if Enter is pressed, it finishes that test and 
> hangs on another, after which pressing Enter again will result in test 
> succeeding. Weird...
> 

This patch is OK, the problem is in the test :) With this patch, it 
apparently blew up in just the right way, but really, the EAL flags 
autotest is wrong on FreeBSD because it launches primary processes with 
'--no-shconf' option, which corrupt main process's memory. For some 
reason this only began manifesting with this patch.

-- 
Thanks,
Anatoly

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

[Olivier Matz]

On Fri, May 25, 2018 at 03:57:07PM +0100, Burakov, Anatoly wrote:
> On 25-May-18 1:18 PM, Burakov, Anatoly wrote:
> > On 25-May-18 11:59 AM, Burakov, Anatoly wrote:
> > > On 03-Apr-18 2:26 PM, Olivier Matz wrote:
> > > > 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
> > > > 
> > > > Link: http://dpdk.org/dev/patchwork/patch/25039/
> > > > Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > > ---
> > > 
> > > This patch causes eal_flags_autotest to hang on FreeBSD. Root cause
> > > at this time is unknown, but this is a weird one - the test seems to
> > > hang while doing read() in bsd/eal_thread.c:59. Reverting this patch
> > > on top of rc5 results in eal_flags_autotest passing.
> > > 
> > 
> > Further investigation shows that for some reason, if Enter is pressed
> > while the test is seemingly "hung", it continues and passes. Or rather,
> > it hangs on one test, and if Enter is pressed, it finishes that test and
> > hangs on another, after which pressing Enter again will result in test
> > succeeding. Weird...
> > 
> 
> This patch is OK, the problem is in the test :) With this patch, it
> apparently blew up in just the right way, but really, the EAL flags autotest
> is wrong on FreeBSD because it launches primary processes with '--no-shconf'
> option, which corrupt main process's memory. For some reason this only began
> manifesting with this patch.

Thanks for the analysis, I was also a bit puzzled by the impact...

Olivier