From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 07539A04B8; Tue, 5 May 2020 15:36:06 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 747161D159; Tue, 5 May 2020 15:36:05 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 174EF1BC24 for ; Tue, 5 May 2020 15:36:03 +0200 (CEST) IronPort-SDR: LQQcxnEOfHQ2NtZPIKFEtvsDi4/EBp2dHyRFS1d6Nx1PgYxWa+oEF6b+C5+l75TXxw+7A6JEwk NyCJ9L9kBhqQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 May 2020 06:36:02 -0700 IronPort-SDR: TLb57m9ezRA7/pyyVFMJXQyvpyLakBmLFquXTeAU+SZZlvEnTJ4ZGwzhvk/9La0IOhxXUm5Y+B QQ7vyUIB/00Q== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,355,1583222400"; d="scan'208";a="304479838" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.213.197.31]) ([10.213.197.31]) by FMSMGA003.fm.intel.com with ESMTP; 05 May 2020 06:36:02 -0700 To: Renata Saiakhova , "dev@dpdk.org" References: <20200503162636.5233-1-Renata.Saiakhova@ekinops.com> <20200503162636.5233-2-Renata.Saiakhova@ekinops.com> From: "Burakov, Anatoly" Message-ID: <6c68ac6d-b669-eb94-0347-36940eb32d43@intel.com> Date: Tue, 5 May 2020 14:36:01 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 1/2] librte_ethdev: Introduce a function to release HW rings X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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 05-May-20 1:49 PM, Renata Saiakhova wrote: > Hi Anatoly, > > thanks! The fact that memzone is found and matched only based on the > name, could it create potential problem like the one I described besides > HW rings? Technically, yes, it could. However, at this point, we can't really do anything. Memzones having names is both an advantage (less code to find memory areas you're looking for) and a disadvantage (lots of ways to shoot yourself in the foot *if* you have a habit of naming different memzones with identical names... which our drivers apparently do!). IMO, trying to fix it will lead us down the rabbit hole of, 1) check if memzone exists, 2) check if it matches the requirements, and 3) handle all possible conditions related to 1) and 2) (do the requirements match? do they have to match *exactly*, or "this or bigger" will do? do we throw an error if we detect mismatch? do we deallocate old memzone?). So from our perspective, it is better to leave it up to the user, and just ensure that our drivers do mostly sane things. > > Kind regards, > Renata > ------------------------------------------------------------------------ > *From:* Burakov, Anatoly > *Sent:* Tuesday, May 5, 2020 12:45 PM > *To:* Renata Saiakhova ; dev@dpdk.org > > *Subject:* Re: [dpdk-dev] [PATCH 1/2] librte_ethdev: Introduce a > function to release HW rings > On 05-May-20 11:25 AM, Burakov, Anatoly wrote: >> On 03-May-20 5:26 PM, Renata Saiakhova wrote: >>> Free previously allocated memzone for HW rings >>> >>> Signed-off-by: Renata Saiakhova >>> --- >>>   lib/librte_ethdev/rte_ethdev.c           | 23 +++++++++++++++++++++++ >>>   lib/librte_ethdev/rte_ethdev_driver.h    | 14 ++++++++++++++ >>>   lib/librte_ethdev/rte_ethdev_version.map |  1 + >>>   3 files changed, 38 insertions(+) >>> >>> diff --git a/lib/librte_ethdev/rte_ethdev.c >>> b/lib/librte_ethdev/rte_ethdev.c >>> index 72aed59a5..c6d27e1aa 100644 >>> --- a/lib/librte_ethdev/rte_ethdev.c >>> +++ b/lib/librte_ethdev/rte_ethdev.c >>> @@ -4206,6 +4206,29 @@ rte_eth_dma_zone_reserve(const struct >>> rte_eth_dev *dev, const char *ring_name, >>>               RTE_MEMZONE_IOVA_CONTIG, align); >>>   } >>> +int >>> +rte_eth_dma_zone_free(const struct rte_eth_dev *dev, const char >>> *ring_name, >>> +        uint16_t queue_id) >>> +{ >>> +    char z_name[RTE_MEMZONE_NAMESIZE]; >>> +    const struct rte_memzone *mz; >>> +    int rc = 0; >>> + >>> +    snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s", >>> +            dev->data->port_id, queue_id, ring_name); >>> +    if (rc >= RTE_MEMZONE_NAMESIZE) { >>> +        RTE_ETHDEV_LOG(ERR, "ring name too long\n"); >>> +        rte_errno = ENAMETOOLONG; >>> +        return NULL; >>> +    } >>> + >>> +    mz = rte_memzone_lookup(z_name); >>> +    if (mz) >>> +        rc = rte_memzone_free(mz); >> >> This is racy. Please just use rte_memzone_free() unconditionally. It'll >> return 0 if memzone existed, or will set rte_errno to EINVAL if it >> didn't. (this is suboptimal, it should be ENOENT, but changing this >> would be an API break... I'll submit a patch for future release to fix >> this) >> > > My apologies, just using rte_memzone_free will not solve the problem > because you don't have memzone pointer. Now that i think of it, the > rte_eth_dma_zone_reserve() suffers from this issue too, and the problem > is lack of atomic "find or create" memzone API. > > This patch is OK for now, as it follows similar code in > rte_eth_dma_zone_reserve(), but ideally, this should be fixed at the > memzone API level. I'll see if i can cobble together a quick patchset > adding atomic "find or reserve" and "find and free" operations. > > -- > Thanks, > Anatoly -- Thanks, Anatoly