[PATCH 1/2] percpu: km: remove SMP check

[PATCH 1/2] percpu: km: remove SMP check

[Peng Fan]

percpu-km could only be selected by NEED_PER_CPU_KM which
depends on !SMP, so CONFIG_SMP will be false when choose percpu-km.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 mm/percpu-km.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/percpu-km.c b/mm/percpu-km.c
index 0f643dc2dc65..66e5598be876 100644
--- a/mm/percpu-km.c
+++ b/mm/percpu-km.c
@@ -27,7 +27,7 @@
  *   chunk size is not aligned.  percpu-km code will whine about it.
  */
 
-#if defined(CONFIG_SMP) && defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
+#if defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
 #error "contiguous percpu allocation is incompatible with paged first chunk"
 #endif
 
-- 
2.16.4

[PATCH 2/2] percpu: km: no need to consider pcpu_group_offsets[0]

[Peng Fan]

percpu-km is used on UP systems which only has one group,
so the group offset will be always 0, there is no need
to subtract pcpu_group_offsets[0] when assigning chunk->base_addr

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 mm/percpu-km.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/percpu-km.c b/mm/percpu-km.c
index 66e5598be876..8872c21a487b 100644
--- a/mm/percpu-km.c
+++ b/mm/percpu-km.c
@@ -67,7 +67,7 @@ static struct pcpu_chunk *pcpu_create_chunk(gfp_t gfp)
 		pcpu_set_page_chunk(nth_page(pages, i), chunk);
 
 	chunk->data = pages;
-	chunk->base_addr = page_address(pages) - pcpu_group_offsets[0];
+	chunk->base_addr = page_address(pages);
 
 	spin_lock_irqsave(&pcpu_lock, flags);
 	pcpu_chunk_populated(chunk, 0, nr_pages, false);
-- 
2.16.4

Re: [PATCH 2/2] percpu: km: no need to consider pcpu_group_offsets[0]

[dennis]

On Sun, Feb 24, 2019 at 01:13:50PM +0000, Peng Fan wrote:
> percpu-km is used on UP systems which only has one group,
> so the group offset will be always 0, there is no need
> to subtract pcpu_group_offsets[0] when assigning chunk->base_addr
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  mm/percpu-km.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/percpu-km.c b/mm/percpu-km.c
> index 66e5598be876..8872c21a487b 100644
> --- a/mm/percpu-km.c
> +++ b/mm/percpu-km.c
> @@ -67,7 +67,7 @@ static struct pcpu_chunk *pcpu_create_chunk(gfp_t gfp)
>  		pcpu_set_page_chunk(nth_page(pages, i), chunk);
>  
>  	chunk->data = pages;
> -	chunk->base_addr = page_address(pages) - pcpu_group_offsets[0];
> +	chunk->base_addr = page_address(pages);
>  
>  	spin_lock_irqsave(&pcpu_lock, flags);
>  	pcpu_chunk_populated(chunk, 0, nr_pages, false);
> -- 
> 2.16.4
> 

While I do think you're right, creating a chunk is not a part of the
critical path and subtracting 0 is incredibly minor overhead. So I'd
rather keep the code as is to maintain consistency between percpu-vm.c
and percpu-km.c.

Thanks,
Dennis

RE: [PATCH 2/2] percpu: km: no need to consider pcpu_group_offsets[0]

[Peng Fan]

Hi Dennis,

> -----Original Message-----
> From: dennis@kernel.org [mailto:dennis@kernel.org]
> Sent: 2019年2月25日 23:16
> To: Peng Fan <peng.fan@nxp.com>
> Cc: tj@kernel.org; cl@linux.com; linux-mm@kvack.org;
> linux-kernel@vger.kernel.org; van.freenix@gmail.com
> Subject: Re: [PATCH 2/2] percpu: km: no need to consider
> pcpu_group_offsets[0]
> 
> On Sun, Feb 24, 2019 at 01:13:50PM +0000, Peng Fan wrote:
> > percpu-km is used on UP systems which only has one group, so the group
> > offset will be always 0, there is no need to subtract
> > pcpu_group_offsets[0] when assigning chunk->base_addr
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  mm/percpu-km.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/percpu-km.c b/mm/percpu-km.c index
> > 66e5598be876..8872c21a487b 100644
> > --- a/mm/percpu-km.c
> > +++ b/mm/percpu-km.c
> > @@ -67,7 +67,7 @@ static struct pcpu_chunk *pcpu_create_chunk(gfp_t
> gfp)
> >  		pcpu_set_page_chunk(nth_page(pages, i), chunk);
> >
> >  	chunk->data = pages;
> > -	chunk->base_addr = page_address(pages) - pcpu_group_offsets[0];
> > +	chunk->base_addr = page_address(pages);
> >
> >  	spin_lock_irqsave(&pcpu_lock, flags);
> >  	pcpu_chunk_populated(chunk, 0, nr_pages, false);
> > --
> > 2.16.4
> >
> 
> While I do think you're right, creating a chunk is not a part of the
> critical path and subtracting 0 is incredibly minor overhead. So I'd
> rather keep the code as is to maintain consistency between percpu-vm.c
> and percpu-km.c.

