[PATCH] percpu: km: Use for SMP+NOMMU

[PATCH] percpu: km: Use for SMP+NOMMU

[Vladimir Murzin]

I have recently updated kernel for R-class (SMP+NOMMU) and observed
following build failure:

arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'

ARM NOMMU declare flush_tlb_kernel_range() but doesn't define it, so I
tried to understand what pulls that function...

First candidate is 93274f1dd6b0 ("percpu: flush tlb in
pcpu_reclaim_populated()") which added another user of
pcpu_post_unmap_tlb_flush(), yet simple revert did not make things
better...

The second candidate turned to be 4ad0ae8c64ac ("mm/vmalloc: remove
unmap_kernel_range"), more precisely NOMMU part. This one is
interesting. Before conversion to vmap_pages_range_noflush() we had

static inline int
map_kernel_range_noflush(unsigned long start, unsigned long size,
                        pgprot_t prot, struct page **pages)
{
        return size >> PAGE_SHIFT;
}

static int __pcpu_map_pages(unsigned long addr, struct page **pages,
                            int nr_pages)
{
        return map_kernel_range_noflush(addr, nr_pages << PAGE_SHIFT,
                                        PAGE_KERNEL, pages);
}

static int pcpu_map_pages(struct pcpu_chunk *chunk,
                          struct page **pages, int page_start, int page_end)
{
        unsigned int cpu, tcpu;
        int i, err;

        for_each_possible_cpu(cpu) {
                err = __pcpu_map_pages(pcpu_chunk_addr(chunk, cpu, page_start),
                                       &pages[pcpu_page_idx(cpu, page_start)],
                                       page_end - page_start);
                if (err < 0)
                        goto err;

                for (i = page_start; i < page_end; i++)
                        pcpu_set_page_chunk(pages[pcpu_page_idx(cpu, i)],
                                            chunk);
        }
        return 0;
err:
        for_each_possible_cpu(tcpu) {
                if (tcpu == cpu)
                        break;
                __pcpu_unmap_pages(pcpu_chunk_addr(chunk, tcpu, page_start),
                                   page_end - page_start);
        }
        pcpu_post_unmap_tlb_flush(chunk, page_start, page_end);
        return err;
}

Here __pcpu_map_pages() would never return negative value, so
compiler optimizes error path and pcpu_post_unmap_tlb_flush() never
referenced.

After conversion to vmap_pages_range_noflush() we got

static inline
int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
                pgprot_t prot, struct page **pages, unsigned int page_shift)
{
       return -EINVAL;
}

static int __pcpu_map_pages(unsigned long addr, struct page **pages,
                            int nr_pages)
{
        return vmap_pages_range_noflush(addr, addr + (nr_pages << PAGE_SHIFT),
                                        PAGE_KERNEL, pages, PAGE_SHIFT);
}

and pcpu_map_pages() unchanged.

Here __pcpu_map_pages() always return negative value, so compiler
cannot optimise error path and pcpu_post_unmap_tlb_flush() stay
referenced.

Now it is kind of clear why it worked before and why it refuses to build.

Next is to understand how to fix that. I noticed [1] following comment
from Nicholas:

> Previous code had a strange NOMMU implementation of
> map_kernel_range_noflush that came in with commit b554cb426a955
> ("NOMMU: support SMP dynamic percpu_alloc") which would return
> success if the size was <= PAGE_SIZE, but that has no way of working
> on NOMMU because the page can not be mapped to the new start address
> even if there is only one of them. So change this code to always
> return failure.
>
> NOMMU probably needs to take a closer look at what it does with
> percpu-vm.c and carve out a special case there if necessary rather
> than pretend vmap works.

So I started looking into it. First thing I noticed is than UP+NOMMU
doesn't run into the same issue. The reason is that they pull
mm/percpu-km.c with

config NEED_PER_CPU_KM
        depends on !SMP
        bool
        default y

Looking more into history of kernel memory based allocator it looks
like it was designed with SMP in mind at least it is mentioned in
b0c9778b1d07 ("percpu: implement kernel memory based chunk
allocation"):

> Implement an alternate percpu chunk management based on kernel memeory
> for nommu SMP architectures. 

... and even latter when NEED_PER_CPU_KM was introduced by
bbddff054587 ("percpu: use percpu allocator on UP too"):

> Currently, users of percpu allocators need to handle UP differently,
> which is somewhat fragile and ugly.  Other than small amount of
> memory, there isn't much to lose by enabling percpu allocator on UP.
> It can simply use kernel memory based chunk allocation which was added
> for SMP archs w/o MMUs.

I could not find justification on prohibiting SMP+NOMMU in patch
discussion [2] either. It looks like that dependency was overlooked and
did not consider SMP+NOMMU.

This probably also was a reason for b554cb426a95 ("NOMMU: support SMP
dynamic percpu_alloc"), yet patch author and reviewers were also included
into original b0c9778b1d07 ("percpu: implement kernel memory based chunk
allocation")...

Unless I'm missing something I'm proposing bringing kernel memory based
allocator back for use with SMP+NOMMU

Vladimir Murzin (1):
  percpu: km: ensure it is used with NOMMU (either UP or SMP)

 mm/Kconfig | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.7.4

[PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Vladimir Murzin]

Currently, NOMMU pull km allocator via !SMP dependency because most of
them are UP, yet for SMP+NOMMU vm allocator gets pulled which:

* may lead to broken build [1]
* ...or not working runtime due to [2]

It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
use percpu allocator on UP too") so restore that.

[1]
For ARM SMP+NOMMU (R-class cores)

arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'

[2]
static inline
int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
                pgprot_t prot, struct page **pages, unsigned int page_shift)
{
       return -EINVAL;
}

Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
---
 mm/Kconfig | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/Kconfig b/mm/Kconfig
index d16ba92..66331e0 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -425,9 +425,8 @@ config THP_SWAP
 # UP and nommu archs use km based percpu allocator
 #
 config NEED_PER_CPU_KM
-	depends on !SMP
 	bool
-	default y
+	default !SMP || !MMU
 
 config CLEANCACHE
 	bool "Enable cleancache driver to cache clean pages if tmem is present"
-- 
2.7.4

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Dennis Zhou]

Hello,

On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> Currently, NOMMU pull km allocator via !SMP dependency because most of
> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> 
> * may lead to broken build [1]
> * ...or not working runtime due to [2]
> 
> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> use percpu allocator on UP too") so restore that.
> 
> [1]
> For ARM SMP+NOMMU (R-class cores)
> 
> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> 
> [2]
> static inline
> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> {
>        return -EINVAL;
> }
> 
> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> ---
>  mm/Kconfig | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/Kconfig b/mm/Kconfig
> index d16ba92..66331e0 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -425,9 +425,8 @@ config THP_SWAP
>  # UP and nommu archs use km based percpu allocator
>  #
>  config NEED_PER_CPU_KM
> -	depends on !SMP
>  	bool
> -	default y
> +	default !SMP || !MMU
>  

Should this be `depends on !SMP || !MMU` with default yes? Because with
SMP && MMU, it shouldn't be an option to run with percpu-km.

>  config CLEANCACHE
>  	bool "Enable cleancache driver to cache clean pages if tmem is present"
> -- 
> 2.7.4
> 

It's interesting to me that this is all coming up at once. Earlier this
month I had the same conversation with people involved with sh [1].

[1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/

I can pull this shortly once I see whatever happened to linux-sh.

Thanks,
Dennis

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Vladimir Murzin]

Hi,

On 11/30/21 5:41 PM, Dennis Zhou wrote:
> Hello,
> 
> On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
>> Currently, NOMMU pull km allocator via !SMP dependency because most of
>> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
>>
>> * may lead to broken build [1]
>> * ...or not working runtime due to [2]
>>
>> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
>> use percpu allocator on UP too") so restore that.
>>
>> [1]
>> For ARM SMP+NOMMU (R-class cores)
>>
>> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
>> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
>>
>> [2]
>> static inline
>> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
>>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
>> {
>>        return -EINVAL;
>> }
>>
>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>> ---
>>  mm/Kconfig | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/mm/Kconfig b/mm/Kconfig
>> index d16ba92..66331e0 100644
>> --- a/mm/Kconfig
>> +++ b/mm/Kconfig
>> @@ -425,9 +425,8 @@ config THP_SWAP
>>  # UP and nommu archs use km based percpu allocator
>>  #
>>  config NEED_PER_CPU_KM
>> -	depends on !SMP
>>  	bool
>> -	default y
>> +	default !SMP || !MMU
>>  
> 
> Should this be `depends on !SMP || !MMU` with default yes? Because with
> SMP && MMU, it shouldn't be an option to run with percpu-km.

IIUC these are equivalent, truth table would not change if is under "depends"
or "default"

SMP    MMU   NEED_PER_CPU_KM
 y      y    !y || !y => n || n => n
 y      n    !y || !n => n || y => y
 n      y    !n || !y => y || n => y
 n      n    !n || !n => y || y => y

> 
>>  config CLEANCACHE
>>  	bool "Enable cleancache driver to cache clean pages if tmem is present"
>> -- 
>> 2.7.4
>>
> 
> It's interesting to me that this is all coming up at once. Earlier this
> month I had the same conversation with people involved with sh [1].
> 
> [1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/
> 
> I can pull this shortly once I see whatever happened to linux-sh.

Ahh, good to know! Adding SH folks here (start of discussion [0]). I see you came
to the same conclusion, right? 

IIRC, RISC-V also have SMP+NOMMU, so adding them as well.

[0] https://lore.kernel.org/linux-mm/20211130172954.129587-1-vladimir.murzin@arm.com/T/

Cheers
Vladimir

> 
> Thanks,
> Dennis
> 

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Dennis Zhou]

On Wed, Dec 01, 2021 at 11:51:04AM +0000, Vladimir Murzin wrote:
> Hi,
> 
> On 11/30/21 5:41 PM, Dennis Zhou wrote:
> > Hello,
> > 
> > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> >> Currently, NOMMU pull km allocator via !SMP dependency because most of
> >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> >>
> >> * may lead to broken build [1]
> >> * ...or not working runtime due to [2]
> >>
> >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> >> use percpu allocator on UP too") so restore that.
> >>
> >> [1]
> >> For ARM SMP+NOMMU (R-class cores)
> >>
> >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> >>
> >> [2]
> >> static inline
> >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> >> {
> >>        return -EINVAL;
> >> }
> >>
> >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> >> ---
> >>  mm/Kconfig | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/mm/Kconfig b/mm/Kconfig
> >> index d16ba92..66331e0 100644
> >> --- a/mm/Kconfig
> >> +++ b/mm/Kconfig
> >> @@ -425,9 +425,8 @@ config THP_SWAP
> >>  # UP and nommu archs use km based percpu allocator
> >>  #
> >>  config NEED_PER_CPU_KM
> >> -	depends on !SMP
> >>  	bool
> >> -	default y
> >> +	default !SMP || !MMU
> >>  
> > 
> > Should this be `depends on !SMP || !MMU` with default yes? Because with
> > SMP && MMU, it shouldn't be an option to run with percpu-km.
> 
> IIUC these are equivalent, truth table would not change if is under "depends"
> or "default"
> 
> SMP    MMU   NEED_PER_CPU_KM
>  y      y    !y || !y => n || n => n
>  y      n    !y || !n => n || y => y
>  n      y    !n || !y => y || n => y
>  n      n    !n || !n => y || y => y
> 

I may be wrong, but I think this is slightly different as we're using
#ifdef / #if defined().

> > 
> >>  config CLEANCACHE
> >>  	bool "Enable cleancache driver to cache clean pages if tmem is present"
> >> -- 
> >> 2.7.4
> >>
> > 
> > It's interesting to me that this is all coming up at once. Earlier this
> > month I had the same conversation with people involved with sh [1].
> > 
> > [1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/
> > 
> > I can pull this shortly once I see whatever happened to linux-sh.
> 
> Ahh, good to know! Adding SH folks here (start of discussion [0]). I see you came
> to the same conclusion, right? 
> 

Yeah, I don't see anything else from linux-sh. So I'll go ahead and
apply this with my change if you're fine with that.

> IIRC, RISC-V also have SMP+NOMMU, so adding them as well.
> 
> [0] https://lore.kernel.org/linux-mm/20211130172954.129587-1-vladimir.murzin@arm.com/T/
> 
> Cheers
> Vladimir
> 

Thanks,
Dennis

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Vladimir Murzin]

On 12/3/21 9:02 PM, Dennis Zhou wrote:
> On Wed, Dec 01, 2021 at 11:51:04AM +0000, Vladimir Murzin wrote:
>> Hi,
>>
>> On 11/30/21 5:41 PM, Dennis Zhou wrote:
>>> Hello,
>>>
>>> On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
>>>> Currently, NOMMU pull km allocator via !SMP dependency because most of
>>>> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
>>>>
>>>> * may lead to broken build [1]
>>>> * ...or not working runtime due to [2]
>>>>
>>>> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
>>>> use percpu allocator on UP too") so restore that.
>>>>
>>>> [1]
>>>> For ARM SMP+NOMMU (R-class cores)
>>>>
>>>> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
>>>> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
>>>>
>>>> [2]
>>>> static inline
>>>> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
>>>>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
>>>> {
>>>>        return -EINVAL;
>>>> }
>>>>
>>>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>>>> ---
>>>>  mm/Kconfig | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/Kconfig b/mm/Kconfig
>>>> index d16ba92..66331e0 100644
>>>> --- a/mm/Kconfig
>>>> +++ b/mm/Kconfig
>>>> @@ -425,9 +425,8 @@ config THP_SWAP
>>>>  # UP and nommu archs use km based percpu allocator
>>>>  #
>>>>  config NEED_PER_CPU_KM
>>>> -	depends on !SMP
>>>>  	bool
>>>> -	default y
>>>> +	default !SMP || !MMU
>>>>  
>>>
>>> Should this be `depends on !SMP || !MMU` with default yes? Because with
>>> SMP && MMU, it shouldn't be an option to run with percpu-km.
>>
>> IIUC these are equivalent, truth table would not change if is under "depends"
>> or "default"
>>
>> SMP    MMU   NEED_PER_CPU_KM
>>  y      y    !y || !y => n || n => n
>>  y      n    !y || !n => n || y => y
>>  n      y    !n || !y => y || n => y
>>  n      n    !n || !n => y || y => y
>>
> 
> I may be wrong, but I think this is slightly different as we're using
> #ifdef / #if defined().
> 
>>>
>>>>  config CLEANCACHE
>>>>  	bool "Enable cleancache driver to cache clean pages if tmem is present"
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>>> It's interesting to me that this is all coming up at once. Earlier this
>>> month I had the same conversation with people involved with sh [1].
>>>
>>> [1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/
>>>
>>> I can pull this shortly once I see whatever happened to linux-sh.
>>
>> Ahh, good to know! Adding SH folks here (start of discussion [0]). I see you came
>> to the same conclusion, right? 
>>
> 
> Yeah, I don't see anything else from linux-sh. So I'll go ahead and
> apply this with my change if you're fine with that.

depends on !SMP || !MMU, also works for me, so feel free to amend. 

Thanks
Vladimir
> 
>> IIRC, RISC-V also have SMP+NOMMU, so adding them as well.
>>
>> [0] https://lore.kernel.org/linux-mm/20211130172954.129587-1-vladimir.murzin@arm.com/T/
>>
>> Cheers
>> Vladimir
>>
> 
> Thanks,
> Dennis
> 

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Rob Landley]

On 12/3/21 3:02 PM, Dennis Zhou wrote:
> On Wed, Dec 01, 2021 at 11:51:04AM +0000, Vladimir Murzin wrote:
>> Hi,
>> 
>> On 11/30/21 5:41 PM, Dennis Zhou wrote:
>> > Hello,
>> > 
>> > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
>> >> Currently, NOMMU pull km allocator via !SMP dependency because most of
>> >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
>> >>
>> >> * may lead to broken build [1]
>> >> * ...or not working runtime due to [2]
>> >>
>> >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
>> >> use percpu allocator on UP too") so restore that.
>> >>
>> >> [1]
>> >> For ARM SMP+NOMMU (R-class cores)
>> >>
>> >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
>> >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
>> >>
>> >> [2]
>> >> static inline
>> >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
>> >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
>> >> {
>> >>        return -EINVAL;
>> >> }
>> >>
>> >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>> >> ---
>> >>  mm/Kconfig | 3 +--
>> >>  1 file changed, 1 insertion(+), 2 deletions(-)
>> >>
>> >> diff --git a/mm/Kconfig b/mm/Kconfig
>> >> index d16ba92..66331e0 100644
>> >> --- a/mm/Kconfig
>> >> +++ b/mm/Kconfig
>> >> @@ -425,9 +425,8 @@ config THP_SWAP
>> >>  # UP and nommu archs use km based percpu allocator
>> >>  #
>> >>  config NEED_PER_CPU_KM
>> >> -	depends on !SMP
>> >>  	bool
>> >> -	default y
>> >> +	default !SMP || !MMU
>> >>  
>> > 
>> > Should this be `depends on !SMP || !MMU` with default yes? Because with
>> > SMP && MMU, it shouldn't be an option to run with percpu-km.
>> 
>> IIUC these are equivalent, truth table would not change if is under "depends"
>> or "default"
>> 
>> SMP    MMU   NEED_PER_CPU_KM
>>  y      y    !y || !y => n || n => n
>>  y      n    !y || !n => n || y => y
>>  n      y    !n || !y => y || n => y
>>  n      n    !n || !n => y || y => y
>> 
> 
> I may be wrong, but I think this is slightly different as we're using
> #ifdef / #if defined().
> 
>> > 
>> >>  config CLEANCACHE
>> >>  	bool "Enable cleancache driver to cache clean pages if tmem is present"
>> >> -- 
>> >> 2.7.4
>> >>
>> > 
>> > It's interesting to me that this is all coming up at once. Earlier this
>> > month I had the same conversation with people involved with sh [1].
>> > 
>> > [1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/
>> > 
>> > I can pull this shortly once I see whatever happened to linux-sh.
>> 
>> Ahh, good to know! Adding SH folks here (start of discussion [0]). I see you came
>> to the same conclusion, right? 
> 
> Yeah, I don't see anything else from linux-sh. So I'll go ahead and
> apply this with my change if you're fine with that.

I can't test against current until I get some unrelated fixes from Rich Felker
(who's been busy over the weekend), but I tested the "depends" version on 5.10
and got a shell prompt on my "make ARCH=sh j2_defconfig" board.

Tested-by: Rob Landley <rob@landley.net>

Rob

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Rich Felker]

On Mon, Dec 06, 2021 at 06:01:59AM -0600, Rob Landley wrote:
> On 12/3/21 3:02 PM, Dennis Zhou wrote:
> > On Wed, Dec 01, 2021 at 11:51:04AM +0000, Vladimir Murzin wrote:
> >> Hi,
> >> 
> >> On 11/30/21 5:41 PM, Dennis Zhou wrote:
> >> > Hello,
> >> > 
> >> > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> >> >> Currently, NOMMU pull km allocator via !SMP dependency because most of
> >> >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> >> >>
> >> >> * may lead to broken build [1]
> >> >> * ...or not working runtime due to [2]
> >> >>
> >> >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> >> >> use percpu allocator on UP too") so restore that.
> >> >>
> >> >> [1]
> >> >> For ARM SMP+NOMMU (R-class cores)
> >> >>
> >> >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> >> >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> >> >>
> >> >> [2]
> >> >> static inline
> >> >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> >> >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> >> >> {
> >> >>        return -EINVAL;
> >> >> }
> >> >>
> >> >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> >> >> ---
> >> >>  mm/Kconfig | 3 +--
> >> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/mm/Kconfig b/mm/Kconfig
> >> >> index d16ba92..66331e0 100644
> >> >> --- a/mm/Kconfig
> >> >> +++ b/mm/Kconfig
> >> >> @@ -425,9 +425,8 @@ config THP_SWAP
> >> >>  # UP and nommu archs use km based percpu allocator
> >> >>  #
> >> >>  config NEED_PER_CPU_KM
> >> >> -	depends on !SMP
> >> >>  	bool
> >> >> -	default y
> >> >> +	default !SMP || !MMU
> >> >>  
> >> > 
> >> > Should this be `depends on !SMP || !MMU` with default yes? Because with
> >> > SMP && MMU, it shouldn't be an option to run with percpu-km.
> >> 
> >> IIUC these are equivalent, truth table would not change if is under "depends"
> >> or "default"
> >> 
> >> SMP    MMU   NEED_PER_CPU_KM
> >>  y      y    !y || !y => n || n => n
> >>  y      n    !y || !n => n || y => y
> >>  n      y    !n || !y => y || n => y
> >>  n      n    !n || !n => y || y => y
> >> 
> > 
> > I may be wrong, but I think this is slightly different as we're using
> > #ifdef / #if defined().
> > 
> >> > 
> >> >>  config CLEANCACHE
> >> >>  	bool "Enable cleancache driver to cache clean pages if tmem is present"
> >> >> -- 
> >> >> 2.7.4
> >> >>
> >> > 
> >> > It's interesting to me that this is all coming up at once. Earlier this
> >> > month I had the same conversation with people involved with sh [1].
> >> > 
> >> > [1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/
> >> > 
> >> > I can pull this shortly once I see whatever happened to linux-sh.
> >> 
> >> Ahh, good to know! Adding SH folks here (start of discussion [0]). I see you came
> >> to the same conclusion, right? 
> > 
> > Yeah, I don't see anything else from linux-sh. So I'll go ahead and
> > apply this with my change if you're fine with that.
> 
> I can't test against current until I get some unrelated fixes from Rich Felker
> (who's been busy over the weekend), but I tested the "depends" version on 5.10
> and got a shell prompt on my "make ARCH=sh j2_defconfig" board.
> 
> Tested-by: Rob Landley <rob@landley.net>

Tested-by: Rich Felker <dalias@libc.org>

I've tested the version as originally posted and it works for j2 (sh2)
(both build and runtime checked). I'd tested the other (depends
version) before and I'm fine with either going upstream.

Rich

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Dennis Zhou]

On Mon, Dec 06, 2021 at 11:21:46AM -0500, Rich Felker wrote:
> On Mon, Dec 06, 2021 at 06:01:59AM -0600, Rob Landley wrote:
> > On 12/3/21 3:02 PM, Dennis Zhou wrote:
> > > On Wed, Dec 01, 2021 at 11:51:04AM +0000, Vladimir Murzin wrote:
> > >> Hi,
> > >> 
> > >> On 11/30/21 5:41 PM, Dennis Zhou wrote:
> > >> > Hello,
> > >> > 
> > >> > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> > >> >> Currently, NOMMU pull km allocator via !SMP dependency because most of
> > >> >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> > >> >>
> > >> >> * may lead to broken build [1]
> > >> >> * ...or not working runtime due to [2]
> > >> >>
> > >> >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> > >> >> use percpu allocator on UP too") so restore that.
> > >> >>
> > >> >> [1]
> > >> >> For ARM SMP+NOMMU (R-class cores)
> > >> >>
> > >> >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> > >> >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> > >> >>
> > >> >> [2]
> > >> >> static inline
> > >> >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> > >> >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> > >> >> {
> > >> >>        return -EINVAL;
> > >> >> }
> > >> >>
> > >> >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> > >> >> ---
> > >> >>  mm/Kconfig | 3 +--
> > >> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> > >> >>
> > >> >> diff --git a/mm/Kconfig b/mm/Kconfig
> > >> >> index d16ba92..66331e0 100644
> > >> >> --- a/mm/Kconfig
> > >> >> +++ b/mm/Kconfig
> > >> >> @@ -425,9 +425,8 @@ config THP_SWAP
> > >> >>  # UP and nommu archs use km based percpu allocator
> > >> >>  #
> > >> >>  config NEED_PER_CPU_KM
> > >> >> -	depends on !SMP
> > >> >>  	bool
> > >> >> -	default y
> > >> >> +	default !SMP || !MMU
> > >> >>  
> > >> > 
> > >> > Should this be `depends on !SMP || !MMU` with default yes? Because with
> > >> > SMP && MMU, it shouldn't be an option to run with percpu-km.
> > >> 
> > >> IIUC these are equivalent, truth table would not change if is under "depends"
> > >> or "default"
> > >> 
> > >> SMP    MMU   NEED_PER_CPU_KM
> > >>  y      y    !y || !y => n || n => n
> > >>  y      n    !y || !n => n || y => y
> > >>  n      y    !n || !y => y || n => y
> > >>  n      n    !n || !n => y || y => y
> > >> 
> > > 
> > > I may be wrong, but I think this is slightly different as we're using
> > > #ifdef / #if defined().
> > > 
> > >> > 
> > >> >>  config CLEANCACHE
> > >> >>  	bool "Enable cleancache driver to cache clean pages if tmem is present"
> > >> >> -- 
> > >> >> 2.7.4
> > >> >>
> > >> > 
> > >> > It's interesting to me that this is all coming up at once. Earlier this
> > >> > month I had the same conversation with people involved with sh [1].
> > >> > 
> > >> > [1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/
> > >> > 
> > >> > I can pull this shortly once I see whatever happened to linux-sh.
> > >> 
> > >> Ahh, good to know! Adding SH folks here (start of discussion [0]). I see you came
> > >> to the same conclusion, right? 
> > > 
> > > Yeah, I don't see anything else from linux-sh. So I'll go ahead and
> > > apply this with my change if you're fine with that.
> > 
> > I can't test against current until I get some unrelated fixes from Rich Felker
> > (who's been busy over the weekend), but I tested the "depends" version on 5.10
> > and got a shell prompt on my "make ARCH=sh j2_defconfig" board.
> > 
> > Tested-by: Rob Landley <rob@landley.net>
> 
> Tested-by: Rich Felker <dalias@libc.org>
> 
> I've tested the version as originally posted and it works for j2 (sh2)
> (both build and runtime checked). I'd tested the other (depends
> version) before and I'm fine with either going upstream.
> 
> Rich
> 

Thank you all for testing and bearing with me. I've applied this to
for-5.16-fixes. Will send it up later in the week.

Thanks,
Dennis

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Geert Uytterhoeven]

Hi Vladimir and Dennis,

On Wed, Dec 1, 2021 at 12:53 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> On 11/30/21 5:41 PM, Dennis Zhou wrote:
> > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> >> Currently, NOMMU pull km allocator via !SMP dependency because most of
> >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> >>
> >> * may lead to broken build [1]
> >> * ...or not working runtime due to [2]
> >>
> >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> >> use percpu allocator on UP too") so restore that.
> >>
> >> [1]
> >> For ARM SMP+NOMMU (R-class cores)
> >>
> >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> >>
> >> [2]
> >> static inline
> >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> >> {
> >>        return -EINVAL;
> >> }
> >>
> >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> >> ---
> >>  mm/Kconfig | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/mm/Kconfig b/mm/Kconfig
> >> index d16ba92..66331e0 100644
> >> --- a/mm/Kconfig
> >> +++ b/mm/Kconfig
> >> @@ -425,9 +425,8 @@ config THP_SWAP
> >>  # UP and nommu archs use km based percpu allocator
> >>  #
> >>  config NEED_PER_CPU_KM
> >> -    depends on !SMP
> >>      bool
> >> -    default y
> >> +    default !SMP || !MMU
> >>
> >
> > Should this be `depends on !SMP || !MMU` with default yes? Because with
> > SMP && MMU, it shouldn't be an option to run with percpu-km.
>
> IIUC these are equivalent, truth table would not change if is under "depends"
> or "default"
>
> SMP    MMU   NEED_PER_CPU_KM
>  y      y    !y || !y => n || n => n
>  y      n    !y || !n => n || y => y
>  n      y    !n || !y => y || n => y
>  n      n    !n || !n => y || y => y
>
> >
> >>  config CLEANCACHE
> >>      bool "Enable cleancache driver to cache clean pages if tmem is present"
> >> --
> >> 2.7.4
> >>
> >
> > It's interesting to me that this is all coming up at once. Earlier this
> > month I had the same conversation with people involved with sh [1].
> >
> > [1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/
> >
> > I can pull this shortly once I see whatever happened to linux-sh.
>
> Ahh, good to know! Adding SH folks here (start of discussion [0]). I see you came
> to the same conclusion, right?
>
> IIRC, RISC-V also have SMP+NOMMU, so adding them as well.

I had seen the j-Core thread, but completely forgot about
Canaan K210 (RV64 SMP+NOMMU).

This became commit 3583521aabac76e5 ("percpu: km: ensure it is used
with NOMMU (either UP or SMP)").  And now booting K210 prints:

    percpu: wasting 10 pages per chunk

a) Is this bad?
b) What exactly was this fixing, and how would I trigger the bad case
   on K210 before, if it was affected at all?

>
> [0] https://lore.kernel.org/linux-mm/20211130172954.129587-1-vladimir.murzin@arm.com/T/

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Dennis Zhou]

Hello,

On Tue, Dec 14, 2021 at 05:29:22PM +0100, Geert Uytterhoeven wrote:
> Hi Vladimir and Dennis,
> 
> On Wed, Dec 1, 2021 at 12:53 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> > On 11/30/21 5:41 PM, Dennis Zhou wrote:
> > > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> > >> Currently, NOMMU pull km allocator via !SMP dependency because most of
> > >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> > >>
> > >> * may lead to broken build [1]
> > >> * ...or not working runtime due to [2]
> > >>
> > >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> > >> use percpu allocator on UP too") so restore that.
> > >>
> > >> [1]
> > >> For ARM SMP+NOMMU (R-class cores)
> > >>
> > >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> > >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> > >>
> > >> [2]
> > >> static inline
> > >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> > >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> > >> {
> > >>        return -EINVAL;
> > >> }
> > >>
> > >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> > >> ---
> > >>  mm/Kconfig | 3 +--
> > >>  1 file changed, 1 insertion(+), 2 deletions(-)
> > >>
> > >> diff --git a/mm/Kconfig b/mm/Kconfig
> > >> index d16ba92..66331e0 100644
> > >> --- a/mm/Kconfig
> > >> +++ b/mm/Kconfig
> > >> @@ -425,9 +425,8 @@ config THP_SWAP
> > >>  # UP and nommu archs use km based percpu allocator
> > >>  #
> > >>  config NEED_PER_CPU_KM
> > >> -    depends on !SMP
> > >>      bool
> > >> -    default y
> > >> +    default !SMP || !MMU
> > >>
> > >
> > > Should this be `depends on !SMP || !MMU` with default yes? Because with
> > > SMP && MMU, it shouldn't be an option to run with percpu-km.
> >
> > IIUC these are equivalent, truth table would not change if is under "depends"
> > or "default"
> >
> > SMP    MMU   NEED_PER_CPU_KM
> >  y      y    !y || !y => n || n => n
> >  y      n    !y || !n => n || y => y
> >  n      y    !n || !y => y || n => y
> >  n      n    !n || !n => y || y => y
> >
> > >
> > >>  config CLEANCACHE
> > >>      bool "Enable cleancache driver to cache clean pages if tmem is present"
> > >> --
> > >> 2.7.4
> > >>
> > >
> > > It's interesting to me that this is all coming up at once. Earlier this
> > > month I had the same conversation with people involved with sh [1].
> > >
> > > [1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/
> > >
> > > I can pull this shortly once I see whatever happened to linux-sh.
> >
> > Ahh, good to know! Adding SH folks here (start of discussion [0]). I see you came
> > to the same conclusion, right?
> >
> > IIRC, RISC-V also have SMP+NOMMU, so adding them as well.
> 
> I had seen the j-Core thread, but completely forgot about
> Canaan K210 (RV64 SMP+NOMMU).
> 
> This became commit 3583521aabac76e5 ("percpu: km: ensure it is used
> with NOMMU (either UP or SMP)").  And now booting K210 prints:
> 
>     percpu: wasting 10 pages per chunk
> 
> a) Is this bad?

It's not great.. Can you share the line on boot with the following
prefix: pcpu-alloc [1].

I'm a little surprised here because this means it's allocating not
against the right atomic size. I don't necesarily think it's an issue of
switching from percpu-vm to percpu-km.

> b) What exactly was this fixing, and how would I trigger the bad case
>    on K210 before, if it was affected at all?
> 

In v5.14, I merged Roman's request for percpu depopulation [2]. This
required calls to flush the tlb. There is an abstraction layer:
percpu-vm vs percpu-km. So if an architecture is using percpu-vm but
doesn't have an MMU AND doesn't map out appropriately the tlb flush
call, then it fails. This happened on arm + sh architectures. Now RV
might be mapping it out appropriately so they never saw the issue.

Now, there is also a bigger caveat with using percpu-vm without an MMU.
In percpu-vm, we allocate pages on demand and map them in into
pre-allocated vmas. This means there are 2 scenarios that I haven't
looked into deeper. 1, the vma alloc maps to allocating physical pages.
2, we're lucky percpu allocates backwards in the vma so we haven't had a
collision problem yet.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/dennis/percpu.git/tree/mm/percpu.c#n2507
[2] https://lore.kernel.org/lkml/20210419225047.3415425-1-dennis@kernel.org/

Thanks,
Dennis

> >
> > [0] https://lore.kernel.org/linux-mm/20211130172954.129587-1-vladimir.murzin@arm.com/T/
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Geert Uytterhoeven]

Hi Dennis,

On Tue, Dec 14, 2021 at 6:26 PM Dennis Zhou <dennis@kernel.org> wrote:
> On Tue, Dec 14, 2021 at 05:29:22PM +0100, Geert Uytterhoeven wrote:
> > On Wed, Dec 1, 2021 at 12:53 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> > > On 11/30/21 5:41 PM, Dennis Zhou wrote:
> > > > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> > > >> Currently, NOMMU pull km allocator via !SMP dependency because most of
> > > >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> > > >>
> > > >> * may lead to broken build [1]
> > > >> * ...or not working runtime due to [2]
> > > >>
> > > >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> > > >> use percpu allocator on UP too") so restore that.
> > > >>
> > > >> [1]
> > > >> For ARM SMP+NOMMU (R-class cores)
> > > >>
> > > >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> > > >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> > > >>
> > > >> [2]
> > > >> static inline
> > > >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> > > >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> > > >> {
> > > >>        return -EINVAL;
> > > >> }
> > > >>
> > > >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> > > >> ---
> > > >>  mm/Kconfig | 3 +--
> > > >>  1 file changed, 1 insertion(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/mm/Kconfig b/mm/Kconfig
> > > >> index d16ba92..66331e0 100644
> > > >> --- a/mm/Kconfig
> > > >> +++ b/mm/Kconfig
> > > >> @@ -425,9 +425,8 @@ config THP_SWAP
> > > >>  # UP and nommu archs use km based percpu allocator
> > > >>  #
> > > >>  config NEED_PER_CPU_KM
> > > >> -    depends on !SMP
> > > >>      bool
> > > >> -    default y
> > > >> +    default !SMP || !MMU
> > > >>
> > > >
> > > > Should this be `depends on !SMP || !MMU` with default yes? Because with
> > > > SMP && MMU, it shouldn't be an option to run with percpu-km.
> > >
> > > IIUC these are equivalent, truth table would not change if is under "depends"
> > > or "default"
> > >
> > > SMP    MMU   NEED_PER_CPU_KM
> > >  y      y    !y || !y => n || n => n
> > >  y      n    !y || !n => n || y => y
> > >  n      y    !n || !y => y || n => y
> > >  n      n    !n || !n => y || y => y
> > >
> > > >
> > > >>  config CLEANCACHE
> > > >>      bool "Enable cleancache driver to cache clean pages if tmem is present"
> > > >> --
> > > >> 2.7.4
> > > >>
> > > >
> > > > It's interesting to me that this is all coming up at once. Earlier this
> > > > month I had the same conversation with people involved with sh [1].
> > > >
> > > > [1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/
> > > >
> > > > I can pull this shortly once I see whatever happened to linux-sh.
> > >
> > > Ahh, good to know! Adding SH folks here (start of discussion [0]). I see you came
> > > to the same conclusion, right?
> > >
> > > IIRC, RISC-V also have SMP+NOMMU, so adding them as well.
> >
> > I had seen the j-Core thread, but completely forgot about
> > Canaan K210 (RV64 SMP+NOMMU).
> >
> > This became commit 3583521aabac76e5 ("percpu: km: ensure it is used
> > with NOMMU (either UP or SMP)").  And now booting K210 prints:
> >
> >     percpu: wasting 10 pages per chunk
> >
> > a) Is this bad?
>
> It's not great.. Can you share the line on boot with the following
> prefix: pcpu-alloc [1].

There are no such lines.
"make mm/percpu.i mm/percpu.s" and inspecting the generated files,
and vmlinux, proves the code is there. But apparently it's not called.

So there may be no issue on my system?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Dennis Zhou]

On Tue, Dec 14, 2021 at 08:02:58PM +0100, Geert Uytterhoeven wrote:
> Hi Dennis,
> 
> On Tue, Dec 14, 2021 at 6:26 PM Dennis Zhou <dennis@kernel.org> wrote:
> > On Tue, Dec 14, 2021 at 05:29:22PM +0100, Geert Uytterhoeven wrote:
> > > On Wed, Dec 1, 2021 at 12:53 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> > > > On 11/30/21 5:41 PM, Dennis Zhou wrote:
> > > > > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> > > > >> Currently, NOMMU pull km allocator via !SMP dependency because most of
> > > > >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> > > > >>
> > > > >> * may lead to broken build [1]
> > > > >> * ...or not working runtime due to [2]
> > > > >>
> > > > >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> > > > >> use percpu allocator on UP too") so restore that.
> > > > >>
> > > > >> [1]
> > > > >> For ARM SMP+NOMMU (R-class cores)
> > > > >>
> > > > >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> > > > >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> > > > >>
> > > > >> [2]
> > > > >> static inline
> > > > >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> > > > >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> > > > >> {
> > > > >>        return -EINVAL;
> > > > >> }
> > > > >>
> > > > >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> > > > >> ---
> > > > >>  mm/Kconfig | 3 +--
> > > > >>  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > >>
> > > > >> diff --git a/mm/Kconfig b/mm/Kconfig
> > > > >> index d16ba92..66331e0 100644
> > > > >> --- a/mm/Kconfig
> > > > >> +++ b/mm/Kconfig
> > > > >> @@ -425,9 +425,8 @@ config THP_SWAP
> > > > >>  # UP and nommu archs use km based percpu allocator
> > > > >>  #
> > > > >>  config NEED_PER_CPU_KM
> > > > >> -    depends on !SMP
> > > > >>      bool
> > > > >> -    default y
> > > > >> +    default !SMP || !MMU
> > > > >>
> > > > >
> > > > > Should this be `depends on !SMP || !MMU` with default yes? Because with
> > > > > SMP && MMU, it shouldn't be an option to run with percpu-km.
> > > >
> > > > IIUC these are equivalent, truth table would not change if is under "depends"
> > > > or "default"
> > > >
> > > > SMP    MMU   NEED_PER_CPU_KM
> > > >  y      y    !y || !y => n || n => n
> > > >  y      n    !y || !n => n || y => y
> > > >  n      y    !n || !y => y || n => y
> > > >  n      n    !n || !n => y || y => y
> > > >
> > > > >
> > > > >>  config CLEANCACHE
> > > > >>      bool "Enable cleancache driver to cache clean pages if tmem is present"
> > > > >> --
> > > > >> 2.7.4
> > > > >>
> > > > >
> > > > > It's interesting to me that this is all coming up at once. Earlier this
> > > > > month I had the same conversation with people involved with sh [1].
> > > > >
> > > > > [1] https://lore.kernel.org/linux-sh/YY7tp5attRyK42Zk@fedora/
> > > > >
> > > > > I can pull this shortly once I see whatever happened to linux-sh.
> > > >
> > > > Ahh, good to know! Adding SH folks here (start of discussion [0]). I see you came
> > > > to the same conclusion, right?
> > > >
> > > > IIRC, RISC-V also have SMP+NOMMU, so adding them as well.
> > >
> > > I had seen the j-Core thread, but completely forgot about
> > > Canaan K210 (RV64 SMP+NOMMU).
> > >
> > > This became commit 3583521aabac76e5 ("percpu: km: ensure it is used
> > > with NOMMU (either UP or SMP)").  And now booting K210 prints:
> > >
> > >     percpu: wasting 10 pages per chunk
> > >
> > > a) Is this bad?
> >
> > It's not great.. Can you share the line on boot with the following
> > prefix: pcpu-alloc [1].
> 
> There are no such lines.
> "make mm/percpu.i mm/percpu.s" and inspecting the generated files,
> and vmlinux, proves the code is there. But apparently it's not called.
> 
> So there may be no issue on my system?
> 

I might be missing something, but that can't be right. Percpu calls
pcpu_dump_alloc_info() from pcpu_setup_first_chunk() which is called by
both embed/page first chunk code.

Ummm. That can't be right. Percpu call pcpu_dump_alloc_info() from
pcpu_setup_first_chunk() which everyone should call. On my machine:

$ dmesg | grep "pcpu-alloc"
[    0.065118] pcpu-alloc: s184320 r8192 d28672 u262144 alloc=1*2097152

Thanks,
Dennis

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Geert Uytterhoeven]

Hi Dennis,

On Tue, Dec 14, 2021 at 8:18 PM Dennis Zhou <dennis@kernel.org> wrote:
> On Tue, Dec 14, 2021 at 08:02:58PM +0100, Geert Uytterhoeven wrote:
> > On Tue, Dec 14, 2021 at 6:26 PM Dennis Zhou <dennis@kernel.org> wrote:
> > > On Tue, Dec 14, 2021 at 05:29:22PM +0100, Geert Uytterhoeven wrote:
> > > > On Wed, Dec 1, 2021 at 12:53 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> > > > > On 11/30/21 5:41 PM, Dennis Zhou wrote:
> > > > > > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> > > > > >> Currently, NOMMU pull km allocator via !SMP dependency because most of
> > > > > >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> > > > > >>
> > > > > >> * may lead to broken build [1]
> > > > > >> * ...or not working runtime due to [2]
> > > > > >>
> > > > > >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> > > > > >> use percpu allocator on UP too") so restore that.
> > > > > >>
> > > > > >> [1]
> > > > > >> For ARM SMP+NOMMU (R-class cores)
> > > > > >>
> > > > > >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> > > > > >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> > > > > >>
> > > > > >> [2]
> > > > > >> static inline
> > > > > >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> > > > > >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> > > > > >> {
> > > > > >>        return -EINVAL;
> > > > > >> }
> > > > > >>
> > > > > >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>

> > > > > IIRC, RISC-V also have SMP+NOMMU, so adding them as well.
> > > >
> > > > I had seen the j-Core thread, but completely forgot about
> > > > Canaan K210 (RV64 SMP+NOMMU).
> > > >
> > > > This became commit 3583521aabac76e5 ("percpu: km: ensure it is used
> > > > with NOMMU (either UP or SMP)").  And now booting K210 prints:
> > > >
> > > >     percpu: wasting 10 pages per chunk
> > > >
> > > > a) Is this bad?
> > >
> > > It's not great.. Can you share the line on boot with the following
> > > prefix: pcpu-alloc [1].
> >
> > There are no such lines.
> > "make mm/percpu.i mm/percpu.s" and inspecting the generated files,
> > and vmlinux, proves the code is there. But apparently it's not called.
> >
> > So there may be no issue on my system?
>
> I might be missing something, but that can't be right. Percpu calls
> pcpu_dump_alloc_info() from pcpu_setup_first_chunk() which is called by
> both embed/page first chunk code.
>
> Ummm. That can't be right. Percpu call pcpu_dump_alloc_info() from
> pcpu_setup_first_chunk() which everyone should call. On my machine:
>
> $ dmesg | grep "pcpu-alloc"
> [    0.065118] pcpu-alloc: s184320 r8192 d28672 u262144 alloc=1*2097152

Doh, it wasn't printed to the console, due to KERN_DEBUG. Dmesg
does have it:

<7>[    0.000000] pcpu-alloc: s15520 r0 d29536 u45056 alloc=11*4096
<7>[    0.000000] pcpu-alloc: [0] 0 [0] 1

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Dennis Zhou]

