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

[Olivier MATZ]

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 at caviumnetworks.com>
> Suggested-by: Konstantin Ananyev <konstantin.ananyev at 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