Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 7 Nov 2019 13:07:51 -0500
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Petr Mladek <pmladek@suse.com>
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>,
        "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>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH] rss_stat: Add support to detect RSS updates of external
 mm
Message-ID: <20191107180751.GA3846@google.com>
References: <20191106024452.81923-1-joel@joelfernandes.org>
 <20191106085959.ae2dgvmny3njnk7n@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191106085959.ae2dgvmny3njnk7n@pathway.suse.cz>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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(&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;
> > +}
> 
> Best Regards,
> Petr