From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alejandro.lucero@netronome.com>
Received: from mail-ed1-f67.google.com (mail-ed1-f67.google.com
 [209.85.208.67]) by dpdk.org (Postfix) with ESMTP id 2F5AB7EC7
 for <dev@dpdk.org>; Thu,  1 Nov 2018 12:30:40 +0100 (CET)
Received: by mail-ed1-f67.google.com with SMTP id z12-v6so10496945edp.7
 for <dev@dpdk.org>; Thu, 01 Nov 2018 04:30:40 -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=C4kShOaOHO3IazN1GBcj318oV3CYubMbIxVmpxUhcUo=;
 b=dOU8vLlf3u0dOGsw4ctXE10P54hOfi4OLnpS3+bWmeH+t6ym3nosAjAFXH0yYU5EKn
 pExajKgptw3SZcjTSTcs+HuXrXJz/24xBMi6YGwCEtnXaSOMQvaYm5PJnvtC7lQ4SXfD
 fRLX2TCux2XV52R8xvWkKRFS0G2+i/AZx1WR+4K1upFxmsKGG17hhBhCELhEdhVKrJP/
 nWWYMhx0HwCCKnz0UhncWjbqRk6rMchOhBKAUcK6FlGeYmvCS7T6P7UI4numLqzIN2PO
 b0vGpuYf66hajJdzGx82uAsTGuml8F2em0xO2f2P/MMNyvNxMa4WjjBw2dlZrnLAX6L5
 sZpA==
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=C4kShOaOHO3IazN1GBcj318oV3CYubMbIxVmpxUhcUo=;
 b=licc2n8cG/LkJ+/nsgArXnOUmemMtcbm8+ZdnZk/wWRQ6ShPEpNNY2ByosJgrl/+/Y
 jrlcmzKbvnIBupGG+ZRzX0XZbfGJJFjlkQaMTjaPGDjvxwD0flkXNPZ99IbK8TzOvJET
 WzJsXjLn+/TeGSpEVJ1eoZihtE2jUe6ZGv+joZU01T61arU8TLc2kNpffXY76ghCNV2H
 MsQ0GQqiCxhaqJR3gJM5Zc6r5i01hrai8a7xO0xN+NFcbxt5FLOBu6UMgl4WlIjcNlXv
 +Ln5GHyjJWq/OyRBkSlkxBN3NHgEWH6qcxrLQUxDRs38Ad3PpfdUSP2NFjFote74+XOa
 5/LA==
X-Gm-Message-State: AGRZ1gK9SHYwH6VLHonxK6TLCuGHDnFtIIhTX+lO2FabVnATTOnFTV/g
 kmJLiJ1nkfvMmq3Jw6NzqKZLI8j6qUcT+BcLb5iNeA==
X-Google-Smtp-Source: AJdET5duAnlb7BbaHle9Shw46nIVLVVf5SHIg8scBcO/6I9W8tc0fG6euC11fygV7nV87CqqYbMXiBb1COvE/oxaOvM=
X-Received: by 2002:a17:906:37da:: with SMTP id
 o26-v6mr3783234ejc.179.1541071839727; 
 Thu, 01 Nov 2018 04:30:39 -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>
 <CAD+H9934bLNk-E29NepX63RSWgmNxmuizOYPvEJoQFWARrBQPg@mail.gmail.com>
 <1496b1e0-f3de-2a02-a5d9-3e6c65eb93d4@intel.com>
In-Reply-To: <1496b1e0-f3de-2a02-a5d9-3e6c65eb93d4@intel.com>
From: Alejandro Lucero <alejandro.lucero@netronome.com>
Date: Thu, 1 Nov 2018 11:30:28 +0000
Message-ID: <CAD+H993iQqriLe=JBpa7=541tj7r7KkhD4D9fwA6QH7iOegb+A@mail.gmail.com>
To: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Cc: dev <dev@dpdk.org>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Thu, 01 Nov 2018 11:30:40 -0000

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
> > <anatoly.burakov@intel.com <mailto:anatoly.burakov@intel.com>> 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 <alejandro.lucero@netronome.com
> >     <mailto:alejandro.lucero@netronome.com>>
> >      > ---
> >      >   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 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
>