diff mbox series

[1/1] arch/um: fix forward declaration for vmalloc

Message ID 20240326073750.726636-1-surenb@google.com (mailing list archive)
State New
Headers show
Series [1/1] arch/um: fix forward declaration for vmalloc | expand

Commit Message

Suren Baghdasaryan March 26, 2024, 7:37 a.m. UTC
Patch [1] replaced vmalloc() function with a new definition but it did
not adjust the forward declaration used in UML architecture. Change it
to act as before.
Note that this prevents the vmalloc() allocations in __wrap_malloc()
from being accounted. If accounting here is critical, we will have
to remove this forward declaration and include vmalloc.h, however
that would pull in more dependencies and would require introducing more
architecture-specific headers, like asm/bug.h, asm/rwonce.h, etc.
This is likely the reason why this forward declaration was introduced
in the first place.

[1] https://lore.kernel.org/all/20240321163705.3067592-31-surenb@google.com/

Fixes: 576477564ede ("mm: vmalloc: enable memory allocation profiling")
Reported-by: SeongJae Park <sj@kernel.org>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 arch/um/include/shared/um_malloc.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

SeongJae Park March 26, 2024, 3:37 p.m. UTC | #1
On Tue, 26 Mar 2024 00:37:50 -0700 Suren Baghdasaryan <surenb@google.com> wrote:

> Patch [1] replaced vmalloc() function with a new definition but it did
> not adjust the forward declaration used in UML architecture. Change it
> to act as before.
> Note that this prevents the vmalloc() allocations in __wrap_malloc()
> from being accounted. If accounting here is critical, we will have
> to remove this forward declaration and include vmalloc.h, however
> that would pull in more dependencies and would require introducing more
> architecture-specific headers, like asm/bug.h, asm/rwonce.h, etc.
> This is likely the reason why this forward declaration was introduced
> in the first place.
> 
> [1] https://lore.kernel.org/all/20240321163705.3067592-31-surenb@google.com/
> 
> Fixes: 576477564ede ("mm: vmalloc: enable memory allocation profiling")
> Reported-by: SeongJae Park <sj@kernel.org>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Thank you for this fix, Suren.  I confirmed that this patch fixes the issue I
reported.

Closes: https://lore.kernel.org/all/20240323180506.195396-1-sj@kernel.org/
Tested-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

> ---
>  arch/um/include/shared/um_malloc.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/um/include/shared/um_malloc.h b/arch/um/include/shared/um_malloc.h
> index 13da93284c2c..bf503658f08e 100644
> --- a/arch/um/include/shared/um_malloc.h
> +++ b/arch/um/include/shared/um_malloc.h
> @@ -11,7 +11,8 @@
>  extern void *uml_kmalloc(int size, int flags);
>  extern void kfree(const void *ptr);
>  
> -extern void *vmalloc(unsigned long size);
> +extern void *vmalloc_noprof(unsigned long size);
> +#define vmalloc(...)		vmalloc_noprof(__VA_ARGS__)
>  extern void vfree(void *ptr);
>  
>  #endif /* __UM_MALLOC_H__ */
> -- 
> 2.44.0.396.g6e790dbe36-goog
Johannes Berg March 28, 2024, 8:48 a.m. UTC | #2
On Tue, 2024-03-26 at 00:37 -0700, Suren Baghdasaryan wrote:
> 
> -extern void *vmalloc(unsigned long size);
> +extern void *vmalloc_noprof(unsigned long size);
> +#define vmalloc(...)		vmalloc_noprof(__VA_ARGS__)
> 

I was confused a bit by the define at first, but that's because this is
a user-side header file.

Reviewed-by: Johannes Berg <johannes@sipsolutions.net

johannes
Richard Weinberger April 22, 2024, 8:11 p.m. UTC | #3
----- Ursprüngliche Mail -----
> Von: "Suren Baghdasaryan" <surenb@google.com>
> Betreff: [PATCH 1/1] arch/um: fix forward declaration for vmalloc

> Patch [1] replaced vmalloc() function with a new definition but it did
> not adjust the forward declaration used in UML architecture. Change it
> to act as before.
> Note that this prevents the vmalloc() allocations in __wrap_malloc()
> from being accounted. If accounting here is critical, we will have
> to remove this forward declaration and include vmalloc.h, however
> that would pull in more dependencies and would require introducing more
> architecture-specific headers, like asm/bug.h, asm/rwonce.h, etc.
> This is likely the reason why this forward declaration was introduced
> in the first place.
> 
> [1] https://lore.kernel.org/all/20240321163705.3067592-31-surenb@google.com/
> 
> Fixes: 576477564ede ("mm: vmalloc: enable memory allocation profiling")

This commit id is not in Linus tree.
Do I miss something?

