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] [day] [month] [year] [list]

Date:   Mon, 27 Jul 2020 15:13:47 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Matthew Wilcox <willy@...radead.org>
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 05:59:17PM +0100, Matthew Wilcox wrote:
> 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.

Then I didn't explain my proposal clearly enough.  It doesn't require 
device driver authors to know anything about smp_load_acquire, 
smp_store_release, spinlocks, or mutexes.

Suppose an author wants to allocate and initialize a struct foo exactly 
once.  Then the driver code would contain something like this:

struct foo *foop;

static struct foo *alloc_foo(gfp_t gfp)
{
	... allocate and initialize ...
}

MAKE_ONCE_FUNC(struct foo, alloc_foo, (gfp_t gfp), (gfp))

The code to use it is:

	struct foo *p = once_alloc_foo(&foop, GFP_KERNEL);

If you don't like the global pointer, encapsulate it as follows:

struct foo *get_foo(grp_t gfp)
{
	static struct foo *foop;

	return once_alloc_foo(&foop, gfp);
}

and have users call get_foo instead of once_alloc_foo.

It's hard to imagine this getting much simpler.

> 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.

The MAKE_ONCE_FUNC API is just as easy to understand and requires less 
boilerplate.  It's type-safe whereas your once_pointer structures 
aren't.  And it's more general, in the sense that it provides a way to 
call a function only once, as opposed to a way to store a pointer only 
once.

Alan Stern