DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Cheng Jiang <Cheng1.jiang@intel.com>
Cc: dev@dpdk.org, patrick.fu@intel.com
Subject: Re: [dpdk-dev] [PATCH 20.11 v2] raw/ioat: add a flag to control copying handle parameters
Date: Fri, 17 Jul 2020 13:49:37 +0100
Message-ID: <20200717124937.GC708@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <20200715021615.28467-1-Cheng1.jiang@intel.com>

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.jiang@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
> 

  reply	other threads:[~2020-07-17 12:49 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-13  7:15 [dpdk-dev] [PATCH 20.11] raw/ioat: added " Cheng Jiang
2020-07-13  9:31 ` Bruce Richardson
2020-07-13  9:55 ` Bruce Richardson
2020-07-13  9:59   ` Bruce Richardson
2020-07-13 13:28 ` Bruce Richardson
2020-07-14 10:45   ` Jiang, Cheng1
2020-07-15  2:16 ` [dpdk-dev] [PATCH 20.11 v2] raw/ioat: add " Cheng Jiang
2020-07-17 12:49   ` Bruce Richardson [this message]
2020-07-21 10:15     ` Bruce Richardson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200717124937.GC708@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=Cheng1.jiang@intel.com \
    --cc=dev@dpdk.org \
    --cc=patrick.fu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git