RE: [PATCH v2 2/3] mempool: include non-DPDK threads in statistics

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> Sent: Friday, 4 November 2022 09.59
> 
> On 2022-11-03 09:59, Morten Brørup wrote:
> >> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> >> Sent: Wednesday, 2 November 2022 18.53
> >>
> >> On 2022-11-02 10:09, Morten Brørup wrote:
> >>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> >>>> Sent: Wednesday, 2 November 2022 08.53
> >>>>
> >>>> On 2022-10-31 12:26, Morten Brørup wrote:
> >>>>> Offset the stats array index by one, and count non-DPDK threads
> at
> >>>> index
> >>>>> zero.
> >>>>>
> >>>>> This patch provides two benefits:
> >>>>> * Non-DPDK threads are also included in the statistics.
> >>>>> * A conditional in the fast path is removed. Static branch
> >> prediction
> >>>> was
> >>>>>      correct, so the performance improvement is negligible.
> >>>>>
> >>>>> v2:
> >>>>> * New. No v1 of this patch in the series.
> >>>>>
> >>>>> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
> >>>>> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> >>>>> ---
> >>>>>     lib/mempool/rte_mempool.c |  2 +-
> >>>>>     lib/mempool/rte_mempool.h | 12 ++++++------
> >>>>>     2 files changed, 7 insertions(+), 7 deletions(-)
> >>>>>
> >>>>> diff --git a/lib/mempool/rte_mempool.c
> b/lib/mempool/rte_mempool.c
> >>>>> index 62d1ce764e..e6208125e0 100644
> >>>>> --- a/lib/mempool/rte_mempool.c
> >>>>> +++ b/lib/mempool/rte_mempool.c
> >>>>> @@ -1272,7 +1272,7 @@ rte_mempool_dump(FILE *f, struct
> rte_mempool
> >>>> *mp)
> >>>>>     #ifdef RTE_LIBRTE_MEMPOOL_STATS
> >>>>>     	rte_mempool_ops_get_info(mp, &info);
> >>>>>     	memset(&sum, 0, sizeof(sum));
> >>>>> -	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> >>>>> +	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE + 1;
> lcore_id++) {
> >>>>>     		sum.put_bulk += mp->stats[lcore_id].put_bulk;
> >>>>>     		sum.put_objs += mp->stats[lcore_id].put_objs;
> >>>>>     		sum.put_common_pool_bulk += mp-
> >>>>> stats[lcore_id].put_common_pool_bulk;
> >>>>> diff --git a/lib/mempool/rte_mempool.h
> b/lib/mempool/rte_mempool.h
> >>>>> index 9c4bf5549f..16e7e62e3c 100644
> >>>>> --- a/lib/mempool/rte_mempool.h
> >>>>> +++ b/lib/mempool/rte_mempool.h
> >>>>> @@ -238,8 +238,11 @@ struct rte_mempool {
> >>>>>     	struct rte_mempool_memhdr_list mem_list; /**< List of
> >> memory
> >>>> chunks */
> >>>>>
> >>>>>     #ifdef RTE_LIBRTE_MEMPOOL_STATS
> >>>>> -	/** Per-lcore statistics. */
> >>>>> -	struct rte_mempool_debug_stats stats[RTE_MAX_LCORE];
> >>>>> +	/** Per-lcore statistics.
> >>>>> +	 *
> >>>>> +	 * Offset by one, to include non-DPDK threads.
> >>>>> +	 */
> >>>>> +	struct rte_mempool_debug_stats stats[RTE_MAX_LCORE + 1];
> >>>>>     #endif
> >>>>>     }  __rte_cache_aligned;
> >>>>>
> >>>>> @@ -304,10 +307,7 @@ struct rte_mempool {
> >>>>>      */
> >>>>>     #ifdef RTE_LIBRTE_MEMPOOL_STATS
> >>>>>     #define RTE_MEMPOOL_STAT_ADD(mp, name, n) do {
> >> \
> >>>>> -		unsigned __lcore_id = rte_lcore_id();           \
> >>>>> -		if (__lcore_id < RTE_MAX_LCORE) {               \
> >>>>> -			mp->stats[__lcore_id].name += n;        \
> >>>>> -		}                                               \
> >>>>> +		(mp)->stats[rte_lcore_id() + 1].name += n;      \
> >>>>
> >>>> This relies on LCORE_ID_ANY being UINT32_MAX, and a wrap to 0, for
> >> an
> >>>> unregistered non-EAL thread? Might be worth a comment, or better a
> >>>> rewrite with an explicit LCORE_ID_ANY comparison.
> >>>
> >>> The purpose of this patch is to avoid the comparison here.
> >>>
> >>> Yes, it relies on the wrap to zero, and these conditions:
> >>> 1. LCORE_ID_ANY being UINT32_MAX, and
> >>> 2. the return type of rte_lcore_id() being unsigned int, and
> >>> 3. unsigned int being uint32_t.
> >>>
> >>> When I wrote this, I considered it safe to assume that LCORE_ID_ANY
> >> will remain the unsigned equivalent of -1 using the return type of
> >> rte_lcore_id(). In other words: If the return type of rte_lcore_id()
> >> should change from 32 to 64 bit, LCORE_ID_ANY would be updated
> >> accordingly to UINT64_MAX.
> >>>
> >>> Because of this assumption, I didn't use [(rte_lcore_id() + 1) &
> >> UINT32_MAX], but just [rte_lcore_id() + 1].
> >>>
> >>> I struggled writing an appropriate comment without making it
> >> unacceptably long, but eventually gave up, and settled for the one-
> line
> >> comment in the structure only.
> >>>
> >>>>
> >>>> You anyways need a conditional. An atomic add must be used in the
> >>>> unregistered EAL thread case.
> >>>
> >>> Good point: The "+= n" must be atomic for non-isolated threads.
> >>>
> >>
> >> If the various unregistered non-EAL threads are run on isolated
> cores
> >> or not does not matter.
> >
> > Agree: They all share the same index, and thus may race, regardless
> which cores they are using.
> >
> > Rephrasing: The "+= n" must be atomic for the unregistered non-EAL
> threads.
> >
> >>
> >>> I just took a look at how software maintained stats are handled
> >> elsewhere, and the first thing I found, is the IOAT DMA driver,
> which
> >> also seems to be using non-atomic increment [1] regardless if used
> by a
> >> DPDK thread or not.
> >>>
> >>> [1]: https://elixir.bootlin.com/dpdk/v22.11-
> >> rc2/source/drivers/dma/ioat/ioat_dmadev.c#L228
> >>>
> >>> However, doing it wrong elsewhere doesn't make it correct doing it
> >> wrong here too. :-)
> >>>
> >>> Atomic increments are costly, so I would rather follow your
> >> suggestion and reintroduce the comparison. How about:
> >>>
> >>> #define RTE_MEMPOOL_STAT_ADD(mp, name, n) do { \
> >>>       unsigned int __lcore_id = rte_lcore_id(); \
> >>>       if (likely(__lcore_id < RTE_MAX_LCORE)) { \
> >>>           (mp)->stats[__lcore_id].name += n; \
> >>>       } else {
> >>>           rte_atomic64_add( \
> >>>                   (rte_atomic64_t*)&((mp)-
> >stats[RTE_MAX_LCORE].name),
> >> n);\
> >>>       } \
> >>> }
> >> You are supposed to use GCC C11 intrinsics (e.g.,
> >> __atomic_fetch_add()).
> >
> > Ups. I forgot!
> >
> > This should be emphasized everywhere in the rte_atomic library, to
> prevent such mistakes.
> >
> >>
> >> In addition: technically, you must use an atomic store for the EAL
> >> thread case (and an atomic load on the reader side), although there
> are
> >> tons of examples in DPDK where tearing is ignored. (The generated
> code
> >> will likely look the same.)
> >
> > The counters are 64 bit aligned, but tearing could happen on 32 bit
> architectures.
> >
> 
> The compiler is free to do it on any architecture, but I'm not sure if
> it happens much in practice.
> 
> > My initial reaction to your comment was to do it correctly on the EAL
> threads too, to avoid tearing there too. However, there might be a
> performance cost for 32 bit architectures, so I will consider that
> these are only debug counters, and accept the risk of tearing.
> >
> 
> What would that cost consist of?

