[RFC] cache guard

[Mattias Rönnblom]

On 2023-08-27 10:34, Morten Brørup wrote:
> +CC Honnappa and Konstantin, Ring lib maintainers
> +CC Mattias, PRNG lib maintainer
> 
>> From: Bruce Richardson [mailto:bruce.richardson at intel.com]
>> Sent: Friday, 25 August 2023 11.24
>>
>> On Fri, Aug 25, 2023 at 11:06:01AM +0200, Morten Brørup wrote:
>>> +CC mempool maintainers
>>>
>>>> From: Bruce Richardson [mailto:bruce.richardson at intel.com]
>>>> Sent: Friday, 25 August 2023 10.23
>>>>
>>>> On Fri, Aug 25, 2023 at 08:45:12AM +0200, Morten Brørup wrote:
>>>>> Bruce,
>>>>>
>>>>> With this patch [1], it is noted that the ring producer and
>> consumer data
>>>> should not be on adjacent cache lines, for performance reasons.
>>>>>
>>>>> [1]:
>>>>
>> https://git.dpdk.org/dpdk/commit/lib/librte_ring/rte_ring.h?id=d9f0d3a1f
>> fd4b66
>>>> e75485cc8b63b9aedfbdfe8b0
>>>>>
>>>>> (It's obvious that they cannot share the same cache line, because
>> they are
>>>> accessed by two different threads.)
>>>>>
>>>>> Intuitively, I would think that having them on different cache
>> lines would
>>>> suffice. Why does having an empty cache line between them make a
>> difference?
>>>>>
>>>>> And does it need to be an empty cache line? Or does it suffice
>> having the
>>>> second structure start at two cache lines after the start of the
>> first
>>>> structure (e.g. if the size of the first structure is two cache
>> lines)?
>>>>>
>>>>> I'm asking because the same principle might apply to other code
>> too.
>>>>>
>>>> Hi Morten,
>>>>
>>>> this was something we discovered when working on the distributor
>> library.
>>>> If we have cachelines per core where there is heavy access, having
>> some
>>>> cachelines as a gap between the content cachelines can help
>> performance. We
>>>> believe this helps due to avoiding issues with the HW prefetchers
>> (e.g.
>>>> adjacent cacheline prefetcher) bringing in the second cacheline
>>>> speculatively when an operation is done on the first line.
>>>
>>> I guessed that it had something to do with speculative prefetching,
>> but wasn't sure. Good to get confirmation, and that it has a measureable
>> effect somewhere. Very interesting!
>>>
>>> NB: More comments in the ring lib about stuff like this would be nice.
>>>
>>> So, for the mempool lib, what do you think about applying the same
>> technique to the rte_mempool_debug_stats structure (which is an array
>> indexed per lcore)... Two adjacent lcores heavily accessing their local
>> mempool caches seems likely to me. But how heavy does the access need to
>> be for this technique to be relevant?
>>>
>>
>> No idea how heavy the accesses need to be for this to have a noticable
>> effect. For things like debug stats, I wonder how worthwhile making such
>> a
>> change would be, but then again, any change would have very low impact
>> too
>> in that case.
> 
> I just tried adding padding to some of the hot structures in our own application, and observed a significant performance improvement for those.
> 
> So I think this technique should have higher visibility in DPDK by adding a new cache macro to rte_common.h:
> 
> /**
>   * Empty cache line, to guard against speculative prefetching.
>   *

"to guard against false sharing-like effects on systems with a 
next-N-lines hardware prefetcher"

>   * Use as spacing between data accessed by different lcores,
>   * to prevent cache thrashing on CPUs with speculative prefetching.
>   */
> #define RTE_CACHE_GUARD(name) char cache_guard_##name[RTE_CACHE_LINE_SIZE] __rte_cache_aligned;
> 

You could have a macro which specified how much guarding there needs to 
be, ideally defined on a per-CPU basis. (These things has nothing to do 
with the ISA, but everything to do with the implementation.)

I'm not sure N is always 1.

So the guard padding should be RTE_CACHE_LINE_SIZE * 
RTE_CACHE_GUARD_LINES bytes, and wrap the whole thing in
#if RTE_CACHE_GUARD_LINES > 0
#endif

...so you can disable this (cute!) hack (on custom DPDK builds) in case 
you have disabled hardware prefetching, which seems generally to be a 
good idea for packet processing type applications.

...which leads me to another suggestions: add a note on disabling 
hardware prefetching in the optimization guide.

Seems like a very good idea to have this in <rte_common.h>, and 
otherwise make this issue visible and known.

