[dpdk-dev] [PATCH v9 1/4] eal: add WC store functions

Nicolau, Radu radu.nicolau at intel.com
Tue Jul 21 10:56:53 CEST 2020


On 7/20/2020 1:20 PM, David Marchand wrote:
> On Mon, Jul 20, 2020 at 11:12 AM Radu Nicolau <radu.nicolau at intel.com> wrote:
>> Add rte_write32_wc and rte_write32_wc_relaxed functions
>> that implement 32bit stores using write combining memory protocol.
>> Provided generic stubs and x86 implementation.
> What is the difference of using this new API when compared to the
> existing pci driver flag RTE_PCI_DRV_WC_ACTIVATE?
> Do we have some overlap between the two?
No, the RTE_PCI_DRV_WC_ACTIVATE define will map the BARs as write 
combining, whereas these functions will use the WC regardless of the 
mapping, and they can be used for memory areas that won't be mapped 
using the PCI infrastructure.
>
> This commitlog is quite short for something that touches performance.
> I saw a question from Ruifeng, it is worth adding this to the commitlog.

Strictly speaking, this patch only enables support for WC stores, and it 
does not have any performance implications on its own. I think a fair 
assumption is that if someone is looking to use WC stores they will know 
exactly how and where they can be used.

>
> Which x86 platforms will benefit from it?
> What is the impact on performance for existing platforms that have no
> MOVDIRI support?

x86 platforms supporting MOVDIRI instruction with this particular patch, 
and any other platform that have similar instructions if they were to be 
enabled.

There is no impact for the ones that don't support it.

