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 E7AFEA034F; Mon, 11 Oct 2021 14:31:48 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D958C40E01; Mon, 11 Oct 2021 14:31:47 +0200 (CEST) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id 8ADB540150 for ; Mon, 11 Oct 2021 14:31:45 +0200 (CEST) Received: from dggemv703-chm.china.huawei.com (unknown [172.30.72.54]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4HSdRN5X1zz1DHYd; Mon, 11 Oct 2021 20:30:08 +0800 (CST) Received: from dggpeml500024.china.huawei.com (7.185.36.10) by dggemv703-chm.china.huawei.com (10.3.19.46) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.8; Mon, 11 Oct 2021 20:31:43 +0800 Received: from [127.0.0.1] (10.67.100.224) 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; Mon, 11 Oct 2021 20:31:42 +0800 To: Bruce Richardson CC: , , , , , , , , , , , , , , , , References: <1625231891-2963-1-git-send-email-fengchengwen@huawei.com> <20211009093340.43237-1-fengchengwen@huawei.com> <20211009093340.43237-4-fengchengwen@huawei.com> From: fengchengwen Message-ID: <6a2b5dc7-908b-d238-629f-cde5c5008fe6@huawei.com> Date: Mon, 11 Oct 2021 20:31:42 +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.67.100.224] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v24 3/6] dmadev: add data plane API support 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/10/11 18:40, Bruce Richardson wrote: > On Sat, Oct 09, 2021 at 05:33:37PM +0800, Chengwen Feng wrote: >> This patch add data plane API for dmadev. >> > > A few initial comments inline. I'll work on rebasing my follow-up patchset > to this, and let you know if I have any more feedback based on that. > > /Bruce > >> diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c >> index a6a5680d2b..891ceeb988 100644 >> --- a/lib/dmadev/rte_dmadev.c >> +++ b/lib/dmadev/rte_dmadev.c >> @@ -17,6 +17,7 @@ >> >> static int16_t dma_devices_max; >> >> +struct rte_dma_fp_object *rte_dma_fp_objs; > > While I think I like this approach of making more of the dmadev hidden, I > think we need a better name for this. While there is the dev_private > pointer in it, the struct is pretty much the datapath functions, so how > about "rte_dma_funcs" as a name? en, I notice ethdev and eventdev both use rte_xxx_fp_ops, but this structure has other fileds(e.g. data pointers) in addition to ops, it's inappropriate to use ops suffix. So I use the 'object' which is widely used in object-oriented. It's better to use uniform naming in ethdev/eventdev/dmadev and so on, would be happy to hear more. > >> struct rte_dma_dev *rte_dma_devices; >> > > > >> +/** >> + * @internal >> + * Fast-path dmadev functions and related data are hold in a flat array. >> + * One entry per dmadev. >> + * >> + * On 64-bit systems contents of this structure occupy exactly two 64B lines. >> + * On 32-bit systems contents of this structure fits into one 64B line. >> + * >> + * The 'dev_private' field was placed in the first cache line to optimize >> + * performance because the PMD driver mainly depends on this field. >> + */ >> +struct rte_dma_fp_object { >> + void *dev_private; /**< PMD-specific private data. */ >> + rte_dma_copy_t copy; >> + rte_dma_copy_sg_t copy_sg; >> + rte_dma_fill_t fill; >> + rte_dma_submit_t submit; >> + rte_dma_completed_t completed; >> + rte_dma_completed_status_t completed_status; >> + void *reserved_cl0; >> + /** Reserve space for future IO functions, while keeping data and >> + * dev_ops pointers on the second cacheline. >> + */ > This comment is out of date. > >> + void *reserved_cl1[6]; >> +} __rte_cache_aligned; > > Small suggestion: since there is no data at the end of the structure, > rather than adding in padding arrays which need to be adjusted as we add > fields into the struct, let's just change the "__rte_cache_aligned" macro > to "__rte_aligned(128)". This will explicitly set the size to 128-bytes and > allow us to remove the reserved fields - making it easier to add new > pointers. Agree > >> + >> +extern struct rte_dma_fp_object *rte_dma_fp_objs; >> + >> +#endif /* RTE_DMADEV_CORE_H */ > > . > Thanks