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 36A35A04B1; Tue, 25 Aug 2020 17:27:41 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0FE38B62; Tue, 25 Aug 2020 17:27:40 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id C5D1A23D for ; Tue, 25 Aug 2020 17:27:38 +0200 (CEST) IronPort-SDR: C+LTNjqIVfaMtOZb9nagHhIt0RgdEJG9T3VMOgEz/XF2vzFHhlGI+cKL0j5bu+sk84OS10oLfv ezNzLJussWJQ== X-IronPort-AV: E=McAfee;i="6000,8403,9723"; a="143794469" X-IronPort-AV: E=Sophos;i="5.76,353,1592895600"; d="scan'208";a="143794469" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Aug 2020 08:27:37 -0700 IronPort-SDR: 7HE/Ez1kJKVfKKpGuhbCOyfaZ4ih6rZAVHaAKp2YsuGaqU/8zwwLofT0wAO6ecJ7bMqRqcIJEa FkvKaaIc5Ecw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.76,353,1592895600"; d="scan'208";a="322796944" Received: from klaatz-mobl1.ger.corp.intel.com (HELO [10.213.235.49]) ([10.213.235.49]) by fmsmga004.fm.intel.com with ESMTP; 25 Aug 2020 08:27:36 -0700 To: Bruce Richardson , dev@dpdk.org Cc: cheng1.jiang@intel.com, patrick.fu@intel.com, ping.yu@intel.com References: <20200721095140.719297-1-bruce.richardson@intel.com> <20200821162944.29840-1-bruce.richardson@intel.com> <20200821162944.29840-3-bruce.richardson@intel.com> From: "Laatz, Kevin" Message-ID: <1b7425a6-fe70-f68c-d314-59c12394d01a@intel.com> Date: Tue, 25 Aug 2020 16:27:35 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <20200821162944.29840-3-bruce.richardson@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v2 02/18] raw/ioat: split header for readability 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 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 > --- > > 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 > > 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 > -#include > -#include > -#include > -#include > -#include "rte_ioat_spec.h" > +#include > > /** 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); > > +/** > + * 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