Thanks,
//richard
Suren Baghdasaryan April 22, 2024, 8:19 p.m. UTC | #4
On Mon, Apr 22, 2024 at 1:11 PM Richard Weinberger <richard@nod.at> wrote:
>
> ----- Ursprüngliche Mail -----
> > Von: "Suren Baghdasaryan" <surenb@google.com>
> > Betreff: [PATCH 1/1] arch/um: fix forward declaration for vmalloc
>
> > Patch [1] replaced vmalloc() function with a new definition but it did
> > not adjust the forward declaration used in UML architecture. Change it
> > to act as before.
> > Note that this prevents the vmalloc() allocations in __wrap_malloc()
> > from being accounted. If accounting here is critical, we will have
> > to remove this forward declaration and include vmalloc.h, however
> > that would pull in more dependencies and would require introducing more
> > architecture-specific headers, like asm/bug.h, asm/rwonce.h, etc.
> > This is likely the reason why this forward declaration was introduced
> > in the first place.
> >
> > [1] https://lore.kernel.org/all/20240321163705.3067592-31-surenb@google.com/
> >
> > Fixes: 576477564ede ("mm: vmalloc: enable memory allocation profiling")
>
> This commit id is not in Linus tree.
> Do I miss something?

It's in mm-unstable under dc26c7e79daf2fc11169b23c150862f0e878ee5a. I
think it just didn't reach Linus' tree yet.

>
> Thanks,
> //richard
Richard Weinberger April 22, 2024, 8:31 p.m. UTC | #5
----- Ursprüngliche Mail -----
> Von: "Suren Baghdasaryan" <surenb@google.com>
>> > Fixes: 576477564ede ("mm: vmalloc: enable memory allocation profiling")
>>
>> This commit id is not in Linus tree.
>> Do I miss something?
> 
> It's in mm-unstable under dc26c7e79daf2fc11169b23c150862f0e878ee5a. I
> think it just didn't reach Linus' tree yet.

Hmm, so we better postpone this path until said commit hits Linus tree,
or you carry it together with the commit in mm-unstable.

Thanks,
//richard
Suren Baghdasaryan April 22, 2024, 8:39 p.m. UTC | #6
On Mon, Apr 22, 2024 at 1:31 PM Richard Weinberger <richard@nod.at> wrote:
>
> ----- Ursprüngliche Mail -----
> > Von: "Suren Baghdasaryan" <surenb@google.com>
> >> > Fixes: 576477564ede ("mm: vmalloc: enable memory allocation profiling")
> >>
> >> This commit id is not in Linus tree.
> >> Do I miss something?
> >
> > It's in mm-unstable under dc26c7e79daf2fc11169b23c150862f0e878ee5a. I
> > think it just didn't reach Linus' tree yet.
>
> Hmm, so we better postpone this path until said commit hits Linus tree,
> or you carry it together with the commit in mm-unstable.

Oh, sorry, I didn't realize you were talking about the `Fixes:
576477564ede` part... Yeah, unfortunately SHAs in mm-unstable are
unstable, so the change being fixed is under
edf0a25633bda1e5b7844478dd13b4326a3d5d09 now. I think Andrew placed
this fix right after the change it fixes with intention to merge them
together later on.

>
> Thanks,
> //richard
Richard Weinberger April 22, 2024, 8:50 p.m. UTC | #7
----- Ursprüngliche Mail -----
> Von: "Suren Baghdasaryan" <surenb@google.com>
>> > It's in mm-unstable under dc26c7e79daf2fc11169b23c150862f0e878ee5a. I
>> > think it just didn't reach Linus' tree yet.
>>
>> Hmm, so we better postpone this path until said commit hits Linus tree,
>> or you carry it together with the commit in mm-unstable.
> 
> Oh, sorry, I didn't realize you were talking about the `Fixes:
> 576477564ede` part... Yeah, unfortunately SHAs in mm-unstable are
> unstable, so the change being fixed is under
> edf0a25633bda1e5b7844478dd13b4326a3d5d09 now. I think Andrew placed
> this fix right after the change it fixes with intention to merge them
> together later on.

Ah, the patch itself goes via Andrew's tree, works for me!
Let me note this in the uml patchwork system.

Acked-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard
diff mbox series

Patch

diff --git a/arch/um/include/shared/um_malloc.h b/arch/um/include/shared/um_malloc.h
index 13da93284c2c..bf503658f08e 100644
--- a/arch/um/include/shared/um_malloc.h
+++ b/arch/um/include/shared/um_malloc.h
@@ -11,7 +11,8 @@ 
 extern void *uml_kmalloc(int size, int flags);
 extern void kfree(const void *ptr);
 
-extern void *vmalloc(unsigned long size);
+extern void *vmalloc_noprof(unsigned long size);
+#define vmalloc(...)		vmalloc_noprof(__VA_ARGS__)
 extern void vfree(void *ptr);
 
 #endif /* __UM_MALLOC_H__ */