[v6,07/15] mm: defining memblock_virt_alloc_try_nid_raw

A new variant of memblock_virt_alloc_* allocations:
memblock_virt_alloc_try_nid_raw()
    - Does not zero the allocated memory
    - Does not panic if request cannot be satisfied

Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <bob.picco@oracle.com>
---
 include/linux/bootmem.h | 27 +++++++++++++++++++++++++
 mm/memblock.c           | 53 ++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 73 insertions(+), 7 deletions(-)

On Mon 07-08-17 16:38:41, Pavel Tatashin wrote:
> A new variant of memblock_virt_alloc_* allocations:
> memblock_virt_alloc_try_nid_raw()
>     - Does not zero the allocated memory
>     - Does not panic if request cannot be satisfied

OK, this looks good but I would not introduce memblock_virt_alloc_raw
here because we do not have any users. Please move that to "mm: optimize
early system hash allocations" which actually uses the API. It would be
easier to review it that way.

> Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
> Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
> Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> Reviewed-by: Bob Picco <bob.picco@oracle.com>

other than that
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  include/linux/bootmem.h | 27 +++++++++++++++++++++++++
>  mm/memblock.c           | 53 ++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 73 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/bootmem.h b/include/linux/bootmem.h
> index e223d91b6439..ea30b3987282 100644
> --- a/include/linux/bootmem.h
> +++ b/include/linux/bootmem.h
> @@ -160,6 +160,9 @@ extern void *__alloc_bootmem_low_node(pg_data_t *pgdat,
>  #define BOOTMEM_ALLOC_ANYWHERE		(~(phys_addr_t)0)
>  
>  /* FIXME: Move to memblock.h at a point where we remove nobootmem.c */
> +void *memblock_virt_alloc_try_nid_raw(phys_addr_t size, phys_addr_t align,
> +				      phys_addr_t min_addr,
> +				      phys_addr_t max_addr, int nid);
>  void *memblock_virt_alloc_try_nid_nopanic(phys_addr_t size,
>  		phys_addr_t align, phys_addr_t min_addr,
>  		phys_addr_t max_addr, int nid);
> @@ -176,6 +179,14 @@ static inline void * __init memblock_virt_alloc(
>  					    NUMA_NO_NODE);
>  }
>  
> +static inline void * __init memblock_virt_alloc_raw(
> +					phys_addr_t size,  phys_addr_t align)
> +{
> +	return memblock_virt_alloc_try_nid_raw(size, align, BOOTMEM_LOW_LIMIT,
> +					    BOOTMEM_ALLOC_ACCESSIBLE,
> +					    NUMA_NO_NODE);
> +}
> +
>  static inline void * __init memblock_virt_alloc_nopanic(
>  					phys_addr_t size, phys_addr_t align)
>  {
> @@ -257,6 +268,14 @@ static inline void * __init memblock_virt_alloc(
>  	return __alloc_bootmem(size, align, BOOTMEM_LOW_LIMIT);
>  }
>  
> +static inline void * __init memblock_virt_alloc_raw(
> +					phys_addr_t size,  phys_addr_t align)
> +{
> +	if (!align)
> +		align = SMP_CACHE_BYTES;
> +	return __alloc_bootmem_nopanic(size, align, BOOTMEM_LOW_LIMIT);
> +}
> +
>  static inline void * __init memblock_virt_alloc_nopanic(
>  					phys_addr_t size, phys_addr_t align)
>  {
> @@ -309,6 +328,14 @@ static inline void * __init memblock_virt_alloc_try_nid(phys_addr_t size,
>  					  min_addr);
>  }
>  
> +static inline void * __init memblock_virt_alloc_try_nid_raw(
> +			phys_addr_t size, phys_addr_t align,
> +			phys_addr_t min_addr, phys_addr_t max_addr, int nid)
> +{
> +	return ___alloc_bootmem_node_nopanic(NODE_DATA(nid), size, align,
> +				min_addr, max_addr);
> +}
> +
>  static inline void * __init memblock_virt_alloc_try_nid_nopanic(
>  			phys_addr_t size, phys_addr_t align,
>  			phys_addr_t min_addr, phys_addr_t max_addr, int nid)
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 08f449acfdd1..3fbf3bcb52d9 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1327,7 +1327,6 @@ static void * __init memblock_virt_alloc_internal(
>  	return NULL;
>  done:
>  	ptr = phys_to_virt(alloc);
> -	memset(ptr, 0, size);
>  
>  	/*
>  	 * The min_count is set to 0 so that bootmem allocated blocks
> @@ -1340,6 +1339,38 @@ static void * __init memblock_virt_alloc_internal(
>  	return ptr;
>  }
>  
> +/**
> + * memblock_virt_alloc_try_nid_raw - allocate boot memory block without zeroing
> + * memory and without panicking
> + * @size: size of memory block to be allocated in bytes
> + * @align: alignment of the region and block's size
> + * @min_addr: the lower bound of the memory region from where the allocation
> + *	  is preferred (phys address)
> + * @max_addr: the upper bound of the memory region from where the allocation
> + *	      is preferred (phys address), or %BOOTMEM_ALLOC_ACCESSIBLE to
> + *	      allocate only from memory limited by memblock.current_limit value
> + * @nid: nid of the free area to find, %NUMA_NO_NODE for any node
> + *
> + * Public function, provides additional debug information (including caller
> + * info), if enabled. Does not zero allocated memory, does not panic if request
> + * cannot be satisfied.
> + *
> + * RETURNS:
> + * Virtual address of allocated memory block on success, NULL on failure.
> + */
> +void * __init memblock_virt_alloc_try_nid_raw(
> +			phys_addr_t size, phys_addr_t align,
> +			phys_addr_t min_addr, phys_addr_t max_addr,
> +			int nid)
> +{
> +	memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n",
> +		     __func__, (u64)size, (u64)align, nid, (u64)min_addr,
> +		     (u64)max_addr, (void *)_RET_IP_);
> +
> +	return memblock_virt_alloc_internal(size, align,
> +					    min_addr, max_addr, nid);
> +}
> +
>  /**
>   * memblock_virt_alloc_try_nid_nopanic - allocate boot memory block
>   * @size: size of memory block to be allocated in bytes
> @@ -1351,8 +1382,8 @@ static void * __init memblock_virt_alloc_internal(
>   *	      allocate only from memory limited by memblock.current_limit value
>   * @nid: nid of the free area to find, %NUMA_NO_NODE for any node
>   *
> - * Public version of _memblock_virt_alloc_try_nid_nopanic() which provides
> - * additional debug information (including caller info), if enabled.
> + * Public function, provides additional debug information (including caller
> + * info), if enabled. This function zeroes the allocated memory.
>   *
>   * RETURNS:
>   * Virtual address of allocated memory block on success, NULL on failure.
> @@ -1362,11 +1393,17 @@ void * __init memblock_virt_alloc_try_nid_nopanic(
>  				phys_addr_t min_addr, phys_addr_t max_addr,
>  				int nid)
>  {
> +	void *ptr;
> +
>  	memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n",
>  		     __func__, (u64)size, (u64)align, nid, (u64)min_addr,
>  		     (u64)max_addr, (void *)_RET_IP_);
> -	return memblock_virt_alloc_internal(size, align, min_addr,
> -					     max_addr, nid);
> +
> +	ptr = memblock_virt_alloc_internal(size, align,
> +					   min_addr, max_addr, nid);
> +	if (ptr)
> +		memset(ptr, 0, size);
> +	return ptr;
>  }
>  
>  /**
> @@ -1380,7 +1417,7 @@ void * __init memblock_virt_alloc_try_nid_nopanic(
>   *	      allocate only from memory limited by memblock.current_limit value
>   * @nid: nid of the free area to find, %NUMA_NO_NODE for any node
>   *
> - * Public panicking version of _memblock_virt_alloc_try_nid_nopanic()
> + * Public panicking version of memblock_virt_alloc_try_nid_nopanic()
>   * which provides debug information (including caller info), if enabled,
>   * and panics if the request can not be satisfied.
>   *
> @@ -1399,8 +1436,10 @@ void * __init memblock_virt_alloc_try_nid(
>  		     (u64)max_addr, (void *)_RET_IP_);
>  	ptr = memblock_virt_alloc_internal(size, align,
>  					   min_addr, max_addr, nid);
> -	if (ptr)
> +	if (ptr) {
> +		memset(ptr, 0, size);
>  		return ptr;
> +	}
>  
>  	panic("%s: Failed to allocate %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx\n",
>  	      __func__, (u64)size, (u64)align, nid, (u64)min_addr,
> -- 
> 2.14.0

On 08/11/2017 08:39 AM, Michal Hocko wrote:
> On Mon 07-08-17 16:38:41, Pavel Tatashin wrote:
>> A new variant of memblock_virt_alloc_* allocations:
>> memblock_virt_alloc_try_nid_raw()
>>      - Does not zero the allocated memory
>>      - Does not panic if request cannot be satisfied
> 
> OK, this looks good but I would not introduce memblock_virt_alloc_raw
> here because we do not have any users. Please move that to "mm: optimize
> early system hash allocations" which actually uses the API. It would be
> easier to review it that way.
> 
>> Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
>> Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
>> Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
>> Reviewed-by: Bob Picco <bob.picco@oracle.com>
> 
> other than that
> Acked-by: Michal Hocko <mhocko@suse.com>

Sure, I could do this, but as I understood from earlier Dave Miller's 
comments, we should do one logical change at a time. Hence, introduce 
API in one patch use it in another. So, this is how I tried to organize 
this patch set. Is this assumption incorrect?
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri 11-08-17 11:58:46, Pasha Tatashin wrote:
> On 08/11/2017 08:39 AM, Michal Hocko wrote:
> >On Mon 07-08-17 16:38:41, Pavel Tatashin wrote:
> >>A new variant of memblock_virt_alloc_* allocations:
> >>memblock_virt_alloc_try_nid_raw()
> >>     - Does not zero the allocated memory
> >>     - Does not panic if request cannot be satisfied
> >
> >OK, this looks good but I would not introduce memblock_virt_alloc_raw
> >here because we do not have any users. Please move that to "mm: optimize
> >early system hash allocations" which actually uses the API. It would be
> >easier to review it that way.
> >
> >>Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
> >>Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
> >>Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> >>Reviewed-by: Bob Picco <bob.picco@oracle.com>
> >
> >other than that
> >Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Sure, I could do this, but as I understood from earlier Dave Miller's
> comments, we should do one logical change at a time. Hence, introduce API in
> one patch use it in another. So, this is how I tried to organize this patch
> set. Is this assumption incorrect?

Well, it really depends. If the patch is really small then adding a new
API along with users is easier to review and backport because you have a
clear view of the usage. I believe this is the case here. But if others
feel otherwise I will not object.

>> Sure, I could do this, but as I understood from earlier Dave Miller's
>> comments, we should do one logical change at a time. Hence, introduce API in
>> one patch use it in another. So, this is how I tried to organize this patch
>> set. Is this assumption incorrect?
> 
> Well, it really depends. If the patch is really small then adding a new
> API along with users is easier to review and backport because you have a
> clear view of the usage. I believe this is the case here. But if others
> feel otherwise I will not object.

I will merge them.

Thank you,
Pasha
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html