linux-kernel - Re: [PATCH] tools/memory-model: document the "one-time init" pattern

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200727165917.GN23808@casper.infradead.org>
Date:   Mon, 27 Jul 2020 17:59:17 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Eric Biggers <ebiggers@...nel.org>, linux-kernel@...r.kernel.org,
        linux-arch@...r.kernel.org,
        "Paul E . McKenney" <paulmck@...nel.org>,
        linux-fsdevel@...r.kernel.org, Akira Yokosawa <akiyks@...il.com>,
        Andrea Parri <parri.andrea@...il.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Daniel Lustig <dlustig@...dia.com>,
        "Darrick J . Wong" <darrick.wong@...cle.com>,
        Dave Chinner <david@...morbit.com>,
        David Howells <dhowells@...hat.com>,
        Jade Alglave <j.alglave@....ac.uk>,
        Luc Maranget <luc.maranget@...ia.fr>,
        Nicholas Piggin <npiggin@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Will Deacon <will@...nel.org>
Subject: Re: [PATCH] tools/memory-model: document the "one-time init" pattern

On Mon, Jul 27, 2020 at 12:31:49PM -0400, Alan Stern wrote:
> On Mon, Jul 27, 2020 at 04:28:27PM +0100, Matthew Wilcox wrote:
> > On Mon, Jul 27, 2020 at 11:17:46AM -0400, Alan Stern wrote:
> > > Given a type "T", an object x of type pointer-to-T, and a function
> > > "func" that takes various arguments and returns a pointer-to-T, the
> > > accepted API for calling func once would be to create once_func() as
> > > follows:
> > > 
> > > T *once_func(T **ppt, args...)
> > > {
> > > 	static DEFINE_MUTEX(mut);
> > > 	T *p;
> > > 
> > > 	p = smp_load_acquire(ppt);	/* Mild optimization */
> > > 	if (p)
> > > 		return p;
> > > 
> > > 	mutex_lock(mut);
> > > 	p = smp_load_acquire(ppt);
> > > 	if (!p) {
> > > 		p = func(args...);
> > > 		if (!IS_ERR_OR_NULL(p))
> > > 			smp_store_release(ppt, p);
> > > 	}
> > > 	mutex_unlock(mut);
> > > 	return p;
> > > }
> > > 
> > > Users then would have to call once_func(&x, args...) and check the
> > > result.  Different x objects would constitute different "once"
> > > domains.
> > [...]
> > > In fact, the only drawback I can think of is that because this relies
> > > on a single mutex for all the different possible x's, it might lead to
> > > locking conflicts (if func had to call once_func() recursively, for
> > > example).  In most reasonable situations such conflicts would not
> > > arise.
> > 
> > Another drawback for this approach relative to my get_foo() approach
> > upthread is that, because we don't have compiler support, there's no
> > enforcement that accesses to 'x' go through once_func().  My approach
> > wraps accesses in a deliberately-opaque struct so you have to write
> > some really ugly code to get at the raw value, and it's just easier to
> > call get_foo().
> 
> Something like that could be included in once_func too.  It's relatively 
> tangential to the main point I was making, which was to settle on an 
> overall API and discuss how it should be described in recipes.txt.

Then I think you're trying to describe something which is too complicated
because it's overly general.  I don't think device drivers should contain
"smp_load_acquire" and "smp_store_release".  Most device driver authors
struggle with spinlocks and mutexes.

The once_get() / once_store() API:

struct foo *get_foo(gfp_t gfp)
{
	static struct once_pointer my_foo;
	struct foo *foop;

	foop = once_get(&my_foo);
	if (foop)
		return foop;

	foop = alloc_foo(gfp);
	if (foop && !once_store(&my_foo, foop)) {
		free_foo(foop);
		foop = once_get(&my_foo);
	}

	return foop;
}

is easy to understand.  There's no need to talk about acquire and release
semantics, barriers, reordering, ... it all just works in the obvious way
that it's written.

If you want to put a mutex around all this, you can:

struct foo *get_foo(gfp_t gfp)
{
	static DEFINE_MUTEX(m);
	static struct init_once_pointer my_foo;
	struct foo *foop;

	foop = once_get(&my_foo);
	if (foop)
		return foop;

	mutex_lock(&m);
	foop = once_get(&my_foo);
	if (!foop) {
		foop = alloc_foo(gfp);
		if (foop && !once_store(&my_foo, foop)) {
			free_foo(foop);
			foop = once_get(&my_foo);
		}
	}
	mutex_unlock(&m);

	return foop;
}