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 C6D2EA0540; Mon, 13 Jul 2020 11:59:25 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6C9961D5F3; Mon, 13 Jul 2020 11:59:18 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id CD8D91C19C for ; Mon, 13 Jul 2020 11:59:15 +0200 (CEST) IronPort-SDR: rg531JT46wE/KhtWiGEcBQUIZJP3WgDB8MYMXtCd6Z+s/SlQu4tGlW8NWAnj9F6eACN73Z7QnV iYpc94ul71QA== X-IronPort-AV: E=McAfee;i="6000,8403,9680"; a="128159084" X-IronPort-AV: E=Sophos;i="5.75,347,1589266800"; d="scan'208";a="128159084" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Jul 2020 02:59:15 -0700 IronPort-SDR: oITTn/vrBlb00AApD+leR42TgBdEPJPNuOSmzrrMACz4WUltiYGaO26mpN4H8kJBYPu8g9aivL huvNkgeDNsEw== X-IronPort-AV: E=Sophos;i="5.75,347,1589266800"; d="scan'208";a="459237131" Received: from bricha3-mobl.ger.corp.intel.com ([10.249.32.149]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 13 Jul 2020 02:59:13 -0700 Date: Mon, 13 Jul 2020 10:59:10 +0100 From: Bruce Richardson To: Cheng Jiang Cc: dev@dpdk.org, patrick.fu@intel.com Message-ID: <20200713095910.GE694@bricha3-MOBL.ger.corp.intel.com> References: <20200713071519.110662-1-Cheng1.jiang@intel.com> <20200713095530.GD694@bricha3-MOBL.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200713095530.GD694@bricha3-MOBL.ger.corp.intel.com> Subject: Re: [dpdk-dev] [PATCH 20.11] raw/ioat: added 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 Mon, Jul 13, 2020 at 10:55:30AM +0100, Bruce Richardson wrote: > On Mon, Jul 13, 2020 at 07:15:19AM +0000, Cheng Jiang wrote: > > Added 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 > > --- > > drivers/raw/ioat/ioat_rawdev.c | 1 + > > drivers/raw/ioat/rte_ioat_rawdev.h | 14 +++++++++++--- > > 2 files changed, 12 insertions(+), 3 deletions(-) > > > > > @@ -208,6 +213,11 @@ rte_ioat_completed_copies(int dev_id, uint8_t max_copies, > > if (count > max_copies) > > count = max_copies; > > > > + ioat->next_read = read + count; > > + ioat->completed += count; > > + if (!ioat->hdls_enable) > > + return count; > > + > > 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]); > > @@ -223,8 +233,6 @@ rte_ioat_completed_copies(int dev_id, uint8_t max_copies, > > dst_hdls[i] = hdls[1]; > > } > > > > - ioat->next_read = read; > > - ioat->completed += count; > > return count; > > } > > This change I think may cause problems if we ever want to have one thread > enqueuing and another taking completions. The next_read and completed > counters should really only be updated after we have finished reading the > completed handles array. Therefore, for safety, I tihnk it might be better > to keep the updates in their original places and put an "end:" label before > them. Then the "return count" in the middle of the function can be "goto > end;" > A further suggestion to the changes to this function: if we are not actually returning completion handles, then there is no need to limit the count to "max_copies". Therefore move the "return count" or "goto end" above the previous length check, and update the doxygen comments to indicate that max_copies is ignored if "hdls_enable" is false, and that the final two parameters can also be NULL when calling the function in this case. /Bruce