From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-f67.google.com (mail-ed1-f67.google.com [209.85.208.67]) by dpdk.org (Postfix) with ESMTP id CB52458C6 for ; Thu, 1 Nov 2018 14:34:13 +0100 (CET) Received: by mail-ed1-f67.google.com with SMTP id c1-v6so16567296ede.5 for ; Thu, 01 Nov 2018 06:34:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netronome-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=WigbIds0t6bx/wS2NDAj0gPt4zCnhW35KyXq2W2tlog=; b=C/Q+R0L5D0zBWCTzURkDj3BjrVvG8i+lyn/CFFx7DB9LcESwzkJa9luF2c4vSYp5MO OIuFZNZLkPS4boBhWqrvZFlH4eJLl4p8CPREmhKbQV2ivi2jwPC4sbZo2GHmoEt5jE1L Zi5gA+OyCezNw1NCH1aPxppHhBxLPjYMDQE33FEzI8JwIf8MJwivm0n+4UZm/U3EhZ0b PXLlUlWVs/e0SddU/EfFiYCoLLGKFrwgm+5lLytIPgcxoKflhaIn3yvLLlN6C25ajKug VEifi2ZCXmBDWOdjxf1UtcBodNh/1fjZQBeg7v41200U4zonr81PL2IE2oOkZ0OZwMeo sIBg== 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; bh=WigbIds0t6bx/wS2NDAj0gPt4zCnhW35KyXq2W2tlog=; b=Tp3MHTo/V+vHk2kTorNrwT8QXvSpU5FoU/c81Wu1uMGWTROSezibgaK0P5yvLv3xCv m7Qcuh46YPp0/QE1usul6AgadN+n6m4ZmAqvEvf6MiJXN4z+iYVFp8RjW1tIO0Yxi1zU VXXYGF01ooLWDm0kt0sr+Zc0Qb00ru7u+6OzceKdBnOWh98PBnLBpydayAJVDxdMwaPg SC4XC0UiK0EV/7CgCQ2T9L7FREFW+iVTAvACE1B+qurp1TmNIZHPf3uulW/F2R3+b+x4 caMr9nytFppmpOfmEm0RNoaKi7alEDvz4RNh0NL3Ft8M+gzCX76RKUmt6mOFTYdxd/uy Lekw== X-Gm-Message-State: AGRZ1gKRyuBYXXM1dCN3GW2f7LdnKSmJbh3IqwCveG3c2wnEKrI7bG7z TEpDQiDDR3GejQuhifvCLb+V+5vi7YPPNDfY/zLsHwP4 X-Google-Smtp-Source: AJdET5exZ8ke5aSHXgAH2v5mrisk/hK3IXEgondPhhsiOoRNlQnsGh09noIn5K3Dxtgywf4SUe7ek3305RuBx//ky8o= X-Received: by 2002:a50:9386:: with SMTP id o6-v6mr5371797eda.248.1541079253396; Thu, 01 Nov 2018 06:34:13 -0700 (PDT) MIME-Version: 1.0 References: <20181031172931.11894-1-alejandro.lucero@netronome.com> <20181031172931.11894-7-alejandro.lucero@netronome.com> In-Reply-To: From: Alejandro Lucero Date: Thu, 1 Nov 2018 13:34:02 +0000 Message-ID: To: "Burakov, Anatoly" Cc: dev Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH 6/7] mem: add safe and unsafe versions for checking DMA mask 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: , X-List-Received-Date: Thu, 01 Nov 2018 13:34:14 -0000 On Thu, Nov 1, 2018 at 10:38 AM Burakov, Anatoly wrote: > On 31-Oct-18 5:29 PM, Alejandro Lucero wrote: > > During memory initialization calling rte_mem_check_dma_mask > > leads to a deadlock because memory_hotplug_lock is locked by a > > writer, the current code in execution, and rte_memseg_walk > > tries to lock as a reader. > > > > This patch adds safe and unsafe versions for invoking the final > > function specifying if the memory_hotplug_lock needs to be > > acquired, this is for the safe version, or not, the unsafe one. > > PMDs should use the safe version and just internal EAL memory > > code should use the unsafe one. > > > > Fixes: 223b7f1d5ef6 ("mem: add function for checking memseg IOVA") > > > > Signed-off-by: Alejandro Lucero > > --- > > I don't think _safe and _unsafe are good names. _unsafe implies > something might blow up, which isn't the case :) I think following the > naming convention established by other functions (rte_mem_check_dma_mask > and rte_mem_check_dma_mask_thread_unsafe) is better. User/driver code is > only supposed to use rte_mem_check_dma_mask safe version anyway, so > there's no need to differentiate between the two if the other one is > never supposed to be used. > > It makes sense. I will do it in next version. Thanks > > drivers/net/nfp/nfp_net.c | 2 +- > > lib/librte_eal/common/eal_common_memory.c | 24 +++++++++++++++--- > > lib/librte_eal/common/include/rte_memory.h | 29 +++++++++++++++++++--- > > lib/librte_eal/common/malloc_heap.c | 2 +- > > lib/librte_eal/rte_eal_version.map | 3 ++- > > 5 files changed, 51 insertions(+), 9 deletions(-) > > > > <...> > > > -/* check memsegs iovas are within a range based on dma mask */ > > -int __rte_experimental rte_mem_check_dma_mask(uint8_t maskbits); > > +/** > > + * * @warning > > Here and in other places - same issue with extra star. > > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Check memsegs iovas are within a range based on dma mask. > > The comments make it seem like the parameter is an actual DMA mask, > rather than DMA mask *width*. In fact, you seem to have tripped yourself > up on that already :) > > Suggested rewording: > > Check if all currently allocated memory segments are compliant with > supplied DMA address width. > > Ok. > > + * > > + * @param maskbits > > + * Address width to check against. > > + */ > > +int __rte_experimental rte_mem_check_dma_mask_safe(uint8_t maskbits); > > + > > +/** > > + * * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Check memsegs iovas are within a range based on dma mask without > acquiring > > + * memory_hotplug_lock first. > > + * > > + * This function is just for EAL core memory internal use. Drivers > should > > + * use the previous safe one. > > This is IMO too detailed. Suggested rewording: > > Check if all currently allocated memory segments are compliant with > supplied DMA address width. > > Ok > @warning This function is not thread-safe and is for internal use only. > > > + * > > + * @param maskbits > > + * Address width to check against. > > + */ > > +int __rte_experimental rte_mem_check_dma_mask_unsafe(uint8_t maskbits); > > > > /** > > * * @warning > > * @b EXPERIMENTAL: this API may change without prior notice > > * > > * Set dma mask to use once memory initialization is done. > > - * Previous function rte_mem_check_dma_mask can not be used > > + * Previous functions rte_mem_check_dma_mask_safe/unsafe can not be > used > > * safely until memory has been initialized. > > */ > > void __rte_experimental rte_mem_set_dma_mask(uint8_t maskbits); > > diff --git a/lib/librte_eal/common/malloc_heap.c > b/lib/librte_eal/common/malloc_heap.c > > index 711622f19..dd8b983e7 100644 > > --- a/lib/librte_eal/common/malloc_heap.c > > +++ b/lib/librte_eal/common/malloc_heap.c > > @@ -335,7 +335,7 @@ alloc_pages_on_heap(struct malloc_heap *heap, > uint64_t pg_sz, size_t elt_size, > > * executed. For 2) implies the new memory can not be added. > > */ > > if (mcfg->dma_maskbits) { > > - if (rte_mem_check_dma_mask(mcfg->dma_maskbits)) { > > + if (rte_mem_check_dma_mask_unsafe(mcfg->dma_maskbits)) { > > /* Currently this can only happen if IOMMU is > enabled > > * with RTE_ARCH_X86. It is not safe to use this > memory > > * so returning an error here. > > diff --git a/lib/librte_eal/rte_eal_version.map > b/lib/librte_eal/rte_eal_version.map > > index ae24b5c73..f863903b6 100644 > > --- a/lib/librte_eal/rte_eal_version.map > > +++ b/lib/librte_eal/rte_eal_version.map > > @@ -296,7 +296,8 @@ EXPERIMENTAL { > > rte_devargs_remove; > > rte_devargs_type_count; > > rte_mem_check_dma_mask; > > - rte_mem_set_dma_mask; > > + rte_mem_set_dma_mask_safe; > > + rte_mem_set_dma_mask_unsafe; > > Again, alphabet :) > > > rte_eal_cleanup; > > rte_fbarray_attach; > > rte_fbarray_destroy; > > > > > -- > Thanks, > Anatoly >