Re: [PATCH] vhost: optimize vhost memcpy

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Thu, Dec 01, 2016 at 08:19:42PM -0500, Zhihong Wang wrote:
> This patch optimizes Vhost performance for large packets when the
> Mergeable Rx buffer feature is enabled. It introduces a dedicated
> memcpy function for vhost enqueue/dequeue to replace rte_memcpy.
> 
> The reason is that rte_memcpy is for general cases, it handles
> unaligned copies and make store aligned, it even makes load aligned
> for micro architectures like Ivy Bridge. However alignment handling
> comes at a price: It introduces extra load/store instructions.
> 
> Vhost memcpy is rather special: The copy is aligned, and remote,
> and there is header write along which is also remote. In this case
> the memcpy instruction stream should be simplified, to reduce extra
> load/store, therefore reduce the probability of load/store buffer
> full caused pipeline stall, to let the actual memcpy instructions
> be issued and let H/W prefetcher goes to work as early as possible.
...
>  
> +/**
> + * This function is used to for vhost memcpy, to replace rte_memcpy.
> + * The reason is that rte_memcpy is for general cases, where vhost
> + * memcpy is a rather special case: The copy is aligned, and remote,
> + * and there is header write along which is also remote. In this case
> + * the memcpy instruction stream should be simplified to reduce extra
> + * load/store, therefore reduce the probability of load/store buffer
> + * full caused pipeline stall, to let the actual memcpy instructions
> + * be issued and let H/W prefetcher goes to work as early as possible.
> + */
> +static inline void __attribute__((always_inline))
> +vhost_memcpy(void *dst, const void *src, size_t n)

I like this function a lot, since it's really simple and straightforward!
Moreover, it performs better.

But, I don't quite like how this function is proposed:

- rte_movX are more like internal help functions that should be used only
  in corresponding rte_memcpy.h file.

- It's a good optimization, however, it will not benefit for other use
  cases, though vhost is the most typical case here.

- The optimization proves to be good for X86, but think there is no
  guarantee it may behave well for other platforms, say ARM.

I still would suggest you to go this way: move this function into x86's
rte_memcpy.h and call it when the data is well aligned.

	--yliu
> +{
> +	/* Copy size <= 16 bytes */
> +	if (n < 16) {
> +		if (n & 0x01) {
> +			*(uint8_t *)dst = *(const uint8_t *)src;
> +			src = (const uint8_t *)src + 1;
> +			dst = (uint8_t *)dst + 1;
> +		}
> +		if (n & 0x02) {
> +			*(uint16_t *)dst = *(const uint16_t *)src;
> +			src = (const uint16_t *)src + 1;
> +			dst = (uint16_t *)dst + 1;
> +		}
> +		if (n & 0x04) {
> +			*(uint32_t *)dst = *(const uint32_t *)src;
> +			src = (const uint32_t *)src + 1;
> +			dst = (uint32_t *)dst + 1;
> +		}
> +		if (n & 0x08)
> +			*(uint64_t *)dst = *(const uint64_t *)src;
> +
> +		return;
> +	}
> +
> +	/* Copy 16 <= size <= 32 bytes */
> +	if (n <= 32) {
> +		rte_mov16((uint8_t *)dst, (const uint8_t *)src);
> +		rte_mov16((uint8_t *)dst - 16 + n,
> +				(const uint8_t *)src - 16 + n);
> +
> +		return;
> +	}
> +
> +	/* Copy 32 < size <= 64 bytes */
> +	if (n <= 64) {
> +		rte_mov32((uint8_t *)dst, (const uint8_t *)src);
> +		rte_mov32((uint8_t *)dst - 32 + n,
> +				(const uint8_t *)src - 32 + n);
> +
> +		return;
> +	}
> +
> +	/* Copy 64 bytes blocks */
> +	for (; n >= 64; n -= 64) {
> +		rte_mov64((uint8_t *)dst, (const uint8_t *)src);
> +		dst = (uint8_t *)dst + 64;
> +		src = (const uint8_t *)src + 64;
> +	}
> +
> +	/* Copy whatever left */
> +	rte_mov64((uint8_t *)dst - 64 + n,
> +			(const uint8_t *)src - 64 + n);
> +}