That's ok to keep consistency, since you prefer that.

Thanks,
Peng.

> 
> Thanks,
> Dennis

Re: [PATCH 2/2] percpu: km: no need to consider pcpu_group_offsets[0]

[Christopher Lameter]

On Mon, 25 Feb 2019, dennis@kernel.org wrote:

> > @@ -67,7 +67,7 @@ static struct pcpu_chunk *pcpu_create_chunk(gfp_t gfp)
> >  		pcpu_set_page_chunk(nth_page(pages, i), chunk);
> >
> >  	chunk->data = pages;
> > -	chunk->base_addr = page_address(pages) - pcpu_group_offsets[0];
> > +	chunk->base_addr = page_address(pages);
> >
> >  	spin_lock_irqsave(&pcpu_lock, flags);
> >  	pcpu_chunk_populated(chunk, 0, nr_pages, false);
> > --
> > 2.16.4
> >
>
> While I do think you're right, creating a chunk is not a part of the
> critical path and subtracting 0 is incredibly minor overhead. So I'd
> rather keep the code as is to maintain consistency between percpu-vm.c
> and percpu-km.c.

Well it is confusing if there the expression is there but never used. It
is clearer with the patch.

Re: [PATCH 2/2] percpu: km: no need to consider pcpu_group_offsets[0]

[Christopher Lameter]

On Mon, 25 Feb 2019, dennis@kernel.org wrote:

> > @@ -67,7 +67,7 @@ static struct pcpu_chunk *pcpu_create_chunk(gfp_t gfp)
> >  		pcpu_set_page_chunk(nth_page(pages, i), chunk);
> >
> >  	chunk->data = pages;
> > -	chunk->base_addr = page_address(pages) - pcpu_group_offsets[0];
> > +	chunk->base_addr = page_address(pages);
> >
> >  	spin_lock_irqsave(&pcpu_lock, flags);
> >  	pcpu_chunk_populated(chunk, 0, nr_pages, false);
> > --
> > 2.16.4
> >
>
> While I do think you're right, creating a chunk is not a part of the
> critical path and subtracting 0 is incredibly minor overhead. So I'd
> rather keep the code as is to maintain consistency between percpu-vm.c
> and percpu-km.c.

Well it is confusing if there the expression is there but never used. It
is clearer with the patch.

Re: [PATCH 2/2] percpu: km: no need to consider pcpu_group_offsets[0]

[Dennis Zhou]

On Tue, Feb 26, 2019 at 03:15:50PM +0000, Christopher Lameter wrote:
> On Mon, 25 Feb 2019, dennis@kernel.org wrote:
> 
> > > @@ -67,7 +67,7 @@ static struct pcpu_chunk *pcpu_create_chunk(gfp_t gfp)
> > >  		pcpu_set_page_chunk(nth_page(pages, i), chunk);
> > >
> > >  	chunk->data = pages;
> > > -	chunk->base_addr = page_address(pages) - pcpu_group_offsets[0];
> > > +	chunk->base_addr = page_address(pages);
> > >
> > >  	spin_lock_irqsave(&pcpu_lock, flags);
> > >  	pcpu_chunk_populated(chunk, 0, nr_pages, false);
> > > --
> > > 2.16.4
> > >
> >
> > While I do think you're right, creating a chunk is not a part of the
> > critical path and subtracting 0 is incredibly minor overhead. So I'd
> > rather keep the code as is to maintain consistency between percpu-vm.c
> > and percpu-km.c.
> 
> Well it is confusing if there the expression is there but never used. It
> is clearer with the patch.
> 

Okay. I'll apply it with your ack if that's fine.

Thanks,
Dennis

