From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-f66.google.com (mail-ed1-f66.google.com [209.85.208.66]) by dpdk.org (Postfix) with ESMTP id 865DF58CB for ; Thu, 1 Nov 2018 15:32:19 +0100 (CET) Received: by mail-ed1-f66.google.com with SMTP id w19-v6so16717401eds.1 for ; Thu, 01 Nov 2018 07:32:19 -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=J7z0M0Lg4B7qq1d9wuXq0u26BTGAEquKGyY/EF2gdIQ=; b=JOdeHq3o9v22RFtXHvu+1OYDEBXVjSugNebndUNNpIbOIuW0Cf59rEMLjWQU7ovz0z iOT9qkrtdiYP2jpgg44Cl2gYdulQ0oBzJQ1a0lyBFlMSM/BTV6faEJw2EbxkrLiq+fFU 4m3awBZRgB9Q0t8icYogCkSqjQ8C/mPzqYiy+71AAmw4GOrbZVc12HGoy40BU+Qwt56n RcM/74CwOQdVtFBhFqFaCD8oIMir1YKEjCm13k8Ww/l0EJCh0jmBD3aEStNPa3Xld9ro XyBcCqDOOSgV3BDeUKPgzi6vla3KNW8vWz8niJCUrjw3/I11hSNmHZv8X93yQdu8ZP+i t9EQ== 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=J7z0M0Lg4B7qq1d9wuXq0u26BTGAEquKGyY/EF2gdIQ=; b=MkSqgFa/e0B9RviljigWcfeTl16z5x1KoSLcxHmvJmvyBEB6DltZv02TxqSUf2DXAB uEl77sqPbu/B0JGkBrjsDWsxkPbn10rz7Q1TuheT59KQkGmzTctpA0z5CalA4C7tqvLR e80dasRG+KQt/I4sCu0CodKKuAuFWUn/kL3f1TBJr6PLOK4OORNW5ZL0kRxlSNbVWr0U T9vtG43j1iGAgQIPa+A2t2rP0n9qdIQepbPuZ3cax9kqXeRMaWtcBNlJ0uybdZvu83rN OR/bqNG1jzmhl+dtSoayXWs2ltdFwipACvuPtEceQImKNaK6odvZTyexfmPv6WIc1jwM TtNw== X-Gm-Message-State: AGRZ1gLeYp5MX+0kEtduth5xg0+EigvGvSjr+wel+vyV8ln1/ZNxt4py 8Ub5MdR6XcYCswL9WShKvvfGKMVgsxVnZiyTd3wNaIYC X-Google-Smtp-Source: AJdET5cKiX50vxB8m/LylXQNxShj2Ck3ltYQeDMdt5QzMnNqe8tjIIha6rZVuV+sdiZ3zpm7bYQbMmqNdegoihDofwY= X-Received: by 2002:a50:86e7:: with SMTP id 36-v6mr5184869edu.104.1541082739128; Thu, 01 Nov 2018 07:32:19 -0700 (PDT) MIME-Version: 1.0 References: <20181031172931.11894-1-alejandro.lucero@netronome.com> <20181031172931.11894-4-alejandro.lucero@netronome.com> <2be201f7-308f-53e1-715a-6c37b92de10b@intel.com> <1496b1e0-f3de-2a02-a5d9-3e6c65eb93d4@intel.com> In-Reply-To: From: Alejandro Lucero Date: Thu, 1 Nov 2018 14:32:08 +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 3/7] mem: add function for setting 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 14:32:19 -0000 On Thu, Nov 1, 2018 at 11:30 AM Alejandro Lucero < alejandro.lucero@netronome.com> wrote: > > > On Thu, Nov 1, 2018 at 10:57 AM Burakov, Anatoly < > anatoly.burakov@intel.com> wrote: > >> On 01-Nov-18 10:48 AM, Alejandro Lucero wrote: >> > >> > >> > On Thu, Nov 1, 2018 at 10:11 AM Burakov, Anatoly >> > > wrote: >> > >> > On 31-Oct-18 5:29 PM, Alejandro Lucero wrote: >> > > This patch adds the possibility of setting a dma mask to be used >> > > once the memory initialization is done. >> > > >> > > This is currently needed when IOVA mode is set by PCI related >> > > code and an x86 IOMMU hardware unit is present. Current code >> calls >> > > rte_mem_check_dma_mask but it is wrong to do so at that point >> > > because the memory has not been initialized yet. >> > > >> > > Signed-off-by: Alejandro Lucero > > > >> > > --- >> > > lib/librte_eal/common/eal_common_memory.c | 10 ++++++++++ >> > > lib/librte_eal/common/include/rte_memory.h | 10 ++++++++++ >> > > lib/librte_eal/rte_eal_version.map | 1 + >> > > 3 files changed, 21 insertions(+) >> > > >> > > diff --git a/lib/librte_eal/common/eal_common_memory.c >> > b/lib/librte_eal/common/eal_common_memory.c >> > > index e0f08f39a..24b72fcb0 100644 >> > > --- a/lib/librte_eal/common/eal_common_memory.c >> > > +++ b/lib/librte_eal/common/eal_common_memory.c >> > > @@ -480,6 +480,16 @@ rte_mem_check_dma_mask(uint8_t maskbits) >> > > return 0; >> > > } >> > > >> > > +/* set dma mask to use when memory initialization is done */ >> > > +void __rte_experimental >> > > +rte_mem_set_dma_mask(uint8_t maskbits) >> > > +{ >> > > + struct rte_mem_config *mcfg = >> > rte_eal_get_configuration()->mem_config; >> > > + >> > > + mcfg->dma_maskbits = mcfg->dma_maskbits == 0 ? maskbits : >> > > + RTE_MIN(mcfg->dma_maskbits, maskbits); >> > > +} >> > > + >> > > /* return the number of memory channels */ >> > > unsigned rte_memory_get_nchannel(void) >> > > { >> > > diff --git a/lib/librte_eal/common/include/rte_memory.h >> > b/lib/librte_eal/common/include/rte_memory.h >> > > index ad3f3cfb0..eff028db1 100644 >> > > --- a/lib/librte_eal/common/include/rte_memory.h >> > > +++ b/lib/librte_eal/common/include/rte_memory.h >> > > @@ -466,6 +466,16 @@ unsigned rte_memory_get_nrank(void); >> > > /* check memsegs iovas are within a range based on dma mask */ >> > > int __rte_experimental rte_mem_check_dma_mask(uint8_t >> maskbits); >> > > >> > > +/** >> > > + * * @warning >> > > + * @b EXPERIMENTAL: this API may change without prior notice >> > >> > I think there's a copy-paste error here (extra star and indent >> before >> > warning). >> > >> > >> > I will fix this. >> > >> > Thanks. >> > >> > >> > > + * >> > > + * Set dma mask to use once memory initialization is done. >> > > + * Previous function rte_mem_check_dma_mask can not be used >> > > + * safely until memory has been initialized. >> > >> > IMO this really requires a big bold warning saying that this >> function >> > must only be called early during initialization, before memory is >> > initialized, and not to be called in user code. >> > >> > >> > what about adding a new EAL variable for keeping memory initialization >> > status? >> > It would be something like "uninit" until is changed to "done" after >> the >> > memory initialization is completed. >> > That variable would help to avoid effective calls to set the dma mask >> > after initialization. >> >> I'm not opposed to it in principle, however, this is a slippery slope >> towards having each and every subsystem storing their init status :) >> >> That said, while it's not a complete solution because it won't prevent >> spurious calls to this function _after_ memory has initialized, there is >> a variable in internal_config that you can use to deny calls to this >> function after EAL init is complete (see internal_config->init_complete). >> >> You could also store a static variable in eal_common_memory.c, and >> change it at the end of rte_eal_memory_init() call, but that does not >> really complete the memory initialization, because we then go to >> rte_eal_malloc_heap_init(), which actually completes the memory init. >> >> I would greatly prefer using internal_config->init_complete - it is >> enough to forbid user code from calling it, while driver/DPDK developers >> presumably know what they're doing enough to read the warning and not >> try to call this after memory init :) >> > > I agree that we should avoid new status variables as I proposed, and in > this case > I think using internal_config->init_complete will be enough. > > I'm wrong. And you already said it, that this will not avoid a bad usage from PMDs. I think I will follow your first advice about adding a warning about its usage. > I will add the check in the next version. > > Thanks > > >> >> > >> > > + */ >> > > +void __rte_experimental rte_mem_set_dma_mask(uint8_t maskbits); >> > > + >> > > /** >> > > * Drivers based on uio will not load unless physical >> > > * addresses are obtainable. It is only possible to get >> > > diff --git a/lib/librte_eal/rte_eal_version.map >> > b/lib/librte_eal/rte_eal_version.map >> > > index ef8126a97..ae24b5c73 100644 >> > > --- a/lib/librte_eal/rte_eal_version.map >> > > +++ b/lib/librte_eal/rte_eal_version.map >> > > @@ -296,6 +296,7 @@ EXPERIMENTAL { >> > > rte_devargs_remove; >> > > rte_devargs_type_count; >> > > rte_mem_check_dma_mask; >> > > + rte_mem_set_dma_mask; >> > >> > Same, this needs to be alphabetic. >> > >> > > rte_eal_cleanup; >> > > rte_fbarray_attach; >> > > rte_fbarray_destroy; >> > > >> > >> > >> > -- >> > Thanks, >> > Anatoly >> > >> >> >> -- >> Thanks, >> Anatoly >> >