2018-09-26 09:32:15

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH 14/30] memblock: add align parameter to memblock_alloc_node()

On Fri 14-09-18 15:10:29, Mike Rapoport wrote:
> With the align parameter memblock_alloc_node() can be used as drop in
> replacement for alloc_bootmem_pages_node() and __alloc_bootmem_node(),
> which is done in the following patches.

/me confused. Why do we need this patch at all? Maybe it should be
folded into the later patch you are refereing here?

> Signed-off-by: Mike Rapoport <[email protected]>
> ---
> include/linux/bootmem.h | 4 ++--
> mm/sparse.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bootmem.h b/include/linux/bootmem.h
> index 7d91f0f..3896af2 100644
> --- a/include/linux/bootmem.h
> +++ b/include/linux/bootmem.h
> @@ -157,9 +157,9 @@ static inline void * __init memblock_alloc_from_nopanic(
> }
>
> static inline void * __init memblock_alloc_node(
> - phys_addr_t size, int nid)
> + phys_addr_t size, phys_addr_t align, int nid)
> {
> - return memblock_alloc_try_nid(size, 0, BOOTMEM_LOW_LIMIT,
> + return memblock_alloc_try_nid(size, align, BOOTMEM_LOW_LIMIT,
> BOOTMEM_ALLOC_ACCESSIBLE, nid);
> }
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 04e97af..509828f 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -68,7 +68,7 @@ static noinline struct mem_section __ref *sparse_index_alloc(int nid)
> if (slab_is_available())
> section = kzalloc_node(array_size, GFP_KERNEL, nid);
> else
> - section = memblock_alloc_node(array_size, nid);
> + section = memblock_alloc_node(array_size, 0, nid);
>
> return section;
> }
> --
> 2.7.4
>

--
Michal Hocko
SUSE Labs


2018-09-26 09:39:11

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH 14/30] memblock: add align parameter to memblock_alloc_node()

On Wed 26-09-18 11:31:27, Michal Hocko wrote:
> On Fri 14-09-18 15:10:29, Mike Rapoport wrote:
> > With the align parameter memblock_alloc_node() can be used as drop in
> > replacement for alloc_bootmem_pages_node() and __alloc_bootmem_node(),
> > which is done in the following patches.
>
> /me confused. Why do we need this patch at all? Maybe it should be
> folded into the later patch you are refereing here?

OK, I can see [email protected]
now. If you are going to repost for whatever reason please merge those
two. Also I would get rid of the implicit "0 implies SMP_CACHE_BYTES"
behavior. It is subtle and you have to dig deep to find that out. Why
not make it explicit?
--
Michal Hocko
SUSE Labs

2018-09-26 13:46:02

by Mike Rapoport

[permalink] [raw]
Subject: Re: [PATCH 14/30] memblock: add align parameter to memblock_alloc_node()

On Wed, Sep 26, 2018 at 11:36:48AM +0200, Michal Hocko wrote:
> On Wed 26-09-18 11:31:27, Michal Hocko wrote:
> > On Fri 14-09-18 15:10:29, Mike Rapoport wrote:
> > > With the align parameter memblock_alloc_node() can be used as drop in
> > > replacement for alloc_bootmem_pages_node() and __alloc_bootmem_node(),
> > > which is done in the following patches.
> >
> > /me confused. Why do we need this patch at all? Maybe it should be
> > folded into the later patch you are refereing here?
>
> OK, I can see [email protected]
> now. If you are going to repost for whatever reason please merge those
> two. Also I would get rid of the implicit "0 implies SMP_CACHE_BYTES"
> behavior. It is subtle and you have to dig deep to find that out. Why
> not make it explicit?

Agree. I'd just prefer to make it a separate patch rather then resend the
whole series.

> --
> Michal Hocko
> SUSE Labs
>

--
Sincerely yours,
Mike.


2018-09-26 14:24:44

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH 14/30] memblock: add align parameter to memblock_alloc_node()

On Wed 26-09-18 16:43:35, Mike Rapoport wrote:
> On Wed, Sep 26, 2018 at 11:36:48AM +0200, Michal Hocko wrote:
> > On Wed 26-09-18 11:31:27, Michal Hocko wrote:
> > > On Fri 14-09-18 15:10:29, Mike Rapoport wrote:
> > > > With the align parameter memblock_alloc_node() can be used as drop in
> > > > replacement for alloc_bootmem_pages_node() and __alloc_bootmem_node(),
> > > > which is done in the following patches.
> > >
> > > /me confused. Why do we need this patch at all? Maybe it should be
> > > folded into the later patch you are refereing here?
> >
> > OK, I can see [email protected]
> > now. If you are going to repost for whatever reason please merge those
> > two. Also I would get rid of the implicit "0 implies SMP_CACHE_BYTES"
> > behavior. It is subtle and you have to dig deep to find that out. Why
> > not make it explicit?
>
> Agree. I'd just prefer to make it a separate patch rather then resend the
> whole series.

Sure, no objection from me.

--
Michal Hocko
SUSE Labs