Hi Bruce, >-----Original Message----- >From: Bruce Richardson <bruce.richard...@intel.com> >Sent: Thursday, September 10, 2020 1:55 AM >To: Maslekar, Omkar <omkar.masle...@intel.com> >Cc: dev@dpdk.org; Loftus, Ciara <ciara.lof...@intel.com> >Subject: Re: [PATCH] EAL: An addition of cache line demote (CLDEMOTE) in >rte_prefetch.h > >On Wed, Sep 09, 2020 at 06:16:54PM -0700, Omkar Maslekar wrote: >> rte_cldemote is similar to a prefetch hint - in reverse. >> cldemote(addr) enables software to hint to hardware that line is likely to >> be >shared. >> Useful in core-to-core communications where cache-line is likely to be >> shared. ARM and PPC implementation is provided with NOP and can be >> added if any equivalent instructions could be used for implementation >> on those architectures. >> >> Signed-off-by: Omkar Maslekar <omkar.masle...@intel.com> >> --- > >Hi Omkar, > >please see some review comments inline below. > >Regards, >/Bruce > >> doc/guides/rel_notes/release_20_11.rst | 26 >> ++++---------------------- >> lib/librte_eal/arm/include/rte_prefetch_32.h | 5 +++++ >> lib/librte_eal/arm/include/rte_prefetch_64.h | 5 +++++ >> lib/librte_eal/include/generic/rte_prefetch.h | 7 +++++++ >> lib/librte_eal/ppc/include/rte_prefetch.h | 5 +++++ >> lib/librte_eal/x86/include/rte_prefetch.h | 9 +++++++++ >> 6 files changed, 35 insertions(+), 22 deletions(-) >> >> diff --git a/doc/guides/rel_notes/release_20_11.rst >> b/doc/guides/rel_notes/release_20_11.rst >> index df227a1..c4a4362 100644 >> --- a/doc/guides/rel_notes/release_20_11.rst >> +++ b/doc/guides/rel_notes/release_20_11.rst >> @@ -27,29 +27,11 @@ New Features >> .. This section should contain new features added in this release. >> Sample format: >> >> - * **Add a title in the past tense with a full stop.** >> +Added new instruction CLDEMOTE in rte_prefetch.h. > >You need to prefix this with the library it is in, in this case EAL. Also, >since this >is C code, you are adding a function, not an instruction.
[I will fix these release notes] > >> >> - Add a short 1-2 sentence description in the past tense. >> - The description should be enough to allow someone scanning >> - the release notes to understand the new feature. >> - >> - If the feature adds a lot of sub-features you can use a bullet list >> - like this: >> - >> - * Added feature foo to do something. >> - * Enhanced feature bar to do something else. >> - >> - Refer to the previous release notes for examples. >> - >> - Suggested order in release notes items: >> - * Core libs (EAL, mempool, ring, mbuf, buses) >> - * Device abstraction libs and PMDs >> - - ethdev (lib, PMDs) >> - - cryptodev (lib, PMDs) >> - - eventdev (lib, PMDs) >> - - etc >> - * Other libs >> - * Apps, Examples, Tools (if significant) > >Don't remove these lines, they are all also part of the same comment as >below where it says "Do not overwrite or remove it" :-) [I will revert original comment and add appropriate] > >> + Added a hardware hint CLDEMOTE which is similar to prefetch in >reverse. >> + CLDEMOTES moves the cache line to the last shared cache, where it >expects >> + sharing to be efficient. >> > >Reading the instruction description in the Intel instruction set reference, it >says about moving the cache line to a more remote cache-line, rather than >guaranteeing that it goes to the last level cache. Therefore, to ensure >compatiblity with the current spec and make it more flexible to meet any >other hardware implementations, I suggest changing the "last shared cache >..." to "more remote cache where sharing may be more efficient". [I will make these changes as per suggestion and make sure it is in sync with software development manual ] > >> This section is a comment. Do not overwrite or remove it. >> Also, make sure to start the actual text at the margin. >> diff --git a/lib/librte_eal/arm/include/rte_prefetch_32.h >> b/lib/librte_eal/arm/include/rte_prefetch_32.h >> index e53420a..ad91edd 100644 >> --- a/lib/librte_eal/arm/include/rte_prefetch_32.h >> +++ b/lib/librte_eal/arm/include/rte_prefetch_32.h >> @@ -33,6 +33,11 @@ static inline void rte_prefetch_non_temporal(const >volatile void *p) >> rte_prefetch0(p); >> } >> >> +static inline void rte_cldemote(const volatile void *p) { >> + RTE_SET_USED(p); >> +} >> + >> #ifdef __cplusplus >> } >> #endif >> diff --git a/lib/librte_eal/arm/include/rte_prefetch_64.h >> b/lib/librte_eal/arm/include/rte_prefetch_64.h >> index fc2b391..35d278a 100644 >> --- a/lib/librte_eal/arm/include/rte_prefetch_64.h >> +++ b/lib/librte_eal/arm/include/rte_prefetch_64.h >> @@ -32,6 +32,11 @@ static inline void rte_prefetch_non_temporal(const >volatile void *p) >> asm volatile ("PRFM PLDL1STRM, [%0]" : : "r" (p)); } >> >> +static inline void rte_cldemote(const volatile void *p) { >> + RTE_SET_USED(p); >> +} >> + >> #ifdef __cplusplus >> } >> #endif >> diff --git a/lib/librte_eal/include/generic/rte_prefetch.h >> b/lib/librte_eal/include/generic/rte_prefetch.h >> index 6e47bdf..89ec69c 100644 >> --- a/lib/librte_eal/include/generic/rte_prefetch.h >> +++ b/lib/librte_eal/include/generic/rte_prefetch.h >> @@ -51,4 +51,11 @@ >> */ >> static inline void rte_prefetch_non_temporal(const volatile void *p); >> >> +/** >> + * Demote a cache line into the last shared cache level. > >Same comment as above. Since this will make it into the official API doxygen >documentation, I think a bit fuller of a description would be good also. [I will add more documentation]