From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) by dpdk.org (Postfix) with ESMTP id 410AE1B133 for ; Thu, 4 Oct 2018 15:00:07 +0200 (CEST) Received: by mail-ed1-f51.google.com with SMTP id c1-v6so8456328ede.5 for ; Thu, 04 Oct 2018 06:00:07 -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; bh=AT649HJ2qYxdUupmUs8Nx4k1MV4e2MLwxULFReLGwlQ=; b=TMgYto+dQDyhDIoSrfhNc0z0pFb42XsLJQRMbnNzIW2DaogX/91Ika0l3KJAU26OaG BiXHoeBxA2oI2tvSPx3yj8wyexe4nQ4jexS3jmq2NgBvd1P8xhZ3pBBh0O8IRicUlci1 LF9lQgMDfWTKzDorl7AvJ+dkfV+s8puwcrdrWtnD2dfsGgPzHQH/XSHzuAeACH0xQraC kMBr73RomShuDCak+Be1HIzcWGSenFZU5wIuPOvMQavrkccTNqF83s5BCxYfnKI04/58 5S/SW1INiY7VWdEFUmkkSa2Sn4gepnYtIqmQ0TSEzHyRH1wIcoJA6HPNdQOBVegt6yzg kigg== 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; bh=AT649HJ2qYxdUupmUs8Nx4k1MV4e2MLwxULFReLGwlQ=; b=KZZgECQAt2juG39HxDv4aoHhmGW4n7mICa8tXhlM3sBkWyBDx9gHRo9R5M8d8feI/E pFGMYzrvF1nBBEhp/pXaNbpNeplZsLn9RJJICWc20c9XDlzHAzm8Srd5N5TMO47qdZq8 duKGRhimWJC626cKpzFIEuHvWqfax4V2i+xyTxr9AD0HyrlNraw5Mz5D7zCAv1tuHX/V zlLnQdXqltoqPm6ekkph3DMwXG3zxC78feptSWHZciotjMXRuUbz29Y2Q0eMCm95RILD xA8bsj8gD4HwYS0HIWG/e1ELPIaiTfcc57zPdL/MkSDNqCVTAWA8VPFvo2vWFWEWgL0L lWGQ== X-Gm-Message-State: ABuFfoh40WWyKq10Xz/nGCeZE6Rh/QaRAWWAMODtnV4xQ+hqkAmjaC+C 27Z7cd3+Z5DZge/f8gVJ6nKTitAq/IQCDE05JA26+N/X X-Google-Smtp-Source: ACcGV62uqq2vSB3Jhiqh2wmR2LB57DlTxj/k8LXorlrxgO5qShT04OKuybZ2+jPegBcn4mbMOswvU2wCj1ZlZyvZROc= X-Received: by 2002:a17:906:6b42:: with SMTP id o2-v6mr6348119ejs.186.1538658006345; Thu, 04 Oct 2018 06:00:06 -0700 (PDT) MIME-Version: 1.0 References: <1535719857-19092-1-git-send-email-alejandro.lucero@netronome.com> <1535719857-19092-2-git-send-email-alejandro.lucero@netronome.com> <1ec4866d-24d4-1881-dfbe-ca2ff878e8c9@intel.com> In-Reply-To: From: Alejandro Lucero Date: Thu, 4 Oct 2018 13:59:54 +0100 Message-ID: To: dev Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: [dpdk-dev] Fwd: [PATCH v2 1/5] mem: add function for checking memsegs IOVAs addresses 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, 04 Oct 2018 13:00:07 -0000 I sent this email only to Anatoly. Sending it again to mailing list. On Wed, Oct 3, 2018 at 1:43 PM Burakov, Anatoly wrote: > On 31-Aug-18 1:50 PM, Alejandro Lucero wrote: > > A device can suffer addressing limitations. This functions checks > > memsegs have iovas within the supported range based on dma mask. > > > > PMD should use this during initialization if supported devices > > suffer addressing limitations, returning an error if this function > > returns memsegs out of range. > > > > Another potential usage is for emulated IOMMU hardware with addressing > > limitations. > > > > It is necessary to save the most restricted dma mask for checking > > memory allocated dynamically after initialization. > > > > Signed-off-by: Alejandro Lucero > > --- > > lib/librte_eal/common/eal_common_memory.c | 56 > +++++++++++++++++++++++ > > lib/librte_eal/common/include/rte_eal_memconfig.h | 3 ++ > > lib/librte_eal/common/include/rte_memory.h | 3 ++ > > lib/librte_eal/common/malloc_heap.c | 12 +++++ > > lib/librte_eal/linuxapp/eal/eal.c | 2 + > > lib/librte_eal/rte_eal_version.map | 1 + > > 6 files changed, 77 insertions(+) > > > > diff --git a/lib/librte_eal/common/eal_common_memory.c > b/lib/librte_eal/common/eal_common_memory.c > > index fbfb1b0..bdd8f44 100644 > > --- a/lib/librte_eal/common/eal_common_memory.c > > +++ b/lib/librte_eal/common/eal_common_memory.c > > @@ -383,6 +383,62 @@ struct virtiova { > > rte_memseg_walk(dump_memseg, f); > > } > > > > +static int > > +check_iova(const struct rte_memseg_list *msl __rte_unused, > > + const struct rte_memseg *ms, void *arg) > > +{ > > + uint64_t *mask = arg; > > + rte_iova_t iova; > > + > > + /* higher address within segment */ > > + iova = (ms->iova + ms->len) - 1; > > + if (!(iova & *mask)) > > + return 0; > > + > > + RTE_LOG(INFO, EAL, "memseg iova %"PRIx64", len %zx, out of > range\n", > > + ms->iova, ms->len); > > + > > + RTE_LOG(INFO, EAL, "\tusing dma mask %"PRIx64"\n", *mask); > > IMO putting these as INFO is overkill. I'd prefer not to spam the output > unless it's really important. Can this go under DEBUG? > > This checks comes from a device or from the alloc_pages_on_heap when expanding memory. If the check discovers an address out of mask, a device can not be used or the new memory can not be allocated. I think having this info will help to understand why the device initialization or the memory allocation are failing. > Also, the message is misleading. You stop before you have a chance to > check other masks, which may restrict them even further. You're > outputting the message about using DMA mask XXX but this may not be the > final DMA mask. > Well, this is the first triggering, and it is enough for reporting the problem and avoiding the device or the new memory to be used. Note that the mask is per device, and for the memory allocation case, it is the most restrictive dma mask. So there are no other masks to try. > > > + /* Stop the walk and change mask */ > > + *mask = 0; > > + return 1; > > +} > > + > > +#if defined(RTE_ARCH_64) > > +#define MAX_DMA_MASK_BITS 63 > > +#else > > +#define MAX_DMA_MASK_BITS 31 > > +#endif > > + > > +/* check memseg iovas are within the required range based on dma mask */ > > +int __rte_experimental > > +rte_eal_check_dma_mask(uint8_t maskbits) > > +{ > > + struct rte_mem_config *mcfg = > rte_eal_get_configuration()->mem_config; > > + uint64_t mask; > > + > > + /* sanity check */ > > + if (maskbits > MAX_DMA_MASK_BITS) { > > + RTE_LOG(INFO, EAL, "wrong dma mask size %u (Max: %u)\n", > > + maskbits, MAX_DMA_MASK_BITS); > > Should be ERR, not INFO. > > Right. I will change it. > > + return -1; > > + } > > + > > + /* keep the more restricted maskbit */ > > + if (!mcfg->dma_maskbits || maskbits < mcfg->dma_maskbits) > > + mcfg->dma_maskbits = maskbits; > > Do we need to modify mcfg->dma_maskbits before we know if we're going to > fail? Suggest using a local variable maybe? > > Yes, that's true. If the check fails, the device will not be used therefore we do not need to keep that dma mask at all. I will change the order here. Thanks! > Also, i think it's a good case for ternary: > > bits = mcfg->dma_maskbits == 0 ? > maskbits : > RTE_MIN(maskbits, mcfg->dma_maskbits); > > IMO the intention looks much clearer. > > Agree. > > + > > + /* create dma mask */ > > + mask = ~((1ULL << maskbits) - 1); > > + > > + rte_memseg_walk(check_iova, &mask); > > + > > + if (!mask) > > + return -1; > > + > > + return 0; > > +} > > + > > /* return the number of memory channels */ > > unsigned rte_memory_get_nchannel(void) > > { > > diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h > b/lib/librte_eal/common/include/rte_eal_memconfig.h > > index aff0688..aea44cb 100644 > > --- a/lib/librte_eal/common/include/rte_eal_memconfig.h > > +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h > > @@ -77,6 +77,9 @@ struct rte_mem_config { > > * exact same address the primary process maps it. > > */ > > uint64_t mem_cfg_addr; > > + > > + /* keeps the more restricted dma mask */ > > + uint8_t dma_maskbits; > > This needs to be documented as an ABI break in the 18.11 release notes. > > Ok. I'll add that in the next version. Thanks > > -- > Thanks, > Anatoly >