Re: [PATCH 1/2] percpu: km: remove SMP check

[Dennis Zhou]

On Sun, Feb 24, 2019 at 01:13:43PM +0000, Peng Fan wrote:
> percpu-km could only be selected by NEED_PER_CPU_KM which
> depends on !SMP, so CONFIG_SMP will be false when choose percpu-km.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  mm/percpu-km.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/percpu-km.c b/mm/percpu-km.c
> index 0f643dc2dc65..66e5598be876 100644
> --- a/mm/percpu-km.c
> +++ b/mm/percpu-km.c
> @@ -27,7 +27,7 @@
>   *   chunk size is not aligned.  percpu-km code will whine about it.
>   */
>  
> -#if defined(CONFIG_SMP) && defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> +#if defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
>  #error "contiguous percpu allocation is incompatible with paged first chunk"
>  #endif
>  
> -- 
> 2.16.4
> 

Hi,

I think keeping CONFIG_SMP makes this easier to remember dependencies
rather than having to dig into the config. So this is a NACK from me.

Thanks,
Dennis

RE: [PATCH 1/2] percpu: km: remove SMP check

[Peng Fan]

Hi Dennis,

> -----Original Message-----
> From: Dennis Zhou [mailto:dennis@kernel.org]
> Sent: 2019年2月25日 23:14
> To: Peng Fan <peng.fan@nxp.com>
> Cc: dennis@kernel.org; tj@kernel.org; cl@linux.com; linux-mm@kvack.org;
> linux-kernel@vger.kernel.org; van.freenix@gmail.com
> Subject: Re: [PATCH 1/2] percpu: km: remove SMP check
> 
> On Sun, Feb 24, 2019 at 01:13:43PM +0000, Peng Fan wrote:
> > percpu-km could only be selected by NEED_PER_CPU_KM which depends
> on
> > !SMP, so CONFIG_SMP will be false when choose percpu-km.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  mm/percpu-km.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/percpu-km.c b/mm/percpu-km.c index
> > 0f643dc2dc65..66e5598be876 100644
> > --- a/mm/percpu-km.c
> > +++ b/mm/percpu-km.c
> > @@ -27,7 +27,7 @@
> >   *   chunk size is not aligned.  percpu-km code will whine about it.
> >   */
> >
> > -#if defined(CONFIG_SMP) &&
> > defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > +#if defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> >  #error "contiguous percpu allocation is incompatible with paged first
> chunk"
> >  #endif
> >
> > --
> > 2.16.4
> >
> 
> Hi,
> 
> I think keeping CONFIG_SMP makes this easier to remember dependencies
> rather than having to dig into the config. So this is a NACK from me.

You might be wrong here.
In mm/Kconfig, NEED_PER_CPU_KM default y depends on !SMP. So if CONFIG_SMP
is not defined, NEED_PER_CPU_KM will be true. If we also define
CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK, the #error will have no chance
to be detected because CONFIG_SMP already be false.
That means CONFIG_SMP will always be false if percpu-km is used.
So need to drop the CONFIG_SMP check here.

Thanks,
Peng.

> 
> Thanks,
> Dennis

Re: [PATCH 1/2] percpu: km: remove SMP check

[Christopher Lameter]

On Mon, 25 Feb 2019, Dennis Zhou wrote:

> > @@ -27,7 +27,7 @@
> >   *   chunk size is not aligned.  percpu-km code will whine about it.
> >   */
> >
> > -#if defined(CONFIG_SMP) && defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > +#if defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> >  #error "contiguous percpu allocation is incompatible with paged first chunk"
> >  #endif
> >
> > --
> > 2.16.4
> >
>
> Hi,
>
> I think keeping CONFIG_SMP makes this easier to remember dependencies
> rather than having to dig into the config. So this is a NACK from me.

But it simplifies the code and makes it easier to read.

Re: [PATCH 1/2] percpu: km: remove SMP check

[Christopher Lameter]

On Mon, 25 Feb 2019, Dennis Zhou wrote:

> > @@ -27,7 +27,7 @@
> >   *   chunk size is not aligned.  percpu-km code will whine about it.
> >   */
> >
> > -#if defined(CONFIG_SMP) && defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > +#if defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> >  #error "contiguous percpu allocation is incompatible with paged first chunk"
> >  #endif
> >
> > --
> > 2.16.4
> >
>
> Hi,
>
> I think keeping CONFIG_SMP makes this easier to remember dependencies
> rather than having to dig into the config. So this is a NACK from me.