I have tested it this morning on Godbolt: https://godbolt.org/z/fz7f3cv8b

ctr += n becomes:

        add     QWORD PTR ctr[rip], rdi

Whereas __atomic_fetch_add(&ctr, n, __ATOMIC_RELAXED) becomes:
 
        lock add        QWORD PTR ctr[rip], rdi

> 
> In theory C11 atomics could be implemented "in software" (i.e., without
> the proper ISA-level guarantees, with locks), but does any of the
> DPDK-supported compiler/32-bit ISAs actually do so?

Adding -m32 to the compiler options, ctr += n becomes:

        xor     edx, edx
        mov     eax, DWORD PTR [esp+4]
        add     DWORD PTR ctr, eax
        adc     DWORD PTR ctr+4, edx

And __atomic_fetch_add(&ctr, n, __ATOMIC_RELAXED) becomes:

        push    edi
        xor     edi, edi
        push    esi
        push    ebx
        sub     esp, 8
        mov     esi, DWORD PTR [esp+24]
        mov     eax, DWORD PTR ctr
        mov     edx, DWORD PTR ctr+4
.L4:
        mov     ecx, eax
        mov     ebx, edx
        add     ecx, esi
        adc     ebx, edi
        mov     DWORD PTR [esp], ecx
        mov     DWORD PTR [esp+4], ebx
        mov     ebx, DWORD PTR [esp]
        mov     ecx, DWORD PTR [esp+4]
        lock cmpxchg8b  QWORD PTR ctr
        jne     .L4
        add     esp, 8
        pop     ebx
        pop     esi
        pop     edi

> 
> It's also not obvious that if there, for a certain
> compiler/ISA-combination, is a performance impact to pay for
> correctness, correctness would have to give way.

In this particular case, and because they are only debug counters, I will argue that DPDK generally uses non-atomic access to 64 bit statistics counters. This was also the case for these counters before this patch.

I am planning to finish v3 of this patch series today, so I hope you can agree to close the atomicity discussion with another argument: Making statistics counters atomic should be elevated to a general patch across DPDK, and not part of this mempool patch series.

The v3 patch (which I am working on right now) counts atomically for the unregistered non-EAL threads:

#define RTE_MEMPOOL_STAT_ADD(mp, name, n) do {                                  \
		unsigned int __lcore_id = rte_lcore_id();                       \
		if (likely(__lcore_id < RTE_MAX_LCORE))                         \
			(mp)->stats[__lcore_id].name += n;                      \
		else                                                            \
			__atomic_fetch_add(&((mp)->stats[RTE_MAX_LCORE].name),  \
					   n, __ATOMIC_RELAXED);                \
	} while (0)

PS: Excellent discussion - thank you for getting involved, Mattias.