Re: [PATCH] rss_stat: Add support to detect RSS updates of external mm

From: Joel Fernandes <joel@joelfernandes.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org, Ioannis Ilkos <ilkos@google.com>,
	minchan@google.com, primiano@google.com, fmayer@google.com,
	hjd@google.com, joaodias@google.com, lalitm@google.com,
	rslawik@google.com, sspatil@google.com, timmurray@google.com,
	Andrew Morton <akpm@linux-foundation.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Changbin Du <changbin.du@intel.com>,
	Ingo Molnar <mingo@redhat.com>, Joe Perches <joe@perches.com>,
	Kees Cook <keescook@chromium.org>,
	linux-mm@kvack.org, Michal Hocko <mhocko@suse.com>,
	Petr Mladek <pmladek@suse.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Stephen Rothwell <sfr@canb.auug.org.au>
Subject: Re: [PATCH] rss_stat: Add support to detect RSS updates of external mm
Date: Thu, 14 Nov 2019 11:46:22 -0500	[thread overview]
Message-ID: <20191114164622.GC233237@google.com> (raw)
In-Reply-To: <20191113153816.14b95acd@gandalf.local.home>

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(&not_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(&not_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 <joelaf@google.com>
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/20191113153816.14b95acd@gandalf.local.home
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Joel Fernandes <joelaf@google.com>
---
 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