But it simplifies the code and makes it easier to read.

Re: [PATCH 1/2] percpu: km: remove SMP check

[Dennis Zhou]

On Tue, Feb 26, 2019 at 03:16:44PM +0000, Christopher Lameter wrote:
> On Mon, 25 Feb 2019, Dennis Zhou wrote:
> 
> > > @@ -27,7 +27,7 @@
> > >   *   chunk size is not aligned.  percpu-km code will whine about it.
> > >   */
> > >
> > > -#if defined(CONFIG_SMP) && defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > > +#if defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > >  #error "contiguous percpu allocation is incompatible with paged first chunk"
> > >  #endif
> > >
> > > --
> > > 2.16.4
> > >
> >
> > Hi,
> >
> > I think keeping CONFIG_SMP makes this easier to remember dependencies
> > rather than having to dig into the config. So this is a NACK from me.
> 
> But it simplifies the code and makes it easier to read.
> 
> 

I think the check isn't quite right after looking at it a little longer.
Looking at x86, I believe you can compile it with !SMP and
CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK will still be set. This should
still work because x86 has an MMU.

I think more correctly it would be something like below, but I don't
have the time to fully verify it right now.

Thanks,
Dennis

---
diff --git a/mm/percpu-km.c b/mm/percpu-km.c
index 0f643dc2dc65..69ccad7d9807 100644
--- a/mm/percpu-km.c
+++ b/mm/percpu-km.c
@@ -27,7 +27,7 @@
  *   chunk size is not aligned.  percpu-km code will whine about it.
  */
 
-#if defined(CONFIG_SMP) && defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
+#if !defined(CONFIG_MMU) && defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
 #error "contiguous percpu allocation is incompatible with paged first chunk"
 #endif
 

RE: [PATCH 1/2] percpu: km: remove SMP check

[Peng Fan]

Hi Dennis

> -----Original Message-----
> From: Dennis Zhou [mailto:dennis@kernel.org]
> Sent: 2019年2月27日 1:04
> To: Christopher Lameter <cl@linux.com>
> Cc: Peng Fan <peng.fan@nxp.com>; tj@kernel.org; linux-mm@kvack.org;
> linux-kernel@vger.kernel.org; van.freenix@gmail.com
> Subject: Re: [PATCH 1/2] percpu: km: remove SMP check
> 
> On Tue, Feb 26, 2019 at 03:16:44PM +0000, Christopher Lameter wrote:
> > On Mon, 25 Feb 2019, Dennis Zhou wrote:
> >
> > > > @@ -27,7 +27,7 @@
> > > >   *   chunk size is not aligned.  percpu-km code will whine about it.
> > > >   */
> > > >
> > > > -#if defined(CONFIG_SMP) &&
> > > > defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > > > +#if defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > > >  #error "contiguous percpu allocation is incompatible with paged first
> chunk"
> > > >  #endif
> > > >
> > > > --
> > > > 2.16.4
> > > >
> > >
> > > Hi,
> > >
> > > I think keeping CONFIG_SMP makes this easier to remember
> > > dependencies rather than having to dig into the config. So this is a NACK
> from me.
> >
> > But it simplifies the code and makes it easier to read.
> >
> >
> 
> I think the check isn't quite right after looking at it a little longer.
> Looking at x86, I believe you can compile it with !SMP and
> CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK will still be set. This should
> still work because x86 has an MMU.

You are right, x86 could boots up with NEED_PER_CPU_PAGE_FIRST_CHUNK
=y and SMP=n. Tested with qemu, info as below:

/ # zcat /proc/config.gz | grep NEED_PER_CPU_KM
CONFIG_NEED_PER_CPU_KM=y
/ # zcat /proc/config.gz | grep SMP
CONFIG_BROKEN_ON_SMP=y
# CONFIG_SMP is not set
CONFIG_GENERIC_SMP_IDLE_THREAD=y
/ # zcat /proc/config.gz | grep NEED_PER_CPU_PAGE_FIRST_CHUNK
CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK=y
/ # cat /proc/cpuinfo
processor       : 0
vendor_id       : AuthenticAMD
cpu family      : 6
model           : 6
model name      : QEMU Virtual CPU version 2.5+
stepping        : 3
cpu MHz         : 3192.613
cache size      : 512 KB
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 syscall nx lm nopl cpuid pni cx16 hypervisor lahf_lm svm 3dnowprefetl
bugs            : fxsave_leak sysret_ss_attrs spectre_v1 spectre_v2 spec_store_bypass
bogomips        : 6385.22
TLB size        : 1024 4K pages
clflush size    : 64
cache_alignment : 64
address sizes   : 42 bits physical, 48 bits virtual
power management:


