| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <850443180c3c48c8bcb146e114556870@hisilicon.com>
Date: Fri, 21 Aug 2020 09:44:08 +0000
From: "Song Bao Hua (Barry Song)" <song.bao.hua@...ilicon.com>
To: Will Deacon <will@...nel.org>
CC: "hch@....de" <hch@....de>,
"m.szyprowski@...sung.com" <m.szyprowski@...sung.com>,
"robin.murphy@....com" <robin.murphy@....com>,
"ganapatrao.kulkarni@...ium.com" <ganapatrao.kulkarni@...ium.com>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
Linuxarm <linuxarm@...wei.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
huangdaode <huangdaode@...wei.com>,
"Jonathan Cameron" <jonathan.cameron@...wei.com>,
Nicolas Saenz Julienne <nsaenzjulienne@...e.de>,
Steve Capper <steve.capper@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
Mike Rapoport <rppt@...ux.ibm.com>
Subject: RE: [PATCH v6 1/2] dma-contiguous: provide the ability to reserve
per-numa CMA
> -----Original Message-----
> From: Will Deacon [mailto:will@...nel.org]
> Sent: Friday, August 21, 2020 9:27 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@...ilicon.com>
> Cc: hch@....de; m.szyprowski@...sung.com; robin.murphy@....com;
> ganapatrao.kulkarni@...ium.com; catalin.marinas@....com;
> iommu@...ts.linux-foundation.org; Linuxarm <linuxarm@...wei.com>;
> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org;
> huangdaode <huangdaode@...wei.com>; Jonathan Cameron
> <jonathan.cameron@...wei.com>; Nicolas Saenz Julienne
> <nsaenzjulienne@...e.de>; Steve Capper <steve.capper@....com>; Andrew
> Morton <akpm@...ux-foundation.org>; Mike Rapoport <rppt@...ux.ibm.com>
> Subject: Re: [PATCH v6 1/2] dma-contiguous: provide the ability to reserve
> per-numa CMA
>
> On Fri, Aug 21, 2020 at 09:13:39AM +0000, Song Bao Hua (Barry Song) wrote:
> >
> >
> > > -----Original Message-----
> > > From: Will Deacon [mailto:will@...nel.org]
> > > Sent: Friday, August 21, 2020 8:47 PM
> > > To: Song Bao Hua (Barry Song) <song.bao.hua@...ilicon.com>
> > > Cc: hch@....de; m.szyprowski@...sung.com; robin.murphy@....com;
> > > ganapatrao.kulkarni@...ium.com; catalin.marinas@....com;
> > > iommu@...ts.linux-foundation.org; Linuxarm <linuxarm@...wei.com>;
> > > linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org;
> > > huangdaode <huangdaode@...wei.com>; Jonathan Cameron
> > > <jonathan.cameron@...wei.com>; Nicolas Saenz Julienne
> > > <nsaenzjulienne@...e.de>; Steve Capper <steve.capper@....com>;
> > > Andrew Morton <akpm@...ux-foundation.org>; Mike Rapoport
> > > <rppt@...ux.ibm.com>
> > > Subject: Re: [PATCH v6 1/2] dma-contiguous: provide the ability to
> > > reserve per-numa CMA
> > >
> > > On Fri, Aug 21, 2020 at 02:26:14PM +1200, Barry Song wrote:
> > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt
> > > b/Documentation/admin-guide/kernel-parameters.txt
> > > > index bdc1f33fd3d1..3f33b89aeab5 100644
> > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > @@ -599,6 +599,15 @@
> > > > altogether. For more information, see
> > > > include/linux/dma-contiguous.h
> > > >
> > > > + pernuma_cma=nn[MG]
> > > > + [ARM64,KNL]
> > > > + Sets the size of kernel per-numa memory area for
> > > > + contiguous memory allocations. A value of 0 disables
> > > > + per-numa CMA altogether. DMA users on node nid will
> > > > + first try to allocate buffer from the pernuma area
> > > > + which is located in node nid, if the allocation fails,
> > > > + they will fallback to the global default memory area.
> > >
> > > What is the default behaviour if this option is not specified? Seems
> > > like that should be mentioned here.
>
> Just wanted to make sure you didn't miss this ^^
If it is not specified, the default size is 0 that means pernuma_cma is disabled.
Will put some words for this.
>
> > >
> > > > diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig index
> > > > 847a9d1fa634..db7a37ed35eb 100644
> > > > --- a/kernel/dma/Kconfig
> > > > +++ b/kernel/dma/Kconfig
> > > > @@ -118,6 +118,16 @@ config DMA_CMA
> > > > If unsure, say "n".
> > > >
> > > > if DMA_CMA
> > > > +
> > > > +config DMA_PERNUMA_CMA
> > > > + bool "Enable separate DMA Contiguous Memory Area for each
> NUMA
> > > Node"
> > >
> > > I don't understand the need for this config option. If you have
> > > DMA_DMA and you have NUMA, why wouldn't you want this enabled?
> >
> > Christoph preferred this in previous patchset in order to be able to
> > remove all of the code in the text if users don't use pernuma CMA.
>
> Ok, I defer to Christoph here, but maybe a "default NUMA" might work?
maybe "default NUMA && ARM64"?
Though I believe it will benefit x86, but I don't have a x86 server hardware
and real scenario to test. So I haven't put the dma_pernuma_cma_reserve()
code in arch/x86.
Hopefully some x86 guys will bring it up and remove the "&& ARM64".
>
> > > > + help
> > > > + Enable this option to get pernuma CMA areas so that devices like
> > > > + ARM64 SMMU can get local memory by DMA coherent APIs.
> > > > +
> > > > + You can set the size of pernuma CMA by specifying
> > > "pernuma_cma=size"
> > > > + on the kernel's command line.
> > > > +
> > > > comment "Default contiguous memory area size:"
> > > >
> > > > config CMA_SIZE_MBYTES
> > > > diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> > > > index cff7e60968b9..89b95f10e56d 100644
> > > > --- a/kernel/dma/contiguous.c
> > > > +++ b/kernel/dma/contiguous.c
> > > > @@ -69,6 +69,19 @@ static int __init early_cma(char *p) }
> > > > early_param("cma", early_cma);
> > > >
> > > > +#ifdef CONFIG_DMA_PERNUMA_CMA
> > > > +
> > > > +static struct cma *dma_contiguous_pernuma_area[MAX_NUMNODES];
> > > > +static phys_addr_t pernuma_size_bytes __initdata;
> > > > +
> > > > +static int __init early_pernuma_cma(char *p) {
> > > > + pernuma_size_bytes = memparse(p, &p);
> > > > + return 0;
> > > > +}
> > > > +early_param("pernuma_cma", early_pernuma_cma); #endif
> > > > +
> > > > #ifdef CONFIG_CMA_SIZE_PERCENTAGE
> > > >
> > > > static phys_addr_t __init __maybe_unused
> > > cma_early_percent_memory(void)
> > > > @@ -96,6 +109,34 @@ static inline __maybe_unused phys_addr_t
> > > cma_early_percent_memory(void)
> > > >
> > > > #endif
> > > >
> > > > +#ifdef CONFIG_DMA_PERNUMA_CMA
> > > > +void __init dma_pernuma_cma_reserve(void) {
> > > > + int nid;
> > > > +
> > > > + if (!pernuma_size_bytes)
> > > > + return;
> > >
> > > If this is useful (I assume it is), then I think we should have a
> > > non-zero default value, a bit like normal CMA does via CMA_SIZE_MBYTES.
> >
> > The patchet used to have a CONFIG_PERNUMA_CMA_SIZE in
> > kernel/dma/Kconfig, but Christoph was not comfortable with it:
> > https://lore.kernel.org/linux-iommu/20200728115231.GA793@lst.de/
> >
> > Would you mind to hardcode the value in CONFIG_CMDLINE in
> arch/arm64/Kconfig as Christoph mentioned:
> > config CMDLINE
> > default "pernuma_cma=16M"
> >
> > If you also don't like the change in arch/arm64/Kconfig CMDLINE, I
> > guess I have to depend on users' setting in cmdline just like hugetlb_cma.
>
> Again, I defere to CHristophe for this code, so leave it like it is.
> However, the same argument applies to CMA_SIZE_MBYTES afaict, and I'm
> mainly looking for consistency.
>
> > > > + for_each_node_state(nid, N_ONLINE) {
> > >
> > > for_each_online_node() {
> > >
> > > > + int ret;
> > > > + char name[20];
> > >
> > > 20?
> > >
> > > Ah, wait, this is copy-pasta from hugetlb_cma_reserve(). Can you
> > > factor out the common parts at all?
> >
> > Actually I have a "#define CMA_MAX_NAME 64" in this commit:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/com
> > mit/?id=18e98e56f440
> >
> > the 20 in hugetlb_cma_reserve() was also made by me. If you are not
> > comfortable, I can move to CMA_MAX_NAME. do you think it does really
> > matter here? 20 seems to be long enough for this scenario.
>
> Using CMA_MAX_NAME seems sensible to me, although I'm still a bit wary
> about the code duplication between this and the hugetlb code.
If the name has no index, we don't have to maintain a local name array, so they
can simply put a const string.
Here for hugetlb_cma and pernuma_cma, it happens they both have to use
sprintf() to get a local name with index. But this kind of scenarios would be rare.
> Will
Thanks
Barry
Powered by blists - more mailing lists