Re: [dpdk-dev] [PATCH v2 02/18] raw/ioat: split header for readability

["Laatz, Kevin" <kevin.laatz@intel.com>]

On 21/08/2020 17:29, Bruce Richardson wrote:
> Rather than having a single long complicated header file for general use we
> can split things so that there is one header with all the publically needed
> information - data structs and function prototypes - while the rest of the
> internal details are put separately. This makes it easier to read,
> understand and use the APIs.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>
> There are a couple of checkpatch errors about spacing in this patch,
> however, it appears that these are false positives.
> ---
>   drivers/raw/ioat/meson.build           |   1 +
>   drivers/raw/ioat/rte_ioat_rawdev.h     | 144 +---------------------
>   drivers/raw/ioat/rte_ioat_rawdev_fns.h | 164 +++++++++++++++++++++++++
>   3 files changed, 171 insertions(+), 138 deletions(-)
>   create mode 100644 drivers/raw/ioat/rte_ioat_rawdev_fns.h
>
<snip>
> diff --git a/drivers/raw/ioat/rte_ioat_rawdev.h b/drivers/raw/ioat/rte_ioat_rawdev.h
> index 4bc6491d91..7ace5c085a 100644
> --- a/drivers/raw/ioat/rte_ioat_rawdev.h
> +++ b/drivers/raw/ioat/rte_ioat_rawdev.h
> @@ -14,12 +14,7 @@
>    * @b EXPERIMENTAL: these structures and APIs may change without prior notice
>    */
>   
> -#include <x86intrin.h>
> -#include <rte_atomic.h>
> -#include <rte_memory.h>
> -#include <rte_memzone.h>
> -#include <rte_prefetch.h>
> -#include "rte_ioat_spec.h"
> +#include <rte_common.h>
>   
>   /** Name of the device driver */
>   #define IOAT_PMD_RAWDEV_NAME rawdev_ioat
> @@ -38,38 +33,6 @@ struct rte_ioat_rawdev_config {
>   	bool hdls_disable;    /**< if set, ignore user-supplied handle params */
>   };
>   
> -/**
> - * @internal
> - * Structure representing a device instance
> - */
> -struct rte_ioat_rawdev {
> -	struct rte_rawdev *rawdev;
> -	const struct rte_memzone *mz;
> -	const struct rte_memzone *desc_mz;
> -
> -	volatile struct rte_ioat_registers *regs;
> -	phys_addr_t status_addr;
> -	phys_addr_t ring_addr;
> -
> -	unsigned short ring_size;
> -	struct rte_ioat_generic_hw_desc *desc_ring;
> -	bool hdls_disable;
> -	__m128i *hdls; /* completion handles for returning to user */
> -
> -
> -	unsigned short next_read;
> -	unsigned short next_write;
> -
> -	/* some statistics for tracking, if added/changed update xstats fns*/
> -	uint64_t enqueue_failed __rte_cache_aligned;
> -	uint64_t enqueued;
> -	uint64_t started;
> -	uint64_t completed;
> -
> -	/* to report completions, the device will write status back here */
> -	volatile uint64_t status __rte_cache_aligned;
> -};
> -
>   /**
>    * Enqueue a copy operation onto the ioat device
>    *
> @@ -104,38 +67,7 @@ struct rte_ioat_rawdev {
>   static inline int
>   rte_ioat_enqueue_copy(int dev_id, phys_addr_t src, phys_addr_t dst,
>   		unsigned int length, uintptr_t src_hdl, uintptr_t dst_hdl,
> -		int fence)
> -{
> -	struct rte_ioat_rawdev *ioat = rte_rawdevs[dev_id].dev_private;


This assignment needs to be type cast to "struct rte_ioat_rawdev *" for 
C++ compilation compatibility.

There are a number of occurrences of this in this patch.


> -	unsigned short read = ioat->next_read;
> -	unsigned short write = ioat->next_write;
> -	unsigned short mask = ioat->ring_size - 1;
> -	unsigned short space = mask + read - write;
> -	struct rte_ioat_generic_hw_desc *desc;
> -
> -	if (space == 0) {
> -		ioat->enqueue_failed++;
> -		return 0;
> -	}
> -
> -	ioat->next_write = write + 1;
> -	write &= mask;
> -
> -	desc = &ioat->desc_ring[write];
> -	desc->size = length;
> -	/* set descriptor write-back every 16th descriptor */
> -	desc->u.control_raw = (uint32_t)((!!fence << 4) | (!(write & 0xF)) << 3);
> -	desc->src_addr = src;
> -	desc->dest_addr = dst;
> -	if (!ioat->hdls_disable)
> -		ioat->hdls[write] = _mm_set_epi64x((int64_t)dst_hdl,
> -					(int64_t)src_hdl);
> -
> -	rte_prefetch0(&ioat->desc_ring[ioat->next_write & mask]);
> -
> -	ioat->enqueued++;
> -	return 1;
> -}
> +		int fence);
>   
<snip>
> +/**
> + * Returns details of copy operations that have been completed
> + */
> +static inline int
> +rte_ioat_completed_copies(int dev_id, uint8_t max_copies,
> +		uintptr_t *src_hdls, uintptr_t *dst_hdls)
> +{
> +	struct rte_ioat_rawdev *ioat = rte_rawdevs[dev_id].dev_private;
> +	unsigned short mask = (ioat->ring_size - 1);
> +	unsigned short read = ioat->next_read;
> +	unsigned short end_read, count;
> +	int error;
> +	int i = 0;
> +
> +	end_read = (rte_ioat_get_last_completed(ioat, &error) + 1) & mask;
> +	count = (end_read - (read & mask)) & mask;
> +
> +	if (error) {
> +		rte_errno = EIO;
> +		return -1;
> +	}
> +
> +	if (ioat->hdls_disable) {
> +		read += count;
> +		goto end;
> +	}
> +
> +	if (count > max_copies)
> +		count = max_copies;
> +
> +	for (; i < count - 1; i += 2, read += 2) {
> +		__m128i hdls0 = _mm_load_si128(&ioat->hdls[read & mask]);
> +		__m128i hdls1 = _mm_load_si128(&ioat->hdls[(read + 1) & mask]);
> +
> +		_mm_storeu_si128((void *)&src_hdls[i],
> +				_mm_unpacklo_epi64(hdls0, hdls1));
> +		_mm_storeu_si128((void *)&dst_hdls[i],
> +				_mm_unpackhi_epi64(hdls0, hdls1));

"src_hdls" and "dst_hdls" need to be type cast to "__m128i *" here for 
C++ compatibility.


> +	}
> +	for (; i < count; i++, read++) {
> +		uintptr_t *hdls = (void *)&ioat->hdls[read & mask];

Type cast for "ioat->hdls" to "__m128i *" needed here for C++ compatibility.


> +		src_hdls[i] = hdls[0];
> +		dst_hdls[i] = hdls[1];
> +	}
> +
> +end:
> +	ioat->next_read = read;
> +	ioat->completed += count;
> +	return count;
> +}
> +
> +#endif /* _RTE_IOAT_RAWDEV_FNS_H_ */

Thanks,
Kevin