But from the comments in this file:
"
* - CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK must not be defined.  It's
 *   not compatible with PER_CPU_KM.  EMBED_FIRST_CHUNK should work
 *   fine.
"

I did not read into details why it is not allowed, but x86 could still work with KM
and NEED_PER_CPU_PAGE_FIRST_CHUNK.

> 
> I think more correctly it would be something like below, but I don't have the
> time to fully verify it right now.
> 
> Thanks,
> Dennis
> 
> ---
> diff --git a/mm/percpu-km.c b/mm/percpu-km.c index
> 0f643dc2dc65..69ccad7d9807 100644
> --- a/mm/percpu-km.c
> +++ b/mm/percpu-km.c
> @@ -27,7 +27,7 @@
>   *   chunk size is not aligned.  percpu-km code will whine about it.
>   */
> 
> -#if defined(CONFIG_SMP) &&
> defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> +#if !defined(CONFIG_MMU) &&
> +defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
>  #error "contiguous percpu allocation is incompatible with paged first chunk"
>  #endif
> 

Acked-by: Peng Fan <peng.fan@nxp.com>

Thanks,
Peng

Re: [PATCH 1/2] percpu: km: remove SMP check

[Dennis Zhou]

On Wed, Feb 27, 2019 at 01:02:16PM +0000, Peng Fan wrote:
> Hi Dennis
> 
> > -----Original Message-----
> > From: Dennis Zhou [mailto:dennis@kernel.org]
> > Sent: 2019年2月27日 1:04
> > To: Christopher Lameter <cl@linux.com>
> > Cc: Peng Fan <peng.fan@nxp.com>; tj@kernel.org; linux-mm@kvack.org;
> > linux-kernel@vger.kernel.org; van.freenix@gmail.com
> > Subject: Re: [PATCH 1/2] percpu: km: remove SMP check
> > 
> > On Tue, Feb 26, 2019 at 03:16:44PM +0000, Christopher Lameter wrote:
> > > On Mon, 25 Feb 2019, Dennis Zhou wrote:
> > >
> > > > > @@ -27,7 +27,7 @@
> > > > >   *   chunk size is not aligned.  percpu-km code will whine about it.
> > > > >   */
> > > > >
> > > > > -#if defined(CONFIG_SMP) &&
> > > > > defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > > > > +#if defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > > > >  #error "contiguous percpu allocation is incompatible with paged first
> > chunk"
> > > > >  #endif
> > > > >
> > > > > --
> > > > > 2.16.4
> > > > >
> > > >
> > > > Hi,
> > > >
> > > > I think keeping CONFIG_SMP makes this easier to remember
> > > > dependencies rather than having to dig into the config. So this is a NACK
> > from me.
> > >
> > > But it simplifies the code and makes it easier to read.
> > >
> > >
> > 
> > I think the check isn't quite right after looking at it a little longer.
> > Looking at x86, I believe you can compile it with !SMP and
> > CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK will still be set. This should
> > still work because x86 has an MMU.
> 
> You are right, x86 could boots up with NEED_PER_CPU_PAGE_FIRST_CHUNK
> =y and SMP=n. Tested with qemu, info as below:
> 
> / # zcat /proc/config.gz | grep NEED_PER_CPU_KM
> CONFIG_NEED_PER_CPU_KM=y
> / # zcat /proc/config.gz | grep SMP
> CONFIG_BROKEN_ON_SMP=y
> # CONFIG_SMP is not set
> CONFIG_GENERIC_SMP_IDLE_THREAD=y
> / # zcat /proc/config.gz | grep NEED_PER_CPU_PAGE_FIRST_CHUNK
> CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK=y
> / # cat /proc/cpuinfo
> processor       : 0
> vendor_id       : AuthenticAMD
> cpu family      : 6
> model           : 6
> model name      : QEMU Virtual CPU version 2.5+
> stepping        : 3
> cpu MHz         : 3192.613
> cache size      : 512 KB
> fpu             : yes
> fpu_exception   : yes
> cpuid level     : 13
> wp              : yes
> flags           : fpu de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 syscall nx lm nopl cpuid pni cx16 hypervisor lahf_lm svm 3dnowprefetl
> bugs            : fxsave_leak sysret_ss_attrs spectre_v1 spectre_v2 spec_store_bypass
> bogomips        : 6385.22
> TLB size        : 1024 4K pages
> clflush size    : 64
> cache_alignment : 64
> address sizes   : 42 bits physical, 48 bits virtual
> power management:
> 
> 
> But from the comments in this file:
> "
> * - CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK must not be defined.  It's
>  *   not compatible with PER_CPU_KM.  EMBED_FIRST_CHUNK should work
>  *   fine.
> "
> 
> I did not read into details why it is not allowed, but x86 could still work with KM
> and NEED_PER_CPU_PAGE_FIRST_CHUNK.
> 

