From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier MATZ Subject: Re: [PATCH] mbuf: add helpers to prefetch mbuf Date: Tue, 10 May 2016 10:08:19 +0200 Message-ID: <573196F3.2030608@6wind.com> References: <1462810707-7434-1-git-send-email-olivier.matz@6wind.com> <645005AB-A5B0-43AC-8E44-AD8D6526DF3D@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit To: "Wiles, Keith" , "dev@dpdk.org" Return-path: Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id B97426A15 for ; Tue, 10 May 2016 10:08:31 +0200 (CEST) In-Reply-To: <645005AB-A5B0-43AC-8E44-AD8D6526DF3D@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, On 05/10/2016 12:02 AM, Wiles, Keith wrote: >> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h >> index 529debb..e3ee0b3 100644 >> --- a/lib/librte_mbuf/rte_mbuf.h >> +++ b/lib/librte_mbuf/rte_mbuf.h >> @@ -842,6 +842,44 @@ struct rte_mbuf { >> uint16_t timesync; >> } __rte_cache_aligned; >> >> +/** >> + * Prefetch the first part of the mbuf >> + * >> + * The first 64 bytes of the mbuf corresponds to fields that are used early >> + * in the receive path. If the cache line of the architecture is higher than >> + * 64B, the second part will also be prefetched. >> + * >> + * @param m >> + * The pointer to the mbuf. >> + */ >> +static inline void >> +rte_mbuf_prefetch_part0(struct rte_mbuf *m) >> +{ >> + rte_prefetch0(&m->cacheline0); >> +} >> + >> +/** >> + * Prefetch the second part of the mbuf >> + * >> + * The next 64 bytes of the mbuf corresponds to fields that are used in the >> + * transmit path. If the cache line of the architecture is higher than 64B, >> + * this function does nothing as it is expected that the full mbuf is >> + * already in cache. >> + * >> + * @param m >> + * The pointer to the mbuf. >> + */ >> +static inline void >> +rte_mbuf_prefetch_part1(struct rte_mbuf *m) >> +{ >> +#if RTE_CACHE_LINE_SIZE == 64 >> + rte_prefetch0(&m->cacheline1); >> +#else >> + RTE_SET_USED(m); >> +#endif >> +} > > I am not super happy with the names here, but I understand that rte_mbuf_prefetch_cacheline0() is a bit long. I could live with them being longer if that makes more sense and adds to readability. Naming these functions rte_mbuf_prefetch_cacheline0() and rte_mbuf_prefetch_cacheline1() was my first intention, but as you said, it's long, and it's also not accurate because here we don't really deal with cache lines, and that's why I preffered to use "part" instead. I'm not opposed to name them part1/part2 instead of part0/part1 as Thomas suggested. Another option would be: - rte_mbuf_prefetch_rx_part(m) - rte_mbuf_prefetch_tx_part(m) The objective is to avoid the drivers to deal with the two possible cache line sizes with #ifdefs. So I don't think the function should be called something_cacheline. As a side note, I'm not really satisfied by the RTE_CACHE_LINE_MIN_SIZE and __rte_cache_min_aligned macros and I think it would be clearer to explicitly align to 64. If other people agree, I can submit a patch for this too. Any comment? > Another idea is to have only one function for both: > > enum { MBUF_CACHELINE0 = 0, MBUF_CACHELINE1, MBUF_CACHELINES }; // Optional enum if you want > > static inline void > rte_mbuf_prefetch(struct rte_mbuf *m, unsigned cacheline) // Make sure we add a comment about the constant value > { > if (cacheline == MBUF_CACHELINE0) > rte_prefetch0(&m->cacheline0); > else if (cacheline == MBUF_CACHELINE1) > rte_prefetch0(&m->cacheline1); > else { > rte_prefetch0(&m->cacheline0); > rte_prefetch0(&m->cacheline1); > } > } > > I believe if you use constant value in the call for the cacheline variable then the extra code should be optimized out. If not then what about a macro instead. > > #define rte_mbuf_prefetch(m, c) \ > do { \ > if ((c) == MBUF_CACHELINE0) \ > rte_prefetch0(&(m)->cacheline0); \ > else if ((c) == MBUF_CACHELINE1) \ > rte_prefetch0(&(m)->cacheline1); \ > else { \ > rte_prefetch0(&(m)->cacheline0); \ > rte_prefetch0(&(m)->cacheline1); \ > } \ > } while((0)) > > Call like this: > rte_mbuf_prefetch(m, 0); // For cacheline 0 > rte_mbuf_prefetch(m, 1); // For cacheline 1 > rte_mbuf_prefetch(m, 2); // For cacheline 0 and 1 In my opinion, the implementation and usage is simpler with 2 separate functions. What would be the advantage of this? > We could have another routine: > rte_mbuf_prefetch_data(m, 0); // Prefetch the first cacheline of the packet data. Well, here, I think there is no need to replace rte_prefetch0(m->data). It's useful for mbuf structure prefetch because many drivers want to prefetch the rx part first, then the tx part. For data, the same function can be used whatever the cache line size. Regards, Olivier