From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (xvm-189-124.dc0.ghst.net [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 173A4A09FF; Wed, 6 Jan 2021 15:07:40 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 970CA160982; Wed, 6 Jan 2021 15:07:39 +0100 (CET) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mails.dpdk.org (Postfix) with ESMTP id 5A8DD160980 for ; Wed, 6 Jan 2021 15:07:38 +0100 (CET) IronPort-SDR: ujtAiXwh7IjYMKuaP4NGEEzJhl0jACVE/PqBCFDUL0bkdUf5ZrCULB+1/HHFfRr4J7wQ2wsVjX 944sjjSjWzVg== X-IronPort-AV: E=McAfee;i="6000,8403,9855"; a="176500643" X-IronPort-AV: E=Sophos;i="5.78,480,1599548400"; d="scan'208";a="176500643" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Jan 2021 06:07:37 -0800 IronPort-SDR: b4/PViThryTlBeYuVY3H6k4OB6TL44lCPC3yP9FaUcfUgmWQ+nFPVl7hbprL8J/rVdcGU60NKw ikaFwzoELiJA== X-IronPort-AV: E=Sophos;i="5.78,480,1599548400"; d="scan'208";a="422175582" Received: from shanedun-mobl.ger.corp.intel.com (HELO bricha3-MOBL.ger.corp.intel.com) ([10.252.27.188]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 06 Jan 2021 06:07:35 -0800 Date: Wed, 6 Jan 2021 14:07:31 +0000 From: Bruce Richardson To: Kumar Amber Cc: dev@dpdk.org Message-ID: <20210106140731.GA1969@bricha3-MOBL.ger.corp.intel.com> References: <20210104162829.304116-1-kumar.amber@intel.com> <20210104163049.304493-1-kumar.amber@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210104163049.304493-1-kumar.amber@intel.com> Subject: Re: [dpdk-dev] [PATCH v4] raw/ioat: add secondary process 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 Mon, Jan 04, 2021 at 10:00:49PM +0530, Kumar Amber wrote: > Add support for secondary processes in ioat devices. The update > allocates a memzone for a primary process or returns it in a > secondary process. > > Signed-off-by: Kumar Amber > --- Thanks for the patch. Some comments below. Also, with each version can you include below the cutline [i.e. here] what has changed since the previous version, to make reviewing easier. Thanks, /Bruce > drivers/raw/ioat/ioat_common.c | 16 +++++++++++++--- > drivers/raw/ioat/ioat_rawdev.c | 17 +++++++++++++---- > 2 files changed, 26 insertions(+), 7 deletions(-) > > diff --git a/drivers/raw/ioat/ioat_common.c b/drivers/raw/ioat/ioat_common.c > index 142e171bc9..c174e4bb28 100644 > --- a/drivers/raw/ioat/ioat_common.c > +++ b/drivers/raw/ioat/ioat_common.c > @@ -215,11 +215,21 @@ idxd_rawdev_create(const char *name, struct rte_device *dev, > goto cleanup; > } > > + /* Allocate memory for the primary process or else return the memory > + * of primary memzone for the secondary process. > + */ > snprintf(mz_name, sizeof(mz_name), "rawdev%u_private", rawdev->dev_id); > - mz = rte_memzone_reserve(mz_name, sizeof(struct idxd_rawdev), > - dev->numa_node, RTE_MEMZONE_IOVA_CONTIG); > + if (rte_eal_process_type() == RTE_PROC_SECONDARY) { > + mz = rte_memzone_lookup(mz_name); > + rawdev->dev_private = mz->addr; I think we should have some error checking here in case the lookup fails. The particular device in question could be unused by the primary. > + return 0; > + } > + mz = rte_memzone_reserve(mz_name, > + sizeof(struct rte_ioat_rawdev), > + dev->numa_node, > + RTE_MEMZONE_IOVA_CONTIG); > if (mz == NULL) { > - IOAT_PMD_ERR("Unable to reserve memzone for private data\n"); > + IOAT_PMD_ERR("Unable to reserve memzone\n"); Is there a need to change the error message here? > ret = -ENOMEM; > goto cleanup; > } > diff --git a/drivers/raw/ioat/ioat_rawdev.c b/drivers/raw/ioat/ioat_rawdev.c > index 2c88b4369f..0d3e904c8d 100644 > --- a/drivers/raw/ioat/ioat_rawdev.c > +++ b/drivers/raw/ioat/ioat_rawdev.c > @@ -165,15 +165,24 @@ ioat_rawdev_create(const char *name, struct rte_pci_device *dev) > goto cleanup; > } > > + /* Allocate memory for the primary process or else return the memory > + * of primary memzone for the secondary process. > + */ > snprintf(mz_name, sizeof(mz_name), "rawdev%u_private", rawdev->dev_id); > - mz = rte_memzone_reserve(mz_name, sizeof(struct rte_ioat_rawdev), > - dev->device.numa_node, RTE_MEMZONE_IOVA_CONTIG); > + if (rte_eal_process_type() == RTE_PROC_SECONDARY) { > + mz = rte_memzone_lookup(mz_name); > + rawdev->dev_private = mz->addr; > + return 0; > + } > + mz = rte_memzone_reserve(mz_name, > + sizeof(struct rte_ioat_rawdev), > + dev->device.numa_node, > + RTE_MEMZONE_IOVA_CONTIG); > if (mz == NULL) { > - IOAT_PMD_ERR("Unable to reserve memzone for private data\n"); > + IOAT_PMD_ERR("Unable to reserve memzone\n"); > ret = -ENOMEM; > goto cleanup; > } > - Same comments for this file as for previous. Also note the extra line removal here, which probably should not be part of this patch too. > rawdev->dev_private = mz->addr; > rawdev->dev_ops = &ioat_rawdev_ops; > rawdev->device = &dev->device; > -- > 2.25.1 >