> To be used like this:
> 
> struct rte_ring {
> 	char name[RTE_RING_NAMESIZE] __rte_cache_aligned;
> 	/**< Name of the ring. */
> 	int flags;               /**< Flags supplied at creation. */
> 	const struct rte_memzone *memzone;
> 			/**< Memzone, if any, containing the rte_ring */
> 	uint32_t size;           /**< Size of ring. */
> 	uint32_t mask;           /**< Mask (size-1) of ring. */
> 	uint32_t capacity;       /**< Usable size of ring */
> 
> -	char pad0 __rte_cache_aligned; /**< empty cache line */
> +	RTE_CACHE_GUARD(prod);  /**< Isolate producer status. */
> 
> 	/** Ring producer status. */
> 	union {
> 		struct rte_ring_headtail prod;
> 		struct rte_ring_hts_headtail hts_prod;
> 		struct rte_ring_rts_headtail rts_prod;
> 	}  __rte_cache_aligned;
> 
> -	char pad1 __rte_cache_aligned; /**< empty cache line */
> +	RTE_CACHE_GUARD(both);  /**< Isolate producer from consumer. */
> 
> 	/** Ring consumer status. */
> 	union {
> 		struct rte_ring_headtail cons;
> 		struct rte_ring_hts_headtail hts_cons;
> 		struct rte_ring_rts_headtail rts_cons;
> 	}  __rte_cache_aligned;
> 
> -	char pad2 __rte_cache_aligned; /**< empty cache line */
> +	RTE_CACHE_GUARD(cons);  /**< Isolate consumer status. */
> };
> 
> 
> And for the mempool library:
> 
> #ifdef RTE_LIBRTE_MEMPOOL_STATS
> /**
>   * A structure that stores the mempool statistics (per-lcore).
>   * Note: Cache stats (put_cache_bulk/objs, get_cache_bulk/objs) are not
>   * captured since they can be calculated from other stats.
>   * For example: put_cache_objs = put_objs - put_common_pool_objs.
>   */
> struct rte_mempool_debug_stats {
> 	uint64_t put_bulk;             /**< Number of puts. */
> 	uint64_t put_objs;             /**< Number of objects successfully put. */
> 	uint64_t put_common_pool_bulk; /**< Number of bulks enqueued in common pool. */
> 	uint64_t put_common_pool_objs; /**< Number of objects enqueued in common pool. */
> 	uint64_t get_common_pool_bulk; /**< Number of bulks dequeued from common pool. */
> 	uint64_t get_common_pool_objs; /**< Number of objects dequeued from common pool. */
> 	uint64_t get_success_bulk;     /**< Successful allocation number. */
> 	uint64_t get_success_objs;     /**< Objects successfully allocated. */
> 	uint64_t get_fail_bulk;        /**< Failed allocation number. */
> 	uint64_t get_fail_objs;        /**< Objects that failed to be allocated. */
> 	uint64_t get_success_blks;     /**< Successful allocation number of contiguous blocks. */
> 	uint64_t get_fail_blks;        /**< Failed allocation number of contiguous blocks. */
> +	RTE_CACHE_GUARD(debug_stats);  /**< Isolation between lcores. */
> } __rte_cache_aligned;
> #endif
> 
> struct rte_mempool {
>   [...]
> #ifdef RTE_LIBRTE_MEMPOOL_STATS
> 	/** Per-lcore statistics.
> 	 *
> 	 * Plus one, for unregistered non-EAL threads.
> 	 */
> 	struct rte_mempool_debug_stats stats[RTE_MAX_LCORE + 1];
> #endif
> }  __rte_cache_aligned;
> 
> 
> It also seems relevant for the PRNG library:
> 
> /lib/eal/common/rte_random.c:
> 
> struct rte_rand_state {
> 	uint64_t z1;
> 	uint64_t z2;
> 	uint64_t z3;
> 	uint64_t z4;
> 	uint64_t z5;
> +	RTE_CACHE_GUARD(z);
> } __rte_cache_aligned;
> 

Yes.

Should there be two cache guard macros? One parameter-free 
RTE_CACHE_GUARD and a RTE_CACHE_NAMED_GUARD(name) macro?

Maybe it's better just to keep the single macro, but have a convention 
with some generic name (i.e., not 'z' above) for the guard field, like 
'cache_guard' or just 'guard'. Having unique name makes no sense, except 
in rare cases where you need multiple guard lines per struct.

> /* One instance each for every lcore id-equipped thread, and one
>   * additional instance to be shared by all others threads (i.e., all
>   * unregistered non-EAL threads).
>   */
> static struct rte_rand_state rand_states[RTE_MAX_LCORE + 1];
>