diff mbox

[dpdk-dev,v3,1/4] eal: Introduce new cache macro definitions

Message ID 1450067576-18803-2-git-send-email-jerin.jacob@caviumnetworks.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Jerin Jacob Dec. 14, 2015, 4:32 a.m. UTC 
  - RTE_CACHE_MIN_LINE_SIZE(Supported minimum cache line size)
- __rte_cache_min_aligned(Force minimum cache line alignment)
- RTE_CACHE_LINE_SIZE_LOG2(Express cache line size in terms of log2)

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 lib/librte_eal/common/include/rte_memory.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Olivier Matz Jan. 4, 2016, 1:15 p.m. UTC | #1 
Hi Jerin,

Please see some comments below.

On 12/14/2015 05:32 AM, Jerin Jacob wrote:
> - RTE_CACHE_MIN_LINE_SIZE(Supported minimum cache line size)
> - __rte_cache_min_aligned(Force minimum cache line alignment)
> - RTE_CACHE_LINE_SIZE_LOG2(Express cache line size in terms of log2)
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  lib/librte_eal/common/include/rte_memory.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/lib/librte_eal/common/include/rte_memory.h b/lib/librte_eal/common/include/rte_memory.h
> index 9c9e40f..b67a76f 100644
> --- a/lib/librte_eal/common/include/rte_memory.h
> +++ b/lib/librte_eal/common/include/rte_memory.h
> @@ -77,11 +77,27 @@ enum rte_page_sizes {
>  	(RTE_CACHE_LINE_SIZE * ((size + RTE_CACHE_LINE_SIZE - 1) / RTE_CACHE_LINE_SIZE))
>  /**< Return the first cache-aligned value greater or equal to size. */
>  
> +/**< Cache line size in terms of log2 */
> +#if RTE_CACHE_LINE_SIZE == 64
> +#define RTE_CACHE_LINE_SIZE_LOG2 6
> +#elif RTE_CACHE_LINE_SIZE == 128
> +#define RTE_CACHE_LINE_SIZE_LOG2 7
> +#else
> +#error "Unsupported cache line size"
> +#endif
> +
> +#define RTE_CACHE_MIN_LINE_SIZE 64	/**< Minimum Cache line size. */
> +

I think RTE_CACHE_LINE_MIN_SIZE or RTE_MIN_CACHE_LINE_SIZE would
be clearer than RTE_CACHE_MIN_LINE_SIZE.

>  /**
>   * Force alignment to cache line.
>   */
>  #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
>  
> +/**
> + * Force minimum cache line alignment.
> + */
> +#define __rte_cache_min_aligned __rte_aligned(RTE_CACHE_MIN_LINE_SIZE)

I'm not really convinced that __rte_cache_min_aligned is straightforward
for someone reading the code that it means "aligned to the minimum cache
line size supported by the dpdk".

In the two cases you are using this macro (mbuf structure and queue
info), I'm wondering if using __attribute__((aligned(64))) wouldn't be
clearer?
- for mbuf, it could be a local define, like MBUF_ALIGN_SIZE
- for queue info, using 64 makes sense as it's used to reserve space
  for future use

What do you think?


Regards,
Olivier
Jerin Jacob Jan. 6, 2016, 3:10 p.m. UTC | #2 
On Mon, Jan 04, 2016 at 02:15:53PM +0100, Olivier MATZ wrote:
> Hi Jerin,

Hi Olivier,

> 
> Please see some comments below.
> 
> On 12/14/2015 05:32 AM, Jerin Jacob wrote:
> > - RTE_CACHE_MIN_LINE_SIZE(Supported minimum cache line size)
> > - __rte_cache_min_aligned(Force minimum cache line alignment)
> > - RTE_CACHE_LINE_SIZE_LOG2(Express cache line size in terms of log2)
> > 
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> >  lib/librte_eal/common/include/rte_memory.h | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/lib/librte_eal/common/include/rte_memory.h b/lib/librte_eal/common/include/rte_memory.h
> > index 9c9e40f..b67a76f 100644
> > --- a/lib/librte_eal/common/include/rte_memory.h
> > +++ b/lib/librte_eal/common/include/rte_memory.h
> > @@ -77,11 +77,27 @@ enum rte_page_sizes {
> >  	(RTE_CACHE_LINE_SIZE * ((size + RTE_CACHE_LINE_SIZE - 1) / RTE_CACHE_LINE_SIZE))
> >  /**< Return the first cache-aligned value greater or equal to size. */
> >  
> > +/**< Cache line size in terms of log2 */
> > +#if RTE_CACHE_LINE_SIZE == 64
> > +#define RTE_CACHE_LINE_SIZE_LOG2 6
> > +#elif RTE_CACHE_LINE_SIZE == 128
> > +#define RTE_CACHE_LINE_SIZE_LOG2 7
> > +#else
> > +#error "Unsupported cache line size"
> > +#endif
> > +
> > +#define RTE_CACHE_MIN_LINE_SIZE 64	/**< Minimum Cache line size. */
> > +
> 
> I think RTE_CACHE_LINE_MIN_SIZE or RTE_MIN_CACHE_LINE_SIZE would
> be clearer than RTE_CACHE_MIN_LINE_SIZE.

OK. I will resend the next version with RTE_CACHE_LINE_MIN_SIZE

> 
> >  /**
> >   * Force alignment to cache line.
> >   */
> >  #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
> >  
> > +/**
> > + * Force minimum cache line alignment.
> > + */
> > +#define __rte_cache_min_aligned __rte_aligned(RTE_CACHE_MIN_LINE_SIZE)
> 
> I'm not really convinced that __rte_cache_min_aligned is straightforward
> for someone reading the code that it means "aligned to the minimum cache
> line size supported by the dpdk".
> 
> In the two cases you are using this macro (mbuf structure and queue
> info), I'm wondering if using __attribute__((aligned(64))) wouldn't be
> clearer?
> - for mbuf, it could be a local define, like MBUF_ALIGN_SIZE
> - for queue info, using 64 makes sense as it's used to reserve space
>   for future use
> 
> What do you think?

IMO, it makes sense to keep "__rte_cache_min_aligned" as it will useful
for forcing the minimum alignment requirements in DPDK in future
structures.

Thoughts?

> 
> 
> Regards,
> Olivier
diff mbox

Patch

diff --git a/lib/librte_eal/common/include/rte_memory.h b/lib/librte_eal/common/include/rte_memory.h
index 9c9e40f..b67a76f 100644
--- a/lib/librte_eal/common/include/rte_memory.h
+++ b/lib/librte_eal/common/include/rte_memory.h
@@ -77,11 +77,27 @@  enum rte_page_sizes {
 	(RTE_CACHE_LINE_SIZE * ((size + RTE_CACHE_LINE_SIZE - 1) / RTE_CACHE_LINE_SIZE))
 /**< Return the first cache-aligned value greater or equal to size. */
 
+/**< Cache line size in terms of log2 */
+#if RTE_CACHE_LINE_SIZE == 64
+#define RTE_CACHE_LINE_SIZE_LOG2 6
+#elif RTE_CACHE_LINE_SIZE == 128
+#define RTE_CACHE_LINE_SIZE_LOG2 7
+#else
+#error "Unsupported cache line size"
+#endif
+
+#define RTE_CACHE_MIN_LINE_SIZE 64	/**< Minimum Cache line size. */
+
 /**
  * Force alignment to cache line.
  */
 #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
 
+/**
+ * Force minimum cache line alignment.
+ */
+#define __rte_cache_min_aligned __rte_aligned(RTE_CACHE_MIN_LINE_SIZE)
+
 typedef uint64_t phys_addr_t; /**< Physical address definition. */
 #define RTE_BAD_PHYS_ADDR ((phys_addr_t)-1)