On Wed, Jul 15, 2020 at 02:16:15AM +0000, Cheng Jiang wrote:
> Add a flag which controls whether rte_ioat_enqueue_copy
> and rte_ioat_completed_copies function should process
> handle parameters to improve the performance when handle
> parameters are not necessary to use. This is targeting
> 20.11 release.
>
> Signed-off-by: Cheng Jiang <cheng1.ji...@intel.com>
> ---
> v2:
> * optimized the logic of some codes
> * added some comments
> ---
> drivers/raw/ioat/ioat_rawdev.c | 1 +
> drivers/raw/ioat/rte_ioat_rawdev.h | 29 ++++++++++++++++++++++-------
> 2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/raw/ioat/ioat_rawdev.c b/drivers/raw/ioat/ioat_rawdev.c
> index 87fd088aa..d70e47d52 100644
> --- a/drivers/raw/ioat/ioat_rawdev.c
> +++ b/drivers/raw/ioat/ioat_rawdev.c
> @@ -57,6 +57,7 @@ ioat_dev_configure(const struct rte_rawdev *dev,
> rte_rawdev_obj_t config)
> return -EINVAL;
>
> ioat->ring_size = params->ring_size;
> + ioat->hdls_disable = params->hdls_disable;
> if (ioat->desc_ring != NULL) {
> rte_memzone_free(ioat->desc_mz);
> ioat->desc_ring = NULL;
> diff --git a/drivers/raw/ioat/rte_ioat_rawdev.h
> b/drivers/raw/ioat/rte_ioat_rawdev.h
> index f765a6557..cf0e634f3 100644
> --- a/drivers/raw/ioat/rte_ioat_rawdev.h
> +++ b/drivers/raw/ioat/rte_ioat_rawdev.h
> @@ -31,10 +31,13 @@
> *
> * This structure is to be passed as the ".dev_private" parameter when
> * calling the rte_rawdev_get_info() and rte_rawdev_configure() APIs on
> - * an ioat rawdev instance.
> + * an ioat rawdev instance. The member hdls_disable controls if handles
> + * need to be copied when calling the rte_ioat_enqueue_copy() and
> + * rte_ioat_completed_copies() APIs.
> */
> struct rte_ioat_rawdev_config {
> unsigned short ring_size;
> + bool hdls_disable;
> };
>
> /**
> @@ -52,6 +55,7 @@ struct rte_ioat_rawdev {
>
> unsigned short ring_size;
> struct rte_ioat_generic_hw_desc *desc_ring;
> + bool hdls_disable;
> __m128i *hdls; /* completion handles for returning to user */
>
>
> @@ -84,10 +88,12 @@ struct rte_ioat_rawdev {
> * The length of the data to be copied
> * @param src_hdl
> * An opaque handle for the source data, to be returned when this operation
> - * has been completed and the user polls for the completion details
> + * has been completed and the user polls for the completion details if
> + * hdls_disable is false
> * @param dst_hdl
> * An opaque handle for the destination data, to be returned when this
> * operation has been completed and the user polls for the completion
> details
> + * if hdls_disable is false
Minor nit - rather than adding "if hdls_disable is false" to the sentence,
I think it would be clearer to just add on as an extra sentence: "NOTE: If
hdls_disable configuration option is set, this parameter is ignored"
> * @param fence
> * A flag parameter indicating that hardware should not begin to perform
> any
> * subsequently enqueued copy operations until after this operation has
> @@ -121,8 +127,10 @@ rte_ioat_enqueue_copy(int dev_id, phys_addr_t src,
> phys_addr_t dst,
> 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);
>
> - 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++;
> @@ -168,9 +176,11 @@ rte_ioat_get_last_completed(struct rte_ioat_rawdev
> *ioat, int *error)
> /**
> * Returns details of copy operations that have been completed
> *
> - * Returns to the caller the user-provided "handles" for the copy operations
> - * which have been completed by the hardware, and not already returned by
> - * a previous call to this API.
> + * If the hdls_disable is false, the function will return to the caller the
> + * user-provided "handles" for the copy operations which have been completed
> + * by the hardware, and not already returned by a previous call to this API.
> + * If the hdls_disable is true, the max_copies will be ignored, and that the
> + * src_hdls and dst_hdls can be NULL when calling the function.
Again I think this might be better left as originally stated, with the
additional case of "hdls_disable" being set left till the end. Rather than
referring to "true/false" set/not-set are better terms to use.
> *
> * @param dev_id
> * The rawdev device id of the ioat instance
> @@ -205,6 +215,11 @@ rte_ioat_completed_copies(int dev_id, uint8_t max_copies,
> return -1;
> }
>
> + if (ioat->hdls_disable) {
> + read += count;
> + goto end;
> + }
> +
> if (count > max_copies)
> count = max_copies;
>
> @@ -222,7 +237,7 @@ rte_ioat_completed_copies(int dev_id, uint8_t max_copies,
> src_hdls[i] = hdls[0];
> dst_hdls[i] = hdls[1];
> }
> -
> +end:
> ioat->next_read = read;
> ioat->completed += count;
> return count;
> --
> 2.27.0
>
