Re: [dpdk-dev] [PATCH] mbuf: extend rte_mbuf_prefetch_part* to support more prefetching methods

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Jianbo,

On 05/31/2016 05:06 AM, Jianbo Liu wrote:
> Change the inline function to macro with parameters
> 
> Signed-off-by: Jianbo Liu <jianbo.liu@linaro.org>
>
> [...]
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -849,14 +849,15 @@ struct rte_mbuf {
>   * in the receive path. If the cache line of the architecture is higher than
>   * 64B, the second part will also be prefetched.
>   *
> + * @param method
> + *   The prefetch method: prefetch0, prefetch1, prefetch2 or
> + *                        prefetch_non_temporal.
> + *
>   * @param m
>   *   The pointer to the mbuf.
>   */
> -static inline void
> -rte_mbuf_prefetch_part1(struct rte_mbuf *m)
> -{
> -	rte_prefetch0(&m->cacheline0);
> -}
> +#define RTE_MBUF_PREFETCH_PART1(method, m)	\
> +	rte_##method(&(m)->cacheline0)

I'm not very fan of this macro, because it allows to
really do everything):

  RTE_MBUF_PREFETCH_PART1(pktmbuf_free, m)

would expand as:

  rte_pktmbuf_free(m)


I'd prefer to have a switch case like this, almost similar
to what Keith proposed in the initial discussion for my
patch:

enum rte_mbuf_prefetch_type {
	PREFETCH0,
	PREFETCH1,
...
};

static inline void
rte_mbuf_prefetch_part1(enum rte_mbuf_prefetch_type type,
	struct rte_mbuf *m)
{
	switch (type) {
	case PREFETCH0:
		rte_prefetch0(&m->cacheline0);
		break;
	case PREFETCH1:
		rte_prefetch1(&m->cacheline0);
		break;
	...
}


Some questions: could you give some details about the use
of non-temporal prefetch in ixgbe_vec_neon? What are the
pros and cons, and would it be useful in other drivers?
Currently all drivers are doing prefetch0 when they prefetch
the mbuf structure. Some drivers use prefetch1 for data.


By the way, I did not try to apply the patch, but it looks
it's on top of dpdk-next-net/rel_16_07, right?

Thanks,
Olivier