From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E9EBBA04B5; Thu, 10 Sep 2020 10:55:26 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 747691BEB3; Thu, 10 Sep 2020 10:55:26 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 71B992BA8 for ; Thu, 10 Sep 2020 10:55:24 +0200 (CEST) IronPort-SDR: NCBd20WIdc7+9TpKe1KXGZM5oRtdosMzXpYR2OgmSjtx31MYRpVfL5nWMymp1EwZ/79w7GcF8U Qrs6b4umLXUw== X-IronPort-AV: E=McAfee;i="6000,8403,9739"; a="220053081" X-IronPort-AV: E=Sophos;i="5.76,412,1592895600"; d="scan'208";a="220053081" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Sep 2020 01:55:23 -0700 IronPort-SDR: 5+sfoI0xLQ6zjX1mAWFBNZNJn6GW2PF/eZfe86LpTc8ifjxm2G8i+41+KyUogGtN4W0yUC8GOO bPob+wjL2NqA== X-IronPort-AV: E=Sophos;i="5.76,412,1592895600"; d="scan'208";a="480810483" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.5.251]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 10 Sep 2020 01:55:22 -0700 Date: Thu, 10 Sep 2020 09:55:19 +0100 From: Bruce Richardson To: Omkar Maslekar Cc: dev@dpdk.org, ciara.loftus@intel.com Message-ID: <20200910085519.GB1789@bricha3-MOBL.ger.corp.intel.com> References: <1599700614-22809-1-git-send-email-omkar.maslekar@intel.com> <1599700614-22809-2-git-send-email-omkar.maslekar@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1599700614-22809-2-git-send-email-omkar.maslekar@intel.com> Subject: Re: [dpdk-dev] [PATCH] EAL: An addition of cache line demote (CLDEMOTE) in rte_prefetch.h X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- 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. > > - 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" :-) > + 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". > 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.