>
>
>> Signed-off-by: Radu Nicolau <radu.nicolau at intel.com>
>> Acked-by: Bruce Richardson <bruce.richardson at intel.com>
>> ---
>>   lib/librte_eal/arm/include/rte_io_64.h  | 12 +++++++
>>   lib/librte_eal/include/generic/rte_io.h | 48 ++++++++++++++++++++++++++++
>>   lib/librte_eal/x86/include/rte_io.h     | 56 +++++++++++++++++++++++++++++++++
>>   3 files changed, 116 insertions(+)
>>
>> diff --git a/lib/librte_eal/arm/include/rte_io_64.h b/lib/librte_eal/arm/include/rte_io_64.h
>> index e534624..d07d9cb 100644
>> --- a/lib/librte_eal/arm/include/rte_io_64.h
>> +++ b/lib/librte_eal/arm/include/rte_io_64.h
>> @@ -164,6 +164,18 @@ rte_write64(uint64_t value, volatile void *addr)
>>          rte_write64_relaxed(value, addr);
>>   }
>>
>> +static __rte_always_inline void
>> +rte_write32_wc(uint32_t value, volatile void *addr)
>> +{
>> +       rte_write32(value, addr);
>> +}
>> +
>> +static __rte_always_inline void
>> +rte_write32_wc_relaxed(uint32_t value, volatile void *addr)
>> +{
>> +       rte_write32_relaxed(value, addr);
>> +}
>> +
> We were using a single knob RTE_OVERRIDE_IO_H for overriding the whole rte_io.h.
> Now we would have a special case for an API for x86 and the code is
> copy/pasted in the ARM header and keeping the "whole" override mode.
>
> This leaves an unfinished taste.
>
> Why did you not flag all relevant "native" helpers?
> This would factor some code from the ARM header.
I agree that having a more granular approach is better, having a single 
knob is why ARM header had about half of the functions overridden and 
half copied and pasted before this patch. But this is outside the scope 
of this patch.
>
>
>>   #ifdef __cplusplus
>>   }
>>   #endif
>> diff --git a/lib/librte_eal/include/generic/rte_io.h b/lib/librte_eal/include/generic/rte_io.h
>> index da457f7..0669baa 100644
>> --- a/lib/librte_eal/include/generic/rte_io.h
>> +++ b/lib/librte_eal/include/generic/rte_io.h
>> @@ -229,6 +229,40 @@ rte_write32(uint32_t value, volatile void *addr);
>>   static inline void
>>   rte_write64(uint64_t value, volatile void *addr);
>>
>> +/**
>> + * Write a 32-bit value to I/O device memory address addr using write
>> + * combining memory write protocol. Depending on the platform write combining
>> + * may not be available and/or may be treated as a hint and the behavior may
>> + * fallback to a regular store.
>> + *
>> + * @param value
>> + *  Value to write
>> + * @param addr
>> + *  I/O memory address to write the value to
>> + */
>> +__rte_experimental
>> +static inline void
>> +rte_write32_wc(uint32_t value, volatile void *addr);
>> +
>> +/**
>> + * Write a 32-bit value to I/O device memory address addr using write
>> + * combining memory write protocol. Depending on the platform write combining
>> + * may not be available and/or may be treated as a hint and the behavior may
>> + * fallback to a regular store.
>> + *
>> + * The relaxed version does not have additional I/O memory barrier, useful in
>> + * accessing the device registers of integrated controllers which implicitly
>> + * strongly ordered with respect to memory access.
> It might be just me, but I have trouble reading the last part of this sentence.
> Maybe remove "with respect to"?
It was copied and pasted from the other _relaxed functions for 
consistency reasons.
>
>
>> + *
>> + * @param value
>> + *  Value to write
>> + * @param addr
>> + *  I/O memory address to write the value to
>> + */
>> +__rte_experimental
>> +static inline void
>> +rte_write32_wc_relaxed(uint32_t value, volatile void *addr);
>> +
>>   #endif /* __DOXYGEN__ */
>>
>>   #ifndef RTE_OVERRIDE_IO_H
>> @@ -345,6 +379,20 @@ rte_write64(uint64_t value, volatile void *addr)
>>          rte_write64_relaxed(value, addr);
>>   }
>>
>> +#ifndef RTE_NATIVE_WRITE32_WC
>> +static __rte_always_inline void
>> +rte_write32_wc(uint32_t value, volatile void *addr)
>> +{
>> +       rte_write32(value, addr);
>> +}
>> +
>> +static __rte_always_inline void
>> +rte_write32_wc_relaxed(uint32_t value, volatile void *addr)
>> +{
>> +       rte_write32_relaxed(value, addr);
>> +}
>> +#endif /* RTE_NATIVE_WRITE32_WC */
>> +
>>   #endif /* RTE_OVERRIDE_IO_H */
>>
>>   #endif /* _RTE_IO_H_ */
>> diff --git a/lib/librte_eal/x86/include/rte_io.h b/lib/librte_eal/x86/include/rte_io.h
>> index 2db71b1..c95ed67 100644
>> --- a/lib/librte_eal/x86/include/rte_io.h
>> +++ b/lib/librte_eal/x86/include/rte_io.h
>> @@ -9,8 +9,64 @@
>>   extern "C" {
>>   #endif
>>
>> +#include "rte_cpuflags.h"
>> +
>> +#define RTE_NATIVE_WRITE32_WC
>>   #include "generic/rte_io.h"
>>
>> +/**
>> + * @internal
>> + * MOVDIRI wrapper.
>> + */
>> +static __rte_always_inline void
>> +_rte_x86_movdiri(uint32_t value, volatile void *addr)
>> +{
>> +       asm volatile(
>> +               /* MOVDIRI */
>> +               ".byte 0x40, 0x0f, 0x38, 0xf9, 0x02"
>> +               :
>> +               : "a" (value), "d" (addr));
>> +}
>> +
>> +static __rte_always_inline void
>> +rte_write32_wc(uint32_t value, volatile void *addr)
>> +{
>> +       static int _x86_movdiri_flag = -1;
>> +       if (_x86_movdiri_flag == 1) {
>> +               rte_wmb();
>> +               _rte_x86_movdiri(value, addr);
>> +       } else if (_x86_movdiri_flag == 0) {
>> +               rte_write32(value, addr);
>> +       } else {
>> +               _x86_movdiri_flag =
>> +                       (rte_cpu_get_flag_enabled(RTE_CPUFLAG_MOVDIRI) > 0);
>> +               if (_x86_movdiri_flag == 1) {
>> +                       rte_wmb();
>> +                       _rte_x86_movdiri(value, addr);
>> +               } else {
>> +                       rte_write32(value, addr);
>> +               }
>> +       }
>> +}
>> +
>> +static __rte_always_inline void
>> +rte_write32_wc_relaxed(uint32_t value, volatile void *addr)
>> +{
>> +       static int _x86_movdiri_flag = -1;
>> +       if (_x86_movdiri_flag == 1) {
>> +               _rte_x86_movdiri(value, addr);
>> +       } else if (_x86_movdiri_flag == 0) {
>> +               rte_write32_relaxed(value, addr);
>> +       } else {
>> +               _x86_movdiri_flag =
>> +                       (rte_cpu_get_flag_enabled(RTE_CPUFLAG_MOVDIRI) > 0);
>> +               if (_x86_movdiri_flag == 1)
>> +                       _rte_x86_movdiri(value, addr);
>> +               else
>> +                       rte_write32_relaxed(value, addr);
>> +       }
>> +}
>> +
> Repeating some comments I made earlier.
>
> - If a single helper called by both rte_write32_wc and
> rte_write32_wc_relaxed with a _constant_ flag is not to your liking (I
> don't see where it would have an impact on performance), then maybe
> rte_write32_wc() can be simply implemented as:
>
> +static __rte_always_inline void
> +rte_write32_wc(uint32_t value, volatile void *addr)
> +{
> +    rte_wmb();
> +    rte_write32_wc_relaxed(value, addr);
> +}
> +
Yes, it can be written as such, I will update the patch.
>
> - Looking at this above suggestion, I wonder about the non-relaxed case.
> Is rte_io_wmb() not enough?
No, we need an actual memory fence, on x86 rte_io_wmb() is defined as 
rte_compiler_barrier()
>
>
> - The cpuflag check can be resolved at init once and for all.
> By this, I mean in lib/librte_eal/x86/rte_cpuflags.c:
>
> +int rte_x86_movdiri_flag = -1;
> +
> +RTE_INIT(rte_x86_movdiri_init)
> +{
> +       rte_x86_movdiri_flag =
> +               (rte_cpu_get_flag_enabled(RTE_CPUFLAG_MOVDIRI) > 0);
> +}
>
> The variable can be exported in lib/librte_eal/x86/include/rte_cpuflags.h.
>
> Then rte_write32_wc_relaxed() becomes:
>
> +static __rte_always_inline void
> +rte_write32_wc_relaxed(uint32_t value, volatile void *addr)
> +{
> +    if (rte_x86_movdiri_flag == 1) {
> +        asm volatile(
> +            /* MOVDIRI */
> +            ".byte 0x40, 0x0f, 0x38, 0xf9, 0x02"
> +            :
> +            : "a" (value), "d" (addr));
> +        return;
> +    }
> +
> +    rte_write32_relaxed(value, addr);
> +}
> +
I tried this before sending the patch, there is an issue when linking 
the shared library version with the --no-undefined flag. This will 
require to have the variable exported in the overall map file, requiring 
a change across all platforms for something that is very x86 specific.
>
> Thanks.
>


More information about the dev mailing list