On Tue, Dec 14, 2021 at 09:12:06PM +0100, Geert Uytterhoeven wrote:
> Hi Dennis,
> 
> On Tue, Dec 14, 2021 at 8:18 PM Dennis Zhou <dennis@kernel.org> wrote:
> > On Tue, Dec 14, 2021 at 08:02:58PM +0100, Geert Uytterhoeven wrote:
> > > On Tue, Dec 14, 2021 at 6:26 PM Dennis Zhou <dennis@kernel.org> wrote:
> > > > On Tue, Dec 14, 2021 at 05:29:22PM +0100, Geert Uytterhoeven wrote:
> > > > > On Wed, Dec 1, 2021 at 12:53 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> > > > > > On 11/30/21 5:41 PM, Dennis Zhou wrote:
> > > > > > > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> > > > > > >> Currently, NOMMU pull km allocator via !SMP dependency because most of
> > > > > > >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> > > > > > >>
> > > > > > >> * may lead to broken build [1]
> > > > > > >> * ...or not working runtime due to [2]
> > > > > > >>
> > > > > > >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> > > > > > >> use percpu allocator on UP too") so restore that.
> > > > > > >>
> > > > > > >> [1]
> > > > > > >> For ARM SMP+NOMMU (R-class cores)
> > > > > > >>
> > > > > > >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> > > > > > >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> > > > > > >>
> > > > > > >> [2]
> > > > > > >> static inline
> > > > > > >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> > > > > > >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> > > > > > >> {
> > > > > > >>        return -EINVAL;
> > > > > > >> }
> > > > > > >>
> > > > > > >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> 
> > > > > > IIRC, RISC-V also have SMP+NOMMU, so adding them as well.
> > > > >
> > > > > I had seen the j-Core thread, but completely forgot about
> > > > > Canaan K210 (RV64 SMP+NOMMU).
> > > > >
> > > > > This became commit 3583521aabac76e5 ("percpu: km: ensure it is used
> > > > > with NOMMU (either UP or SMP)").  And now booting K210 prints:
> > > > >
> > > > >     percpu: wasting 10 pages per chunk
> > > > >
> > > > > a) Is this bad?
> > > >
> > > > It's not great.. Can you share the line on boot with the following
> > > > prefix: pcpu-alloc [1].
> > >
> > > There are no such lines.
> > > "make mm/percpu.i mm/percpu.s" and inspecting the generated files,
> > > and vmlinux, proves the code is there. But apparently it's not called.
> > >
> > > So there may be no issue on my system?
> >
> > I might be missing something, but that can't be right. Percpu calls
> > pcpu_dump_alloc_info() from pcpu_setup_first_chunk() which is called by
> > both embed/page first chunk code.
> >
> > Ummm. That can't be right. Percpu call pcpu_dump_alloc_info() from
> > pcpu_setup_first_chunk() which everyone should call. On my machine:
> >
> > $ dmesg | grep "pcpu-alloc"
> > [    0.065118] pcpu-alloc: s184320 r8192 d28672 u262144 alloc=1*2097152
> 
> Doh, it wasn't printed to the console, due to KERN_DEBUG. Dmesg
> does have it:
> 
> <7>[    0.000000] pcpu-alloc: s15520 r0 d29536 u45056 alloc=11*4096
> <7>[    0.000000] pcpu-alloc: [0] 0 [0] 1
> 

