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 C620EA0C4D; Thu, 15 Jul 2021 08:44:34 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7418340143; Thu, 15 Jul 2021 08:44:34 +0200 (CEST) Received: from mail-il1-f172.google.com (mail-il1-f172.google.com [209.85.166.172]) by mails.dpdk.org (Postfix) with ESMTP id DC45840140 for ; Thu, 15 Jul 2021 08:44:32 +0200 (CEST) Received: by mail-il1-f172.google.com with SMTP id b14so4058020ilf.7 for ; Wed, 14 Jul 2021 23:44:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=Row1mOpJiPhiRsvYgJKvl91G9eH7/6GlG9x0bcdfWgs=; b=OOvhQUMv5YiMf8WRcqmXeSNlq+7R89TuwOMomlee3wy0C/Ni3PPL5LDC1jzaolgDsH Sxvd6Vsrjrm2ZeLHpvoFdEprpF8AswXAMurZKVOEgDyI2VSZ5omkv508dwTaBErFUlZh YJLroZcPmUoiUEwpHVHp5cACztF52MxbH9jMP4acmBXe7B/D51xIgqm/xUZiiOHozXdA K3cQsUHLgFiWVLw8xybTLz5VO8AHQzQW2/KQu1NjJaeHdgy8h+Yo+mQ/rPkDEK0OhmvE vpFAyOITALdWj0PKzcUw0Os2Hh0SuQJ5KWAqI+bYADy4GRZTPQ5be5mWvS4h4Jk5xRhu i+Cg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=Row1mOpJiPhiRsvYgJKvl91G9eH7/6GlG9x0bcdfWgs=; b=KMIbNbylP3GGDhwnBeMdOU6CU3KqBbEQDtaOwvozdX36mn/9wenMsNZuKvUWbKf5ID 7Cf+A7Op3ATDeLK4E3j2nd8YuLpoeGqRMx628SNUtS2BycfG/NS3CkUFliRW7n6baoCO jhxhIXUqM4rkOUICeWvJz1ystc8Oq1WPFza1sOM0PHQRC9lIlT9CCat8PHRkc2p+UdWN a1DgLbaY2X+DEdaiDTe5CWaFUyw2Q8mRreXJtoxdmizNo0hjmTBmfPuEcoai0Twtpiuk okMalt2RdlXGU5VpQlxkIpVXBWWKZUO5IRInTV4i8kIWooG6G5+QMtpdo5c3xHm2CYtW xfSQ== X-Gm-Message-State: AOAM531phrT2Ynho6BK9qNyGxazZDvSmS43Pc55qZYH7kxQbuBdlN45E TIvHWKZDcKLouIbWPmIXSSiwrpuFDCXjq53JYVQ= X-Google-Smtp-Source: ABdhPJzstLNrejSoStCMV8/r0i8IKFoZ1vprQh3ERP1IKXlU2sw5P/Rejx9MDyRe4g2OXYQ+YdG4SYHlmqmlgl7uGHg= X-Received: by 2002:a92:bf0b:: with SMTP id z11mr1743378ilh.60.1626331472103; Wed, 14 Jul 2021 23:44:32 -0700 (PDT) MIME-Version: 1.0 References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <1626179263-14645-1-git-send-email-fengchengwen@huawei.com> <02340ac8-9d09-15e8-3969-1850f08e5831@huawei.com> In-Reply-To: From: Jerin Jacob Date: Thu, 15 Jul 2021 12:14:05 +0530 Message-ID: To: Bruce Richardson Cc: fengchengwen , Thomas Monjalon , Ferruh Yigit , Jerin Jacob , Andrew Rybchenko , dpdk-dev , =?UTF-8?Q?Morten_Br=C3=B8rup?= , Nipun Gupta , Hemant Agrawal , Maxime Coquelin , Honnappa Nagarahalli , David Marchand , Satananda Burla , Prasun Kapoor , "Ananyev, Konstantin" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH v3] dmadev: introduce DMA device library 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 Tue, Jul 13, 2021 at 7:08 PM Bruce Richardson wrote: > > On Tue, Jul 13, 2021 at 09:06:39PM +0800, fengchengwen wrote: > > Thank you for your valuable comments, and I think we've taken a big ste= p forward. > > > > @andrew Could you provide the copyright line so that I can add it to re= levant file. > > > > @burce, jerin Some unmodified review comments are returned here: > > Thanks. Some further comments inline below. Most points you make I'm ok > with, but I do disagree on a number of others. > > /Bruce > > > > > 1. > > COMMENT: We allow up to 100 characters per line for DPDK code, so these= don't need > > to be wrapped so aggressively. > > > > REPLY: Our CI still has 80 characters limit, and I review most framewor= k still comply. > > > Ok. > > > 2. > > COMMENT: > +#define RTE_DMA_MEM_TO_MEM (1ull << 0) > > RTE_DMA_DIRECTION_... > > > > REPLY: add the 'DIRECTION' may the macro too long, I prefer keep it sim= ple. > > > DIRECTION could be shortened to DIR, but I think this is probably ok as i= s > too. > I prefer to keep DIR so that it easy to point in documentation like @see RTE_DMA_DIR_* > > 3. > > COMMENT: > +rte_dmadev_vchan_release(uint16_t dev_id, uint16_t vchan); > > We are not making release as pubic API in other device class. See ethde= v spec. > > bbdev/eventdev/rawdev > > > > REPLY: because ethdev's queue is hard-queue, and here is the software d= efined channels, > > I think release is OK, BTW: bbdev/eventdev also have release ops. I don't see any API like rte_event_queue_release() in event dev. It has the only setup. Typical flow is 1) configure() the N vchan 2) for i..N setup() the chan 3) start() 3) stop() 4) configure again with M vchan 5) for i..M setup() the chan 5) start() And above is documented at the beginning of the rte_dmadev.h header file. I think, above sequence makes it easy for drivers. Just like other device class _release can be PMD hook which will be handled in configure() common code. > > > Ok > > 4. COMMENT:> + uint64_t reserved[4]; /**< Reserved for future > > fields */ > > > +}; > > Please add the capability for each counter in info structure as one > > device may support all the counters. > > > > REPLY: This is a statistics function. If this function is not supported= , > > then do not need to implement the stats ops function. Also could to set > > the unimplemented ones to zero. > > > +1 > The stats functions should be a minimum set that is supported by all > drivers. Each of these stats can be easily tracked by software if HW > support for it is not available, so I agree that we should not have each > stat as a capability. In our current HW, submitted_count and completed_count offloaded to HW. In addition to that, we have a provision for getting stats for bytes copied.( We can make it as xstat, if other drivers won't support) our plan is to use enqueued_count and completed_fail_count in SW under condition compilation flags or another scheme as it is in fastpath. If we are not planning to add capability, IMO, we need to update the documentation, like unimplemented counters will return zero. But there is the question of how to differentiate between unimplemented vs genuine zero value. IMO, we can update the doc for this case as well or add capability. > > > 5. > > COMMENT: > +#endif > > > + return (*dev->fill)(dev, vchan, pattern, dst, length, flags); > > Instead of every driver set the NOP function, In the common code, If > > the CAPA is not set, > > common code can set NOP function for this with <0 return value. > > > > REPLY: I don't think it's a good idea to judge in IO path, it's applica= tion duty to ensure > > don't call API which driver not supported (which could get from capabil= ities). > > > For datapath functions, +1. OK. Probably add some NOP function(returns it as error) in pmd.h so that all drivers can reuse. No strong opnion. > > > 6. > > COMMENT: > +rte_dmadev_completed_fails(uint16_t dev_id, uint16_t vchan, > > > + const uint16_t nb_status, uint32_t *status= , > > uint32_t -> enum rte_dma_status_code > > > > REPLY=EF=BC=9AI'm still evaluating this. It takes a long time for the d= river to perform error code > > conversion in this API. Do we need to provide an error code conversion = function alone ? > > > It's not that difficult a conversion to do, and so long as we have the > regular "completed" function which doesn't do all the error manipulation = we > should be fine. Performance in the case of errors is not expected to be a= s > good, since errors should be very rare. +1 > > > 7. > > COMMENT: > +typedef int (*dmadev_info_get_t)(struct rte_dmadev *dev, > > > + struct rte_dmadev_info *dev_info); > > Please change to rte_dmadev_info_get_t to avoid conflict due to namespa= ce issue > > as this header is exported. > > > > REPLY: I prefer not add 'rte_' prefix, it make the define too long. > > > I disagree on this, they need the rte_ prefix, despite the fact it makes > them longer. If length is a concern, these can be changed from "dmadev_" = to > "rte_dma_", which is only one character longer. > In fact, I believe Morten already suggested we use "rte_dma" rather than > "rte_dmadev" as a function prefix across the library. +1 > > > 8. > > COMMENT: > + * - rte_dmadev_completed_fails() > > > + * - return the number of operation requests failed to co= mplete. > > Please rename this to "completed_status" to allow the return of informa= tion > > other than just errors. As I suggested before, I think this should also= be > > usable as a slower version of "completed" even in the case where there = are > > no errors, in that it returns status information for each and every job > > rather than just returning as soon as it hits a failure. > > > > REPLY: well, I think it maybe confuse (current OK/FAIL API is easy to u= nderstand.), > > and we can build the slow path function on the two API. > > > I still disagree on this too. We have a "completed" op where we get > informed of what has completed and minimal error indication, and a > "completed_status" operation which provides status information for each > operation completed, at the cost of speed. +1 > > > 9. > > COMMENT: > +#define RTE_DMA_DEV_CAPA_MEM_TO_MEM (1ull << 0) > > > +/**< DMA device support mem-to-mem transfer. > > Do we need this? Can we assume that any device appearing as a dmadev ca= n > > do mem-to-mem copies, and drop the capability for mem-to-mem and the > > capability for copying? > > also for RTE_DMA_DEV_CAPA_OPS_COPY > > > > REPLY: yes, I insist on adding this for the sake of conceptual integrit= y. > > For ioat driver just make a statement. > > > > Ok. It seems a wasted bit to me, but I don't see us running out of them > soon. > > > 10. > > COMMENT: > + uint16_t nb_vchans; /**< Number of virtual DMA channel co= nfigured */ > > > +}; > > Let's add rte_dmadev_conf struct into this to return the configuration > > settings. > > > > REPLY: If we add rte_dmadev_conf in, it may break ABI when rte_dmadev_c= onf add fields. > > > Yes, that is true, but I fail to see why that is a major problem. It just > means that if the conf structure changes we have two functions to version > instead of one. The information is still useful. > > If you don't want the actual conf structure explicitly put into the info > struct, we can instead put the fields in directly. I really think that th= e > info_get function should provide back to the user the details of what way > the device was configured previously. > > regards, > /Bruce