Re: [PATCH v5 1/5] mm/sparse: abstract sparse buffer allocations
On Fri, 13 Jul 2018 09:24:44 -0400 Pavel Tatashin
wrote:
> On 07/13/2018 09:17 AM, Oscar Salvador wrote:
> > On Thu, Jul 12, 2018 at 04:37:26PM -0400, Pavel Tatashin wrote:
> >> +static void *sparsemap_buf __meminitdata;
> >> +static void *sparsemap_buf_end __meminitdata;
> >> +
> >> +void __init sparse_buffer_init(unsigned long size, int nid)
> >> +{
> >> + BUG_ON(sparsemap_buf);
> >
> > Why do we need a BUG_ON() here?
> > Looking at the code I cannot really see how we can end up with
> > sparsemap_buf being NULL.
> > Is it just for over-protection?
>
> This checks that we do not accidentally leak memory by calling
> sparse_buffer_init() consequently without sparse_buffer_fini() in-between.
A memory leak isn't serious enough to justify crashing the kernel.
Therefore
--- a/mm/sparse.c~mm-sparse-abstract-sparse-buffer-allocations-fix-fix
+++ a/mm/sparse.c
@@ -469,7 +469,7 @@ static void *sparsemap_buf_end __meminit
void __init sparse_buffer_init(unsigned long size, int nid)
{
- BUG_ON(sparsemap_buf);
+ WARN_ON(sparsemap_buf); /* forgot to call sparse_buffer_fini()? */
sparsemap_buf =
memblock_virt_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
_
Re: [PATCH v5 1/5] mm/sparse: abstract sparse buffer allocations
On Fri, 13 Jul 2018 09:24:44 -0400 Pavel Tatashin
wrote:
> On 07/13/2018 09:17 AM, Oscar Salvador wrote:
> > On Thu, Jul 12, 2018 at 04:37:26PM -0400, Pavel Tatashin wrote:
> >> +static void *sparsemap_buf __meminitdata;
> >> +static void *sparsemap_buf_end __meminitdata;
> >> +
> >> +void __init sparse_buffer_init(unsigned long size, int nid)
> >> +{
> >> + BUG_ON(sparsemap_buf);
> >
> > Why do we need a BUG_ON() here?
> > Looking at the code I cannot really see how we can end up with
> > sparsemap_buf being NULL.
> > Is it just for over-protection?
>
> This checks that we do not accidentally leak memory by calling
> sparse_buffer_init() consequently without sparse_buffer_fini() in-between.
A memory leak isn't serious enough to justify crashing the kernel.
Therefore
--- a/mm/sparse.c~mm-sparse-abstract-sparse-buffer-allocations-fix-fix
+++ a/mm/sparse.c
@@ -469,7 +469,7 @@ static void *sparsemap_buf_end __meminit
void __init sparse_buffer_init(unsigned long size, int nid)
{
- BUG_ON(sparsemap_buf);
+ WARN_ON(sparsemap_buf); /* forgot to call sparse_buffer_fini()? */
sparsemap_buf =
memblock_virt_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
_
Re: [PATCH v5 1/5] mm/sparse: abstract sparse buffer allocations
On 07/13/2018 09:17 AM, Oscar Salvador wrote:
> On Thu, Jul 12, 2018 at 04:37:26PM -0400, Pavel Tatashin wrote:
>> +static void *sparsemap_buf __meminitdata;
>> +static void *sparsemap_buf_end __meminitdata;
>> +
>> +void __init sparse_buffer_init(unsigned long size, int nid)
>> +{
>> +BUG_ON(sparsemap_buf);
>
> Why do we need a BUG_ON() here?
> Looking at the code I cannot really see how we can end up with sparsemap_buf
> being NULL.
> Is it just for over-protection?
This checks that we do not accidentally leak memory by calling
sparse_buffer_init() consequently without sparse_buffer_fini() in-between.
>
>> +sparsemap_buf =
>> +memblock_virt_alloc_try_nid_raw(size, PAGE_SIZE,
>> +__pa(MAX_DMA_ADDRESS),
>> +BOOTMEM_ALLOC_ACCESSIBLE, nid);
>
> In your previous version, you didn't pass a required alignment when setting
> up sparsemap_buf.
> size is already PMD_SIZE aligned, do we need to align it also to PAGE_SIZE?
>
I decided to add PAGE_SIZE alignment, because the implicit memblock alignment
is SMP_CACHE_BYTES which is smaller than page size. While, in practice we will
most likely get a page size aligned allocation, it is still possible that some
ranges in memblock are not page size aligned if that the way they were passed
from BIOS.
Thank you,
Pavel
Re: [PATCH v5 1/5] mm/sparse: abstract sparse buffer allocations
On 07/13/2018 09:17 AM, Oscar Salvador wrote:
> On Thu, Jul 12, 2018 at 04:37:26PM -0400, Pavel Tatashin wrote:
>> +static void *sparsemap_buf __meminitdata;
>> +static void *sparsemap_buf_end __meminitdata;
>> +
>> +void __init sparse_buffer_init(unsigned long size, int nid)
>> +{
>> +BUG_ON(sparsemap_buf);
>
> Why do we need a BUG_ON() here?
> Looking at the code I cannot really see how we can end up with sparsemap_buf
> being NULL.
> Is it just for over-protection?
This checks that we do not accidentally leak memory by calling
sparse_buffer_init() consequently without sparse_buffer_fini() in-between.
>
>> +sparsemap_buf =
>> +memblock_virt_alloc_try_nid_raw(size, PAGE_SIZE,
>> +__pa(MAX_DMA_ADDRESS),
>> +BOOTMEM_ALLOC_ACCESSIBLE, nid);
>
> In your previous version, you didn't pass a required alignment when setting
> up sparsemap_buf.
> size is already PMD_SIZE aligned, do we need to align it also to PAGE_SIZE?
>
I decided to add PAGE_SIZE alignment, because the implicit memblock alignment
is SMP_CACHE_BYTES which is smaller than page size. While, in practice we will
most likely get a page size aligned allocation, it is still possible that some
ranges in memblock are not page size aligned if that the way they were passed
from BIOS.
Thank you,
Pavel
Re: [PATCH v5 1/5] mm/sparse: abstract sparse buffer allocations
On Thu, Jul 12, 2018 at 04:37:26PM -0400, Pavel Tatashin wrote:
> +static void *sparsemap_buf __meminitdata;
> +static void *sparsemap_buf_end __meminitdata;
> +
> +void __init sparse_buffer_init(unsigned long size, int nid)
> +{
> + BUG_ON(sparsemap_buf);
Why do we need a BUG_ON() here?
Looking at the code I cannot really see how we can end up with sparsemap_buf
being NULL.
Is it just for over-protection?
> + sparsemap_buf =
> + memblock_virt_alloc_try_nid_raw(size, PAGE_SIZE,
> + __pa(MAX_DMA_ADDRESS),
> + BOOTMEM_ALLOC_ACCESSIBLE, nid);
In your previous version, you didn't pass a required alignment when setting up
sparsemap_buf.
size is already PMD_SIZE aligned, do we need to align it also to PAGE_SIZE?
Thanks
--
Oscar Salvador
SUSE L3
Re: [PATCH v5 1/5] mm/sparse: abstract sparse buffer allocations
On Thu, Jul 12, 2018 at 04:37:26PM -0400, Pavel Tatashin wrote:
> +static void *sparsemap_buf __meminitdata;
> +static void *sparsemap_buf_end __meminitdata;
> +
> +void __init sparse_buffer_init(unsigned long size, int nid)
> +{
> + BUG_ON(sparsemap_buf);
Why do we need a BUG_ON() here?
Looking at the code I cannot really see how we can end up with sparsemap_buf
being NULL.
Is it just for over-protection?
> + sparsemap_buf =
> + memblock_virt_alloc_try_nid_raw(size, PAGE_SIZE,
> + __pa(MAX_DMA_ADDRESS),
> + BOOTMEM_ALLOC_ACCESSIBLE, nid);
In your previous version, you didn't pass a required alignment when setting up
sparsemap_buf.
size is already PMD_SIZE aligned, do we need to align it also to PAGE_SIZE?
Thanks
--
Oscar Salvador
SUSE L3
Re: [PATCH v5 1/5] mm/sparse: abstract sparse buffer allocations
On Thu, 12 Jul 2018 16:37:26 -0400 Pavel Tatashin
wrote:
> When struct pages are allocated for sparse-vmemmap VA layout, we first
> try to allocate one large buffer, and than if that fails allocate struct
> pages for each section as we go.
>
> The code that allocates buffer is uses global variables and is spread
> across several call sites.
>
> Cleanup the code by introducing three functions to handle the global
> buffer:
>
> sparse_buffer_init() initialize the buffer
> sparse_buffer_fini() free the remaining part of the buffer
> sparse_buffer_alloc() alloc from the buffer, and if buffer is empty
> return NULL
>
> Define these functions in sparse.c instead of sparse-vmemmap.c because
> later we will use them for non-vmemmap sparse allocations as well.
>
> ...
>
> +void * __meminit sparse_buffer_alloc(unsigned long size)
> +{
> + void *ptr = NULL;
> +
> + if (sparsemap_buf) {
> + ptr = (void *)ALIGN((unsigned long)sparsemap_buf, size);
> + if (ptr + size > sparsemap_buf_end)
> + ptr = NULL;
> + else
> + sparsemap_buf = ptr + size;
> + }
> + return ptr;
> +}
tweak...
diff -puN mm/sparse.c~mm-sparse-abstract-sparse-buffer-allocations-fix
mm/sparse.c
--- a/mm/sparse.c~mm-sparse-abstract-sparse-buffer-allocations-fix
+++ a/mm/sparse.c
@@ -491,7 +491,7 @@ void * __meminit sparse_buffer_alloc(uns
void *ptr = NULL;
if (sparsemap_buf) {
- ptr = (void *)ALIGN((unsigned long)sparsemap_buf, size);
+ ptr = PTR_ALIGN(sparsemap_buf, size);
if (ptr + size > sparsemap_buf_end)
ptr = NULL;
else
Re: [PATCH v5 1/5] mm/sparse: abstract sparse buffer allocations
On Thu, 12 Jul 2018 16:37:26 -0400 Pavel Tatashin
wrote:
> When struct pages are allocated for sparse-vmemmap VA layout, we first
> try to allocate one large buffer, and than if that fails allocate struct
> pages for each section as we go.
>
> The code that allocates buffer is uses global variables and is spread
> across several call sites.
>
> Cleanup the code by introducing three functions to handle the global
> buffer:
>
> sparse_buffer_init() initialize the buffer
> sparse_buffer_fini() free the remaining part of the buffer
> sparse_buffer_alloc() alloc from the buffer, and if buffer is empty
> return NULL
>
> Define these functions in sparse.c instead of sparse-vmemmap.c because
> later we will use them for non-vmemmap sparse allocations as well.
>
> ...
>
> +void * __meminit sparse_buffer_alloc(unsigned long size)
> +{
> + void *ptr = NULL;
> +
> + if (sparsemap_buf) {
> + ptr = (void *)ALIGN((unsigned long)sparsemap_buf, size);
> + if (ptr + size > sparsemap_buf_end)
> + ptr = NULL;
> + else
> + sparsemap_buf = ptr + size;
> + }
> + return ptr;
> +}
tweak...
diff -puN mm/sparse.c~mm-sparse-abstract-sparse-buffer-allocations-fix
mm/sparse.c
--- a/mm/sparse.c~mm-sparse-abstract-sparse-buffer-allocations-fix
+++ a/mm/sparse.c
@@ -491,7 +491,7 @@ void * __meminit sparse_buffer_alloc(uns
void *ptr = NULL;
if (sparsemap_buf) {
- ptr = (void *)ALIGN((unsigned long)sparsemap_buf, size);
+ ptr = PTR_ALIGN(sparsemap_buf, size);
if (ptr + size > sparsemap_buf_end)
ptr = NULL;
else
