When a process updates the RSS of a different process, the rss_stat
tracepoint appears in the context of the process doing the update. This
can confuse userspace that the RSS of process doing the update is
updated, while in reality a different process's RSS was updated.
This issue happens in reclaim paths such as with direct reclaim or
background reclaim.
This patch adds more information to the tracepoint about whether the mm
being updated belongs to the current process's context (curr field). We
also include a hash of the mm pointer so that the process who the mm
belongs to can be uniquely identified (mm_id field).
Also vsprintf.c is refactored a bit to allow reuse of hashing code.
Reported-by: Ioannis Ilkos <[email protected]>
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
Based on top of the commit in linux-next:
8342d836dc7c ("mm: emit tracepoint when RSS changes")
Google Bug: 140711541
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
include/linux/mm.h | 8 ++++----
include/linux/string.h | 2 ++
include/trace/events/kmem.h | 32 +++++++++++++++++++++++++++++---
lib/vsprintf.c | 36 +++++++++++++++++++++++++-----------
mm/memory.c | 4 ++--
5 files changed, 62 insertions(+), 20 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 31d8cfb3d988..bfbe65ccffa3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1643,27 +1643,27 @@ static inline unsigned long get_mm_counter(struct mm_struct *mm, int member)
return (unsigned long)val;
}
-void mm_trace_rss_stat(int member, long count);
+void mm_trace_rss_stat(struct mm_struct *mm, int member, long count);
static inline void add_mm_counter(struct mm_struct *mm, int member, long value)
{
long count = atomic_long_add_return(value, &mm->rss_stat.count[member]);
- mm_trace_rss_stat(member, count);
+ mm_trace_rss_stat(mm, member, count);
}
static inline void inc_mm_counter(struct mm_struct *mm, int member)
{
long count = atomic_long_inc_return(&mm->rss_stat.count[member]);
- mm_trace_rss_stat(member, count);
+ mm_trace_rss_stat(mm, member, count);
}
static inline void dec_mm_counter(struct mm_struct *mm, int member)
{
long count = atomic_long_dec_return(&mm->rss_stat.count[member]);
- mm_trace_rss_stat(member, count);
+ mm_trace_rss_stat(mm, member, count);
}
/* Optimized variant when page is already known not to be PageAnon */
diff --git a/include/linux/string.h b/include/linux/string.h
index f516cec5277c..6b0e950701d0 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -261,6 +261,8 @@ int bprintf(u32 *bin_buf, size_t size, const char *fmt, ...) __printf(3, 4);
extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
const void *from, size_t available);
+int ptr_to_hashval(const void *ptr, unsigned long *hashval_out);
+
/**
* strstarts - does @str start with @prefix?
* @str: string to examine
diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
index 5a0666bfcf85..ad7e642bd497 100644
--- a/include/trace/events/kmem.h
+++ b/include/trace/events/kmem.h
@@ -316,24 +316,50 @@ TRACE_EVENT(mm_page_alloc_extfrag,
__entry->change_ownership)
);
+/*
+ * Required for uniquely and securely identifying mm in rss_stat tracepoint.
+ */
+#ifndef __PTR_TO_HASHVAL
+static unsigned int __maybe_unused mm_ptr_to_hash(const void *ptr)
+{
+ int ret;
+ unsigned long hashval;
+
+ ret = ptr_to_hashval(ptr, &hashval);
+ if (ret)
+ return 0;
+
+ /* The hashed value is only 32-bit */
+ return (unsigned int)hashval;
+}
+#define __PTR_TO_HASHVAL
+#endif
+
TRACE_EVENT(rss_stat,
- TP_PROTO(int member,
+ TP_PROTO(struct mm_struct *mm,
+ int member,
long count),
- TP_ARGS(member, count),
+ TP_ARGS(mm, member, count),
TP_STRUCT__entry(
+ __field(unsigned int, mm_id)
+ __field(unsigned int, curr)
__field(int, member)
__field(long, size)
),
TP_fast_assign(
+ __entry->mm_id = mm_ptr_to_hash(mm);
+ __entry->curr = !!(current->mm == mm);
__entry->member = member;
__entry->size = (count << PAGE_SHIFT);
),
- TP_printk("member=%d size=%ldB",
+ TP_printk("mm_id=%u curr=%d member=%d size=%ldB",
+ __entry->mm_id,
+ __entry->curr,
__entry->member,
__entry->size)
);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index dee8fc467fcf..401baaac1813 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
early_initcall(initialize_ptr_random);
/* Maps a pointer to a 32 bit unique identifier. */
+int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
+{
+ const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
+ unsigned long hashval;
+
+ if (static_branch_unlikely(¬_filled_random_ptr_key))
+ return -EAGAIN;
+
+#ifdef CONFIG_64BIT
+ hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
+ /*
+ * Mask off the first 32 bits, this makes explicit that we have
+ * modified the address (and 32 bits is plenty for a unique ID).
+ */
+ hashval = hashval & 0xffffffff;
+#else
+ hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
+#endif
+ *hashval_out = hashval;
+ return 0;
+}
+
static char *ptr_to_id(char *buf, char *end, const void *ptr,
struct printf_spec spec)
{
const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
unsigned long hashval;
+ int ret;
/* When debugging early boot use non-cryptographically secure hash. */
if (unlikely(debug_boot_weak_hash)) {
@@ -773,22 +796,13 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
return pointer_string(buf, end, (const void *)hashval, spec);
}
- if (static_branch_unlikely(¬_filled_random_ptr_key)) {
+ ret = ptr_to_hashval(ptr, &hashval);
+ if (ret) {
spec.field_width = 2 * sizeof(ptr);
/* string length must be less than default_width */
return error_string(buf, end, str, spec);
}
-#ifdef CONFIG_64BIT
- hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
- /*
- * Mask off the first 32 bits, this makes explicit that we have
- * modified the address (and 32 bits is plenty for a unique ID).
- */
- hashval = hashval & 0xffffffff;
-#else
- hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
-#endif
return pointer_string(buf, end, (const void *)hashval, spec);
}
diff --git a/mm/memory.c b/mm/memory.c
index 7596d625ebd1..d3c9784e6dc1 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -154,9 +154,9 @@ static int __init init_zero_pfn(void)
}
core_initcall(init_zero_pfn);
-void mm_trace_rss_stat(int member, long count)
+void mm_trace_rss_stat(struct mm_struct *mm, int member, long count)
{
- trace_rss_stat(member, count);
+ trace_rss_stat(mm, member, count);
}
#if defined(SPLIT_RSS_COUNTING)
--
2.24.0.rc1.363.gb1bccd3e3d-goog
On (19/11/05 21:44), Joel Fernandes (Google) wrote:
[..]
> +++ b/lib/vsprintf.c
> @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
> early_initcall(initialize_ptr_random);
>
> /* Maps a pointer to a 32 bit unique identifier. */
> +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> +{
> + const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
> + unsigned long hashval;
> +
> + if (static_branch_unlikely(¬_filled_random_ptr_key))
> + return -EAGAIN;
> +
> +#ifdef CONFIG_64BIT
> + hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> + /*
> + * Mask off the first 32 bits, this makes explicit that we have
> + * modified the address (and 32 bits is plenty for a unique ID).
> + */
> + hashval = hashval & 0xffffffff;
> +#else
> + hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> +#endif
> + *hashval_out = hashval;
> + return 0;
> +}
Maybe we want to move this thing to more generic code - lib/siphash.c -
since it's going to have several users.
-ss
On Tue 2019-11-05 21:44:51, Joel Fernandes (Google) wrote:
> Also vsprintf.c is refactored a bit to allow reuse of hashing code.
I agree with Sergey that it would make sense to move this outside
vsprintf.c.
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index dee8fc467fcf..401baaac1813 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
> early_initcall(initialize_ptr_random);
>
> /* Maps a pointer to a 32 bit unique identifier. */
> +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> +{
> + const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
str is unused.
> + unsigned long hashval;
IMHO, this local variable unnecessarily complicates the code.
I would use the pointer directly and let compiler to optimize.
> + if (static_branch_unlikely(¬_filled_random_ptr_key))
> + return -EAGAIN;
> +
> +#ifdef CONFIG_64BIT
> + hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> + /*
> + * Mask off the first 32 bits, this makes explicit that we have
> + * modified the address (and 32 bits is plenty for a unique ID).
> + */
> + hashval = hashval & 0xffffffff;
> +#else
> + hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> +#endif
> + *hashval_out = hashval;
> + return 0;
> +}
Best Regards,
Petr
On Wed, Nov 06, 2019 at 09:59:59AM +0100, Petr Mladek wrote:
> On Tue 2019-11-05 21:44:51, Joel Fernandes (Google) wrote:
> > Also vsprintf.c is refactored a bit to allow reuse of hashing code.
>
> I agree with Sergey that it would make sense to move this outside
> vsprintf.c.
I am of the opinion that its Ok to have it this way and I am not sure if
another translation unit is worth it just for this. If we have more users,
then yes we can consider splitting into its own translation unit at that
time.
If Andrew Morton objects, then I'll do it since the intention was for this
patch to go through his tree and I believe he is Ok with it this way.
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index dee8fc467fcf..401baaac1813 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
> > early_initcall(initialize_ptr_random);
> >
> > /* Maps a pointer to a 32 bit unique identifier. */
> > +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> > +{
> > + const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
>
> str is unused.
I believe Andrew has already fixed this in his tree.
linux-next has local variable removed now:
7422993b4f8e ("rss_stat-add-support-to-detect-rss-updates-of-external-mm-fix")
> > + unsigned long hashval;
>
> IMHO, this local variable unnecessarily complicates the code.
> I would use the pointer directly and let compiler to optimize.
Agreed.
thanks,
- Joel
> > + if (static_branch_unlikely(¬_filled_random_ptr_key))
> > + return -EAGAIN;
> > +
> > +#ifdef CONFIG_64BIT
> > + hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> > + /*
> > + * Mask off the first 32 bits, this makes explicit that we have
> > + * modified the address (and 32 bits is plenty for a unique ID).
> > + */
> > + hashval = hashval & 0xffffffff;
> > +#else
> > + hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> > +#endif
> > + *hashval_out = hashval;
> > + return 0;
> > +}
>
> Best Regards,
> Petr
On Thu 2019-11-07 13:07:51, Joel Fernandes wrote:
> On Wed, Nov 06, 2019 at 09:59:59AM +0100, Petr Mladek wrote:
> > On Tue 2019-11-05 21:44:51, Joel Fernandes (Google) wrote:
> > > Also vsprintf.c is refactored a bit to allow reuse of hashing code.
> >
> > I agree with Sergey that it would make sense to move this outside
> > vsprintf.c.
>
> I am of the opinion that its Ok to have it this way and I am not sure if
> another translation unit is worth it just for this. If we have more users,
> then yes we can consider splitting into its own translation unit at that
> time.
Fair enough. I still think that it would make sense to move the code
but I do not want to block this patch because of this.
My view is that vsprintf.c is huge and the hashing-related code
is lost there. It consists of more pieces, early_param, static key,
workqueue work.
> If Andrew Morton objects, then I'll do it since the intention was for this
> patch to go through his tree and I believe he is Ok with it this way.
>
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index dee8fc467fcf..401baaac1813 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
> > > early_initcall(initialize_ptr_random);
> > >
> > > /* Maps a pointer to a 32 bit unique identifier. */
> > > +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> > > +{
> > > + const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
> >
> > str is unused.
>
> I believe Andrew has already fixed this in his tree.
>
> linux-next has local variable removed now:
> 7422993b4f8e ("rss_stat-add-support-to-detect-rss-updates-of-external-mm-fix")
I see. With this fix, feel free to add:
Acked-by: Petr Mladek <[email protected]> # lib/vsprintf.c
The clean up could be done by a followup patches.
Best Regards,
Petr
/me coming late to the party.
On Tue, 5 Nov 2019 21:44:51 -0500
"Joel Fernandes (Google)" <[email protected]> wrote:
> When a process updates the RSS of a different process, the rss_stat
> tracepoint appears in the context of the process doing the update. This
> can confuse userspace that the RSS of process doing the update is
> updated, while in reality a different process's RSS was updated.
>
> This issue happens in reclaim paths such as with direct reclaim or
> background reclaim.
>
> This patch adds more information to the tracepoint about whether the mm
> being updated belongs to the current process's context (curr field). We
> also include a hash of the mm pointer so that the process who the mm
> belongs to can be uniquely identified (mm_id field).
>
> Also vsprintf.c is refactored a bit to allow reuse of hashing code.
>
> Reported-by: Ioannis Ilkos <[email protected]>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
>
> ---
> Based on top of the commit in linux-next:
> 8342d836dc7c ("mm: emit tracepoint when RSS changes")
>
> Google Bug: 140711541
>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> include/linux/mm.h | 8 ++++----
> include/linux/string.h | 2 ++
> include/trace/events/kmem.h | 32 +++++++++++++++++++++++++++++---
> lib/vsprintf.c | 36 +++++++++++++++++++++++++-----------
> mm/memory.c | 4 ++--
> 5 files changed, 62 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 31d8cfb3d988..bfbe65ccffa3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1643,27 +1643,27 @@ static inline unsigned long get_mm_counter(struct mm_struct *mm, int member)
> return (unsigned long)val;
> }
>
> -void mm_trace_rss_stat(int member, long count);
> +void mm_trace_rss_stat(struct mm_struct *mm, int member, long count);
>
> static inline void add_mm_counter(struct mm_struct *mm, int member, long value)
> {
> long count = atomic_long_add_return(value, &mm->rss_stat.count[member]);
>
> - mm_trace_rss_stat(member, count);
> + mm_trace_rss_stat(mm, member, count);
> }
>
> static inline void inc_mm_counter(struct mm_struct *mm, int member)
> {
> long count = atomic_long_inc_return(&mm->rss_stat.count[member]);
>
> - mm_trace_rss_stat(member, count);
> + mm_trace_rss_stat(mm, member, count);
> }
>
> static inline void dec_mm_counter(struct mm_struct *mm, int member)
> {
> long count = atomic_long_dec_return(&mm->rss_stat.count[member]);
>
> - mm_trace_rss_stat(member, count);
> + mm_trace_rss_stat(mm, member, count);
> }
>
> /* Optimized variant when page is already known not to be PageAnon */
> diff --git a/include/linux/string.h b/include/linux/string.h
> index f516cec5277c..6b0e950701d0 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -261,6 +261,8 @@ int bprintf(u32 *bin_buf, size_t size, const char *fmt, ...) __printf(3, 4);
> extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
> const void *from, size_t available);
>
> +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out);
> +
> /**
> * strstarts - does @str start with @prefix?
> * @str: string to examine
> diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
> index 5a0666bfcf85..ad7e642bd497 100644
> --- a/include/trace/events/kmem.h
> +++ b/include/trace/events/kmem.h
> @@ -316,24 +316,50 @@ TRACE_EVENT(mm_page_alloc_extfrag,
> __entry->change_ownership)
> );
>
> +/*
> + * Required for uniquely and securely identifying mm in rss_stat tracepoint.
> + */
> +#ifndef __PTR_TO_HASHVAL
> +static unsigned int __maybe_unused mm_ptr_to_hash(const void *ptr)
> +{
> + int ret;
> + unsigned long hashval;
> +
> + ret = ptr_to_hashval(ptr, &hashval);
> + if (ret)
> + return 0;
> +
> + /* The hashed value is only 32-bit */
> + return (unsigned int)hashval;
> +}
> +#define __PTR_TO_HASHVAL
> +#endif
> +
> TRACE_EVENT(rss_stat,
>
> - TP_PROTO(int member,
> + TP_PROTO(struct mm_struct *mm,
> + int member,
> long count),
>
> - TP_ARGS(member, count),
> + TP_ARGS(mm, member, count),
>
> TP_STRUCT__entry(
> + __field(unsigned int, mm_id)
> + __field(unsigned int, curr)
> __field(int, member)
> __field(long, size)
> ),
>
> TP_fast_assign(
> + __entry->mm_id = mm_ptr_to_hash(mm);
> + __entry->curr = !!(current->mm == mm);
> __entry->member = member;
> __entry->size = (count << PAGE_SHIFT);
> ),
>
> - TP_printk("member=%d size=%ldB",
> + TP_printk("mm_id=%u curr=%d member=%d size=%ldB",
> + __entry->mm_id,
> + __entry->curr,
> __entry->member,
> __entry->size)
> );
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index dee8fc467fcf..401baaac1813 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
> early_initcall(initialize_ptr_random);
>
> /* Maps a pointer to a 32 bit unique identifier. */
> +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> +{
> + const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
> + unsigned long hashval;
> +
> + if (static_branch_unlikely(¬_filled_random_ptr_key))
> + return -EAGAIN;
> +
> +#ifdef CONFIG_64BIT
> + hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> + /*
> + * Mask off the first 32 bits, this makes explicit that we have
> + * modified the address (and 32 bits is plenty for a unique ID).
> + */
> + hashval = hashval & 0xffffffff;
> +#else
> + hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> +#endif
> + *hashval_out = hashval;
> + return 0;
> +}
> +
> static char *ptr_to_id(char *buf, char *end, const void *ptr,
> struct printf_spec spec)
> {
> const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
> unsigned long hashval;
> + int ret;
>
> /* When debugging early boot use non-cryptographically secure hash. */
> if (unlikely(debug_boot_weak_hash)) {
> @@ -773,22 +796,13 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
> return pointer_string(buf, end, (const void *)hashval, spec);
> }
>
> - if (static_branch_unlikely(¬_filled_random_ptr_key)) {
> + ret = ptr_to_hashval(ptr, &hashval);
> + if (ret) {
This is a hot path (used in trace_printk()), and you just turned a
static_branch into a function call.
Can we make a __ptr_to_hashval() static inline, and have
ptr_to_hashval() call that, but use the static inline here, where the
static_branch gets called directly here?
-- Steve
> spec.field_width = 2 * sizeof(ptr);
> /* string length must be less than default_width */
> return error_string(buf, end, str, spec);
> }
>
> -#ifdef CONFIG_64BIT
> - hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> - /*
> - * Mask off the first 32 bits, this makes explicit that we have
> - * modified the address (and 32 bits is plenty for a unique ID).
> - */
> - hashval = hashval & 0xffffffff;
> -#else
> - hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> -#endif
> return pointer_string(buf, end, (const void *)hashval, spec);
> }
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 7596d625ebd1..d3c9784e6dc1 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -154,9 +154,9 @@ static int __init init_zero_pfn(void)
> }
> core_initcall(init_zero_pfn);
>
> -void mm_trace_rss_stat(int member, long count)
> +void mm_trace_rss_stat(struct mm_struct *mm, int member, long count)
> {
> - trace_rss_stat(member, count);
> + trace_rss_stat(mm, member, count);
> }
>
> #if defined(SPLIT_RSS_COUNTING)
On Wed, Nov 13, 2019 at 03:38:16PM -0500, Steven Rostedt wrote:
[snip]
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index dee8fc467fcf..401baaac1813 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
> > early_initcall(initialize_ptr_random);
> >
> > /* Maps a pointer to a 32 bit unique identifier. */
> > +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> > +{
> > + const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
> > + unsigned long hashval;
> > +
> > + if (static_branch_unlikely(¬_filled_random_ptr_key))
> > + return -EAGAIN;
> > +
> > +#ifdef CONFIG_64BIT
> > + hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> > + /*
> > + * Mask off the first 32 bits, this makes explicit that we have
> > + * modified the address (and 32 bits is plenty for a unique ID).
> > + */
> > + hashval = hashval & 0xffffffff;
> > +#else
> > + hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> > +#endif
> > + *hashval_out = hashval;
> > + return 0;
> > +}
> > +
> > static char *ptr_to_id(char *buf, char *end, const void *ptr,
> > struct printf_spec spec)
> > {
> > const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
> > unsigned long hashval;
> > + int ret;
> >
> > /* When debugging early boot use non-cryptographically secure hash. */
> > if (unlikely(debug_boot_weak_hash)) {
> > @@ -773,22 +796,13 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
> > return pointer_string(buf, end, (const void *)hashval, spec);
> > }
> >
> > - if (static_branch_unlikely(¬_filled_random_ptr_key)) {
> > + ret = ptr_to_hashval(ptr, &hashval);
> > + if (ret) {
>
> This is a hot path (used in trace_printk()), and you just turned a
> static_branch into a function call.
>
> Can we make a __ptr_to_hashval() static inline, and have
> ptr_to_hashval() call that, but use the static inline here, where the
> static_branch gets called directly here?
Sure, like this?
---8<-----------------------
From: Joel Fernandes <[email protected]>
Subject: [PATCH] vsprintf: Inline call to ptr_to_hashval
There is concern that ptr_to_hashval not being inlined can cause performance
issues (unlike before where it was a static branch) with trace_printk being a
hot path for it. Just create an inline version called __ptr_to_hashval(), and
have the actual ptr_to_hashval() call it.
Link: http://lore.kernel.org/r/[email protected]
Suggested-by: Steven Rostedt <[email protected]>
Signed-off-by: Joel Fernandes <[email protected]>
---
lib/vsprintf.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 405a8ca220a0..7c488a1ce318 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -761,7 +761,7 @@ static int __init initialize_ptr_random(void)
early_initcall(initialize_ptr_random);
/* Maps a pointer to a 32 bit unique identifier. */
-int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
+static inline int __ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
{
unsigned long hashval;
@@ -782,6 +782,11 @@ int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
return 0;
}
+int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
+{
+ return __ptr_to_hashval(ptr, hashval_out);
+}
+
static char *ptr_to_id(char *buf, char *end, const void *ptr,
struct printf_spec spec)
{
@@ -795,7 +800,7 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
return pointer_string(buf, end, (const void *)hashval, spec);
}
- ret = ptr_to_hashval(ptr, &hashval);
+ ret = __ptr_to_hashval(ptr, &hashval);
if (ret) {
spec.field_width = 2 * sizeof(ptr);
/* string length must be less than default_width */
--
2.24.0.rc1.363.gb1bccd3e3d-goog
On Thu, 14 Nov 2019 11:46:22 -0500
Joel Fernandes <[email protected]> wrote:
> > Can we make a __ptr_to_hashval() static inline, and have
> > ptr_to_hashval() call that, but use the static inline here, where the
> > static_branch gets called directly here?
>
> Sure, like this?
Yep.
-- Steve