I see, so what's happening is we're allocating 11 pages * 2, and due to
percpu-km we round up to a contiguous 32 pages for backing pages. This
results in the warning of wasting 10 pages. Given the size of the static
region, I'm not too worried for now. I can't imagine the config would
use that much percpu memory.

We can massage the discrepancy for-v5.17. Basically in percpu-km, we
align to 4k even though our allocation gets rounded up to the next power
of 2. I don't have a lot of bandwidth right now, but I might be able to
think about it over the next few weeks.

Thanks,
Dennis

Re: [PATCH] percpu: km: ensure it is used with NOMMU (either UP or SMP)

[Geert Uytterhoeven]

Hi Dennis,

On Tue, Dec 14, 2021 at 9:50 PM Dennis Zhou <dennis@kernel.org> wrote:
> On Tue, Dec 14, 2021 at 09:12:06PM +0100, Geert Uytterhoeven wrote:
> > On Tue, Dec 14, 2021 at 8:18 PM Dennis Zhou <dennis@kernel.org> wrote:
> > > On Tue, Dec 14, 2021 at 08:02:58PM +0100, Geert Uytterhoeven wrote:
> > > > On Tue, Dec 14, 2021 at 6:26 PM Dennis Zhou <dennis@kernel.org> wrote:
> > > > > On Tue, Dec 14, 2021 at 05:29:22PM +0100, Geert Uytterhoeven wrote:
> > > > > > On Wed, Dec 1, 2021 at 12:53 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> > > > > > > On 11/30/21 5:41 PM, Dennis Zhou wrote:
> > > > > > > > On Tue, Nov 30, 2021 at 05:29:54PM +0000, Vladimir Murzin wrote:
> > > > > > > >> Currently, NOMMU pull km allocator via !SMP dependency because most of
> > > > > > > >> them are UP, yet for SMP+NOMMU vm allocator gets pulled which:
> > > > > > > >>
> > > > > > > >> * may lead to broken build [1]
> > > > > > > >> * ...or not working runtime due to [2]
> > > > > > > >>
> > > > > > > >> It looks like SMP+NOMMU case was overlooked in bbddff054587 ("percpu:
> > > > > > > >> use percpu allocator on UP too") so restore that.
> > > > > > > >>
> > > > > > > >> [1]
> > > > > > > >> For ARM SMP+NOMMU (R-class cores)
> > > > > > > >>
> > > > > > > >> arm-none-linux-gnueabihf-ld: mm/percpu.o: in function `pcpu_post_unmap_tlb_flush':
> > > > > > > >> mm/percpu-vm.c:188: undefined reference to `flush_tlb_kernel_range'
> > > > > > > >>
> > > > > > > >> [2]
> > > > > > > >> static inline
> > > > > > > >> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> > > > > > > >>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
> > > > > > > >> {
> > > > > > > >>        return -EINVAL;
> > > > > > > >> }
> > > > > > > >>
> > > > > > > >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> >
> > > > > > > IIRC, RISC-V also have SMP+NOMMU, so adding them as well.
> > > > > >
> > > > > > I had seen the j-Core thread, but completely forgot about
> > > > > > Canaan K210 (RV64 SMP+NOMMU).
> > > > > >
> > > > > > This became commit 3583521aabac76e5 ("percpu: km: ensure it is used
> > > > > > with NOMMU (either UP or SMP)").  And now booting K210 prints:
> > > > > >
> > > > > >     percpu: wasting 10 pages per chunk
> > > > > >
> > > > > > a) Is this bad?
> > > > >
> > > > > It's not great.. Can you share the line on boot with the following
> > > > > prefix: pcpu-alloc [1].
> > > >
> > > > There are no such lines.
> > > > "make mm/percpu.i mm/percpu.s" and inspecting the generated files,
> > > > and vmlinux, proves the code is there. But apparently it's not called.
> > > >
> > > > So there may be no issue on my system?
> > >
> > > I might be missing something, but that can't be right. Percpu calls
> > > pcpu_dump_alloc_info() from pcpu_setup_first_chunk() which is called by
> > > both embed/page first chunk code.
> > >
> > > Ummm. That can't be right. Percpu call pcpu_dump_alloc_info() from
> > > pcpu_setup_first_chunk() which everyone should call. On my machine:
> > >
> > > $ dmesg | grep "pcpu-alloc"
> > > [    0.065118] pcpu-alloc: s184320 r8192 d28672 u262144 alloc=1*2097152
> >
> > Doh, it wasn't printed to the console, due to KERN_DEBUG. Dmesg
> > does have it:
> >
> > <7>[    0.000000] pcpu-alloc: s15520 r0 d29536 u45056 alloc=11*4096
> > <7>[    0.000000] pcpu-alloc: [0] 0 [0] 1
> >
>
> I see, so what's happening is we're allocating 11 pages * 2, and due to
> percpu-km we round up to a contiguous 32 pages for backing pages. This
> results in the warning of wasting 10 pages. Given the size of the static
> region, I'm not too worried for now. I can't imagine the config would
> use that much percpu memory.
>
> We can massage the discrepancy for-v5.17. Basically in percpu-km, we
> align to 4k even though our allocation gets rounded up to the next power
> of 2. I don't have a lot of bandwidth right now, but I might be able to
> think about it over the next few weeks.

Note that K210 has only 8 MiB of SRAM, so wasting 10 pages means
wasting 0.5% of RAM.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds