[PATCH 1/2] mm, slob: Prevent false positive trace upon allocation failure

[PATCH 1/2] mm, slob: Prevent false positive trace upon allocation failure

[Ezequiel Garcia]

This patch changes the __kmalloc_node() logic to return NULL
if alloc_pages() fails to return valid pages.
This is done to avoid to trace a false positive kmalloc event.

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Glauber Costa <glommer@parallels.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 mm/slob.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/mm/slob.c b/mm/slob.c
index 45d4ca7..686e98b 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -450,15 +450,16 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
 				   size, size + align, gfp, node);
 	} else {
 		unsigned int order = get_order(size);
+		struct page *page;
 
 		if (likely(order))
 			gfp |= __GFP_COMP;
 		ret = slob_new_pages(gfp, order, node);
-		if (ret) {
-			struct page *page;
-			page = virt_to_page(ret);
-			page->private = size;
-		}
+		if (!ret)
+			return NULL;
+
+		page = virt_to_page(ret);
+		page->private = size;
 
 		trace_kmalloc_node(_RET_IP_, ret,
 				   size, PAGE_SIZE << order, gfp, node);
-- 
1.7.8.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC/PATCH 2/2] mm, slob: Save real allocated size in page->private

[Ezequiel Garcia]

As documented in slob.c header, page->private field is used to return
accurately the allocated size, through ksize().
Therefore, if one allocates a contiguous set of pages the available size
is PAGE_SIZE << order, instead of the requested size.

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Glauber Costa <glommer@parallels.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 mm/slob.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/slob.c b/mm/slob.c
index 686e98b..4c89b7d 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -459,7 +459,7 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
 			return NULL;
 
 		page = virt_to_page(ret);
-		page->private = size;
+		page->private = PAGE_SIZE << order;
 
 		trace_kmalloc_node(_RET_IP_, ret,
 				   size, PAGE_SIZE << order, gfp, node);
-- 
1.7.8.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm, slob: Prevent false positive trace upon allocation failure

[Christoph Lameter]

On Tue, 14 Aug 2012, Ezequiel Garcia wrote:

> This patch changes the __kmalloc_node() logic to return NULL
> if alloc_pages() fails to return valid pages.
> This is done to avoid to trace a false positive kmalloc event.

Acked-by: Christoph Lameter <cl@linux.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC/PATCH 2/2] mm, slob: Save real allocated size in page->private

[Christoph Lameter]

On Tue, 14 Aug 2012, Ezequiel Garcia wrote:

> As documented in slob.c header, page->private field is used to return
> accurately the allocated size, through ksize().
> Therefore, if one allocates a contiguous set of pages the available size
> is PAGE_SIZE << order, instead of the requested size.

I would prefer if you would remove this strange feature from slob. The
ksize for a !PageSlab() "slab" page is always PAGE_SIZE << compound_order(page).
There is no need to use page->private here. It is a bad practice to not
mark a page as a slab page but then use fields for special purposes.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC/PATCH 2/2] mm, slob: Save real allocated size in page->private

[Ezequiel Garcia]

Hi Christoph,

Thanks for your comments.

On Tue, Aug 14, 2012 at 11:56 AM, Christoph Lameter <cl@linux.com> wrote:
> On Tue, 14 Aug 2012, Ezequiel Garcia wrote:
>
>> As documented in slob.c header, page->private field is used to return
>> accurately the allocated size, through ksize().
>> Therefore, if one allocates a contiguous set of pages the available size
>> is PAGE_SIZE << order, instead of the requested size.
>
> I would prefer if you would remove this strange feature from slob. The
> ksize for a !PageSlab() "slab" page is always PAGE_SIZE << compound_order(page).
> There is no need to use page->private here. It is a bad practice to not
> mark a page as a slab page but then use fields for special purposes.
>

Mmm, I see. Sounds sensible.
Fortunately I don't have to squeeze my brain thinking,
since I have a nice example of this in slub's ksize().

        if (unlikely(!PageSlab(page))) {
                WARN_ON(!PageCompound(page));
                return PAGE_SIZE << compound_order(page);
        }

I'll resend this patch alone with implementing something like
it and removing page->private usage.

Thanks again!
Ezequiel.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm, slob: Prevent false positive trace upon allocation failure

[Ezequiel Garcia]

Pekka,

I'd like to bring your attention to this patch, and make a little question.


