From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 86EA7A0547; Thu, 9 Sep 2021 14:45:28 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D2CCE40041; Thu, 9 Sep 2021 14:45:27 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id 4506F4003E for ; Thu, 9 Sep 2021 14:45:26 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10101"; a="200304299" X-IronPort-AV: E=Sophos;i="5.85,280,1624345200"; d="scan'208";a="200304299" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Sep 2021 05:45:25 -0700 X-IronPort-AV: E=Sophos;i="5.85,280,1624345200"; d="scan'208";a="548545601" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.3.161]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 09 Sep 2021 05:45:21 -0700 Date: Thu, 9 Sep 2021 13:45:18 +0100 From: Bruce Richardson To: Thomas Monjalon Cc: Chengwen Feng , ferruh.yigit@intel.com, jerinj@marvell.com, jerinjacobk@gmail.com, andrew.rybchenko@oktetlabs.ru, dev@dpdk.org, mb@smartsharesystems.com, nipun.gupta@nxp.com, hemant.agrawal@nxp.com, maxime.coquelin@redhat.com, honnappa.nagarahalli@arm.com, david.marchand@redhat.com, sburla@marvell.com, pkapoor@marvell.com, konstantin.ananyev@intel.com, conor.walsh@intel.com, kevin.laatz@intel.com Message-ID: References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <1855788.BnNMhiXu0z@thomas> <1912226.bRRTmGCQLz@thomas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1912226.bRRTmGCQLz@thomas> Subject: Re: [dpdk-dev] [PATCH v21 1/7] dmadev: introduce DMA device library public APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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 Thu, Sep 09, 2021 at 01:29:33PM +0200, Thomas Monjalon wrote: > 09/09/2021 13:18, Bruce Richardson: > > On Thu, Sep 09, 2021 at 12:33:00PM +0200, Thomas Monjalon wrote: > > > 07/09/2021 14:56, Chengwen Feng: > > > > + * The first three APIs are used to submit the operation request to the virtual > > > > + * DMA channel, if the submission is successful, an uint16_t ring_idx is > > > > + * returned, otherwise a negative number is returned. > > > > > > unsigned or negative? looks weird. > > > > May be, but it works well. We could perhaps rephase to make it less weird > > though: > > "if the submission is successful, a positive ring_idx <= UINT16_MAX is > > returned, otherwise a negative number is returned." > > I am advocating for int16_t, > it makes a lot of things simpler. > No, it doesn't work as you can't have wrap-around of the IDs once you use signed values - and that impacts both the end app and the internals of the drivers. Let's keep it as-is otherwise it will have massive impacts - including potential perf impacts. > > > > + * > > > > + * The last API was used to issue doorbell to hardware, and also there are flags > > > > + * (@see RTE_DMA_OP_FLAG_SUBMIT) parameter of the first three APIs could do the > > > > + * same work. > > > > > > I don't understand this sentence. > > > You mean rte_dmadev_submit function? > > > Why past tense "was"? > > > Why having a redundant function? > > > > > > > Just because there are two ways to do something does not mean that one of > > them is redundant, as both may be more suitable for different situations. > > I agree. > > > When enqueuing a set of jobs to the device, having a separate submit > > outside a loop makes for clearer code than having a check for the last > > iteration inside the loop to set a special submit flag. However, for cases > > where one item alone is to be submitted or there is a small set of jobs to > > be submitted sequentially, having a submit flag provides a lower-overhead > > way of doing the submission while still keeping the code clean. > > This kind of explanation may be missing in doxygen? > It can be added, sure. > > > > +bool > > > > +rte_dmadev_is_valid_dev(uint16_t dev_id); > > > > > > I would suggest dropping the final "_dev" in the function name. > > > > > > > The alternative, which I would support, is replacing "rte_dmadev" with > > "rte_dma" across the API. This would then become "rte_dma_is_valid_dev" > > which is clearer, since the dev is not part of the standard prefix. It also > > would fit in with a possible future function of "rte_dma_is_valid_vchan" > > for instance. > > Yes > The question is whether it would make sense to reserver rte_dma_ prefix > for some DMA functions which would be outside of dmadev lib? > If you think that all DMA functions will be in dmadev, > then yes we can shorten the prefix to rte_dma_. > Well, any DPDK dma functions which are not in dma library should have the prefix of the library they are in e.g. rte_eal_dma_*, rte_pci_dma_* Therefore, I don't think name conflicts should be an issue, and I like having less typing to do in function names (and I believe Morten was strongly proposing this previously too) > > > > +uint16_t > > > > +rte_dmadev_count(void); > > > > > > It would be safer to name it rte_dmadev_count_avail > > > in case we need new kind of device count later. > > > > > > > If we change "dmadev" to "dma" this could then be > > "rte_dma_count_avail_dev". > > Yes > > > > > +/** > > > > + * A structure used to retrieve the information of a DMA device. > > > > + */ > > > > +struct rte_dmadev_info { > > > > + struct rte_device *device; /**< Generic Device information. */ > > > > > > Please do not expose this. > > > > > > > + uint64_t dev_capa; /**< Device capabilities (RTE_DMADEV_CAPA_*). */ > > > > + uint16_t max_vchans; > > > > + /**< Maximum number of virtual DMA channels supported. */ > > > > + uint16_t max_desc; > > > > + /**< Maximum allowed number of virtual DMA channel descriptors. */ > > > > + uint16_t min_desc; > > > > + /**< Minimum allowed number of virtual DMA channel descriptors. */ > > > > + uint16_t max_sges; > > > > + /**< Maximum number of source or destination scatter-gather entry > > > > + * supported. > > > > + * If the device does not support COPY_SG capability, this value can be > > > > + * zero. > > > > + * If the device supports COPY_SG capability, then rte_dmadev_copy_sg() > > > > + * parameter nb_src/nb_dst should not exceed this value. > > > > + */ > > > > + uint16_t nb_vchans; /**< Number of virtual DMA channel configured. */ > > > > > > What about adding NUMA node? > > > > > > /* Local NUMA memory ID. -1 if unknown. */ > > > int16_t numa_node; > > > > > > > That was omitted as it could be got through the device structure. If device > > is removed, we need to ensure all fields needed from device, such as numa > > node, are made available here. > > Ah yes, forgot about rte_device :) > Yes please remove rte_device from this struct. >