The first chunk requires special handling on SMP to bring the static
variables into the percpu address space. On UP, identity mapping makes
static variables indistinguishable by aligning the percpu address space
and the virtual adress space. The percpu-km allocator allocates full
chunks at a time to deal with NOMMU archs. So the difference is if the
virtual address space is the same as the physical.

> > 
> > I think more correctly it would be something like below, but I don't have the
> > time to fully verify it right now.
> > 
> > Thanks,
> > Dennis
> > 
> > ---
> > diff --git a/mm/percpu-km.c b/mm/percpu-km.c index
> > 0f643dc2dc65..69ccad7d9807 100644
> > --- a/mm/percpu-km.c
> > +++ b/mm/percpu-km.c
> > @@ -27,7 +27,7 @@
> >   *   chunk size is not aligned.  percpu-km code will whine about it.
> >   */
> > 
> > -#if defined(CONFIG_SMP) &&
> > defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > +#if !defined(CONFIG_MMU) &&
> > +defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> >  #error "contiguous percpu allocation is incompatible with paged first chunk"
> >  #endif
> > 
> 
> Acked-by: Peng Fan <peng.fan@nxp.com>
> 
> Thanks,
> Peng

While this change may seem right to me. Verification would be to double
check other architectures too. x86 just happened to be a counter example
I had in mind. Unless someone reports this as being an issue or someone
takes the time to validate this more thoroughly than my cursory look.
I think the risk of this outweighs the benefit. This may be something I
fix in the future when I have more time. This would also involve making
sure the comments are consistent.

Thanks,
Dennis

RE: [PATCH 1/2] percpu: km: remove SMP check

[Peng Fan]