On Tue, Aug 14, 2012 at 11:38 AM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> This patch changes the __kmalloc_node() logic to return NULL
> if alloc_pages() fails to return valid pages.
> This is done to avoid to trace a false positive kmalloc event.
>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Glauber Costa <glommer@parallels.com>
> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> ---
>  mm/slob.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/mm/slob.c b/mm/slob.c
> index 45d4ca7..686e98b 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -450,15 +450,16 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
>                                    size, size + align, gfp, node);
>         } else {
>                 unsigned int order = get_order(size);
> +               struct page *page;
>
>                 if (likely(order))
>                         gfp |= __GFP_COMP;
>                 ret = slob_new_pages(gfp, order, node);
> -               if (ret) {
> -                       struct page *page;
> -                       page = virt_to_page(ret);
> -                       page->private = size;
> -               }
> +               if (!ret)
> +                       return NULL;
> +
> +               page = virt_to_page(ret);
> +               page->private = size;
>
>                 trace_kmalloc_node(_RET_IP_, ret,
>                                    size, PAGE_SIZE << order, gfp, node);


As you can see this patch prevents to trace a kmem event if the allocation
fails.

I'm still unsure about tracing or not this ones, and I'm considering tracing
failures, perhaps with return=0 and allocated size=0.

In this case, it would be nice to have SLxB all do the same.
Right now, this is not the case.

You can see how slob::kmem_cache_alloc_node traces independently
of the allocation succeeding.
I have no problem trying a fix for this, but I don't now how to trace
this cases.

Although it is a corner case, I think it's important to define a clear
and consistent
behaviour to make tracing reliable.

Thanks,
Ezequiel.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm, slob: Prevent false positive trace upon allocation failure

[Pekka Enberg]

On Wed, Aug 15, 2012 at 3:34 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> As you can see this patch prevents to trace a kmem event if the allocation
> fails.
>
> I'm still unsure about tracing or not this ones, and I'm considering tracing
> failures, perhaps with return=0 and allocated size=0.
>
> In this case, it would be nice to have SLxB all do the same.
> Right now, this is not the case.
>
> You can see how slob::kmem_cache_alloc_node traces independently
> of the allocation succeeding.
> I have no problem trying a fix for this, but I don't now how to trace
> this cases.
>
> Although it is a corner case, I think it's important to define a clear
> and consistent behaviour to make tracing reliable.

Agreed on consistency. I think it's valuable to be able to trace
allocation failures and let userspace filter them out if they don't
need them.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm, slob: Prevent false positive trace upon allocation failure

[Pekka Enberg]

Hi Ezequiel,

On Tue, Aug 14, 2012 at 5:38 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> This patch changes the __kmalloc_node() logic to return NULL
> if alloc_pages() fails to return valid pages.
> This is done to avoid to trace a false positive kmalloc event.
>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Glauber Costa <glommer@parallels.com>
> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> ---
>  mm/slob.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/mm/slob.c b/mm/slob.c
> index 45d4ca7..686e98b 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -450,15 +450,16 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
>                                    size, size + align, gfp, node);
>         } else {
>                 unsigned int order = get_order(size);
> +               struct page *page;
>
>                 if (likely(order))
>                         gfp |= __GFP_COMP;
>                 ret = slob_new_pages(gfp, order, node);
> -               if (ret) {
> -                       struct page *page;
> -                       page = virt_to_page(ret);
> -                       page->private = size;
> -               }
> +               if (!ret)
> +                       return NULL;
> +
> +               page = virt_to_page(ret);
> +               page->private = size;
>
>                 trace_kmalloc_node(_RET_IP_, ret,
>                                    size, PAGE_SIZE << order, gfp, node);

As mentioned earlier, I think it's valuable for the userspace to be
able to trace allocation failures as well. So I'm not applying this
patch.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm, slob: Prevent false positive trace upon allocation failure

[Ezequiel Garcia]

Hi Pekka,

On Tue, Sep 4, 2012 at 4:32 AM, Pekka Enberg <penberg@kernel.org> wrote:
> Hi Ezequiel,
>
> On Tue, Aug 14, 2012 at 5:38 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>> This patch changes the __kmalloc_node() logic to return NULL
>> if alloc_pages() fails to return valid pages.
>> This is done to avoid to trace a false positive kmalloc event.
>>
>> Cc: Pekka Enberg <penberg@kernel.org>
>> Cc: Christoph Lameter <cl@linux.com>
>> Cc: Glauber Costa <glommer@parallels.com>
>> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
>> ---
>>  mm/slob.c |   11 ++++++-----
>>  1 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/slob.c b/mm/slob.c
>> index 45d4ca7..686e98b 100644
>> --- a/mm/slob.c
>> +++ b/mm/slob.c
>> @@ -450,15 +450,16 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
>>                                    size, size + align, gfp, node);
>>         } else {
>>                 unsigned int order = get_order(size);
>> +               struct page *page;
>>
>>                 if (likely(order))
>>                         gfp |= __GFP_COMP;
>>                 ret = slob_new_pages(gfp, order, node);
>> -               if (ret) {
>> -                       struct page *page;
>> -                       page = virt_to_page(ret);
>> -                       page->private = size;
>> -               }
>> +               if (!ret)
>> +                       return NULL;
>> +
>> +               page = virt_to_page(ret);
>> +               page->private = size;
>>
>>                 trace_kmalloc_node(_RET_IP_, ret,
>>                                    size, PAGE_SIZE << order, gfp, node);
>
> As mentioned earlier, I think it's valuable for the userspace to be
> able to trace allocation failures as well. So I'm not applying this
> patch.

Yes, you're right. I have a few patches for that.

Thanks!
Ezequiel.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>