From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 87D1FA053B; Fri, 17 Jul 2020 14:49:45 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 913EF1C10A; Fri, 17 Jul 2020 14:49:44 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 6E00D1C0D7 for ; Fri, 17 Jul 2020 14:49:42 +0200 (CEST) IronPort-SDR: uUKPKV/qoTBDrNPiW6rMTVfujbOdSygA+LW2JwhsS+XcJeM60WNzk8LIeW7HQ7YPb00TlSTFY/ eIF05MCtr3uQ== X-IronPort-AV: E=McAfee;i="6000,8403,9684"; a="137695142" X-IronPort-AV: E=Sophos;i="5.75,362,1589266800"; d="scan'208";a="137695142" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Jul 2020 05:49:41 -0700 IronPort-SDR: hZFfhWpFH0wF7LOUMFi7LlK+MFUQummz6GdolHm21VbhI2NjN/R4itAXDo7vqON0DAOed9KgHw D+SR898rJaJw== X-IronPort-AV: E=Sophos;i="5.75,362,1589266800"; d="scan'208";a="430831633" Received: from bricha3-mobl.ger.corp.intel.com ([10.213.193.115]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 17 Jul 2020 05:49:40 -0700 Date: Fri, 17 Jul 2020 13:49:37 +0100 From: Bruce Richardson To: Cheng Jiang Cc: dev@dpdk.org, patrick.fu@intel.com Message-ID: <20200717124937.GC708@bricha3-MOBL.ger.corp.intel.com> References: <20200713071519.110662-1-Cheng1.jiang@intel.com> <20200715021615.28467-1-Cheng1.jiang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200715021615.28467-1-Cheng1.jiang@intel.com> Subject: Re: [dpdk-dev] [PATCH 20.11 v2] raw/ioat: add a flag to control copying handle parameters X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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 >