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 DA6F9A0547; Thu, 9 Sep 2021 15:54:32 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 99D8241145; Thu, 9 Sep 2021 15:54:32 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 9535640041 for ; Thu, 9 Sep 2021 15:54:30 +0200 (CEST) Received: from dggemv711-chm.china.huawei.com (unknown [172.30.72.53]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4H50kQ55Vlz8yCr; Thu, 9 Sep 2021 21:50:06 +0800 (CST) Received: from dggpeml500024.china.huawei.com (7.185.36.10) by dggemv711-chm.china.huawei.com (10.1.198.66) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.8; Thu, 9 Sep 2021 21:54:28 +0800 Received: from [10.40.190.165] (10.40.190.165) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.8; Thu, 9 Sep 2021 21:54:27 +0800 To: Bruce Richardson , Thomas Monjalon CC: , , , , , , , , , , , , , , , References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <1855788.BnNMhiXu0z@thomas> <1912226.bRRTmGCQLz@thomas> From: fengchengwen Message-ID: <4108b879-a365-67db-72bb-1deb8440fa73@huawei.com> Date: Thu, 9 Sep 2021 21:54:27 +0800 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: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.40.190.165] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected 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 2021/9/9 20:45, Bruce Richardson wrote: > 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) The dmadev is rather short, if change I prefer all public API with rte_dma_ prefix, and don't have rte_dma_dev_ prefix for such start/stop/close. (ps: the rte_eth_ also have rte_eth_dev_close which is painful for OCD). Also should the filename(e.g. rte_dmadev.h) and directory-name(lib/dmadev) also change ? > >>>>> +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. >> > . >