> -----Original Message-----
> From: Dennis Zhou [mailto:dennis@kernel.org]
> Sent: 2019年2月28日 0:41
> To: Peng Fan <peng.fan@nxp.com>
> Cc: Dennis Zhou <dennis@kernel.org>; Christopher Lameter <cl@linux.com>;
> tj@kernel.org; linux-mm@kvack.org; linux-kernel@vger.kernel.org;
> van.freenix@gmail.com
> Subject: Re: [PATCH 1/2] percpu: km: remove SMP check
> 
> On Wed, Feb 27, 2019 at 01:02:16PM +0000, Peng Fan wrote:
> > Hi Dennis
> >
> > > -----Original Message-----
> > > From: Dennis Zhou [mailto:dennis@kernel.org]
> > > Sent: 2019年2月27日 1:04
> > > To: Christopher Lameter <cl@linux.com>
> > > Cc: Peng Fan <peng.fan@nxp.com>; tj@kernel.org; linux-mm@kvack.org;
> > > linux-kernel@vger.kernel.org; van.freenix@gmail.com
> > > Subject: Re: [PATCH 1/2] percpu: km: remove SMP check
> > >
> > > On Tue, Feb 26, 2019 at 03:16:44PM +0000, Christopher Lameter wrote:
> > > > On Mon, 25 Feb 2019, Dennis Zhou wrote:
> > > >
> > > > > > @@ -27,7 +27,7 @@
> > > > > >   *   chunk size is not aligned.  percpu-km code will whine about
> it.
> > > > > >   */
> > > > > >
> > > > > > -#if defined(CONFIG_SMP) &&
> > > > > > defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > > > > > +#if defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > > > > >  #error "contiguous percpu allocation is incompatible with
> > > > > > paged first
> > > chunk"
> > > > > >  #endif
> > > > > >
> > > > > > --
> > > > > > 2.16.4
> > > > > >
> > > > >
> > > > > Hi,
> > > > >
> > > > > I think keeping CONFIG_SMP makes this easier to remember
> > > > > dependencies rather than having to dig into the config. So this
> > > > > is a NACK
> > > from me.
> > > >
> > > > But it simplifies the code and makes it easier to read.
> > > >
> > > >
> > >
> > > I think the check isn't quite right after looking at it a little longer.
> > > Looking at x86, I believe you can compile it with !SMP and
> > > CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK will still be set. This
> should
> > > still work because x86 has an MMU.
> >
> > You are right, x86 could boots up with
> NEED_PER_CPU_PAGE_FIRST_CHUNK
> > =y and SMP=n. Tested with qemu, info as below:
> >
> > / # zcat /proc/config.gz | grep NEED_PER_CPU_KM
> > CONFIG_NEED_PER_CPU_KM=y / # zcat /proc/config.gz | grep SMP
> > CONFIG_BROKEN_ON_SMP=y # CONFIG_SMP is not set
> > CONFIG_GENERIC_SMP_IDLE_THREAD=y / # zcat /proc/config.gz | grep
> > NEED_PER_CPU_PAGE_FIRST_CHUNK
> CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK=y
> > / # cat /proc/cpuinfo
> > processor       : 0
> > vendor_id       : AuthenticAMD
> > cpu family      : 6
> > model           : 6
> > model name      : QEMU Virtual CPU version 2.5+
> > stepping        : 3
> > cpu MHz         : 3192.613
> > cache size      : 512 KB
> > fpu             : yes
> > fpu_exception   : yes
> > cpuid level     : 13
> > wp              : yes
> > flags           : fpu de pse tsc msr pae mce cx8 apic sep mtrr pge mca
> cmov pat pse36 clflush mmx fxsr sse sse2 syscall nx lm nopl cpuid pni cx16
> hypervisor lahf_lm svm 3dnowprefetl
> > bugs            : fxsave_leak sysret_ss_attrs spectre_v1 spectre_v2
> spec_store_bypass
> > bogomips        : 6385.22
> > TLB size        : 1024 4K pages
> > clflush size    : 64
> > cache_alignment : 64
> > address sizes   : 42 bits physical, 48 bits virtual
> > power management:
> >
> >
> > But from the comments in this file:
> > "
> > * - CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK must not be defined.
> It's
> >  *   not compatible with PER_CPU_KM.  EMBED_FIRST_CHUNK should
> work
> >  *   fine.
> > "
> >
> > I did not read into details why it is not allowed, but x86 could still
> > work with KM and NEED_PER_CPU_PAGE_FIRST_CHUNK.
> >
> 
> The first chunk requires special handling on SMP to bring the static variables
> into the percpu address space. On UP, identity mapping makes static variables
> indistinguishable by aligning the percpu address space and the virtual adress
> space. The percpu-km allocator allocates full chunks at a time to deal with
> NOMMU archs. So the difference is if the virtual address space is the same as
> the physical.

Thanks for clarification.

> 
> > >
> > > I think more correctly it would be something like below, but I don't
> > > have the time to fully verify it right now.
> > >
> > > Thanks,
> > > Dennis
> > >
> > > ---
> > > diff --git a/mm/percpu-km.c b/mm/percpu-km.c index
> > > 0f643dc2dc65..69ccad7d9807 100644
> > > --- a/mm/percpu-km.c
> > > +++ b/mm/percpu-km.c
> > > @@ -27,7 +27,7 @@
> > >   *   chunk size is not aligned.  percpu-km code will whine about it.
> > >   */
> > >
> > > -#if defined(CONFIG_SMP) &&
> > > defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > > +#if !defined(CONFIG_MMU) &&
> > > +defined(CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK)
> > >  #error "contiguous percpu allocation is incompatible with paged first
> chunk"
> > >  #endif
> > >
> >
> > Acked-by: Peng Fan <peng.fan@nxp.com>
> >
> > Thanks,
> > Peng
> 
> While this change may seem right to me. Verification would be to double
> check other architectures too. x86 just happened to be a counter example I
> had in mind. Unless someone reports this as being an issue or someone takes
> the time to validate this more thoroughly than my cursory look.
> I think the risk of this outweighs the benefit. This may be something I fix in the
> future when I have more time. This would also involve making sure the
> comments are consistent.

I am not able to check other architectures now. So just leave the code as is.

Thanks,
Peng.

> 
> Thanks,
> Dennis