Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: Eric Biggers <ebiggers@kernel.org>,
	linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org,
	"Paul E . McKenney" <paulmck@kernel.org>,
	linux-fsdevel@vger.kernel.org, Akira Yokosawa <akiyks@gmail.com>,
	Andrea Parri <parri.andrea@gmail.com>,
	Boqun Feng <boqun.feng@gmail.com>,
	Daniel Lustig <dlustig@nvidia.com>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	Dave Chinner <david@fromorbit.com>,
	David Howells <dhowells@redhat.com>,
	Jade Alglave <j.alglave@ucl.ac.uk>,
	Luc Maranget <luc.maranget@inria.fr>,
	Nicholas Piggin <npiggin@gmail.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH] tools/memory-model: document the "one-time init" pattern
Date: Mon, 27 Jul 2020 17:59:17 +0100
Message-ID: <20200727165917.GN23808@casper.infradead.org> (raw)
In-Reply-To: <20200727163149.GD1468275@rowland.harvard.edu>

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;
}

  reply index

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-17  4:44 Eric Biggers
2020-07-17  5:49 ` Sedat Dilek
2020-07-17 12:35 ` Matthew Wilcox
2020-07-17 14:26 ` Alan Stern
2020-07-17 17:47 ` Matthew Wilcox
2020-07-17 17:51   ` Alan Stern
2020-07-18  1:02     ` Eric Biggers
2020-07-27 12:51       ` Matthew Wilcox
2020-07-17 21:05   ` Darrick J. Wong
2020-07-18  0:44   ` Darrick J. Wong
2020-07-18  1:38   ` Eric Biggers
2020-07-18  2:13     ` Matthew Wilcox
2020-07-18  5:28       ` Eric Biggers
2020-07-18 14:35         ` Alan Stern
2020-07-20  2:07         ` Dave Chinner
2020-07-20  9:00           ` Peter Zijlstra
2020-07-27 15:17         ` Alan Stern
2020-07-27 15:28           ` Matthew Wilcox
2020-07-27 16:01             ` Paul E. McKenney
2020-07-27 16:31             ` Alan Stern
2020-07-27 16:59               ` Matthew Wilcox [this message]
2020-07-27 19:13                 ` Alan Stern
2020-07-17 20:53 ` Darrick J. Wong
2020-07-18  0:58   ` Eric Biggers
2020-07-18  1:25     ` Alan Stern
2020-07-18  1:40       ` Matthew Wilcox
2020-07-18  2:00       ` Dave Chinner
2020-07-18 14:21         ` Alan Stern
2020-07-18  2:00       ` Eric Biggers
2020-07-18  1:42 ` Dave Chinner
2020-07-18 14:08   ` Alan Stern
2020-07-20  1:33     ` Dave Chinner
2020-07-20 14:52       ` Alan Stern
2020-07-20 15:37         ` Darrick J. Wong
2020-07-20 15:39         ` Matthew Wilcox
2020-07-20 16:04           ` Paul E. McKenney
2020-07-20 16:48             ` peterz
2020-07-20 22:06               ` Paul E. McKenney
2020-07-20 16:12           ` Alan Stern

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200727165917.GN23808@casper.infradead.org \
    --to=willy@infradead.org \
    --cc=akiyks@gmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=darrick.wong@oracle.com \
    --cc=david@fromorbit.com \
    --cc=dhowells@redhat.com \
    --cc=dlustig@nvidia.com \
    --cc=ebiggers@kernel.org \
    --cc=j.alglave@ucl.ac.uk \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luc.maranget@inria.fr \
    --cc=npiggin@gmail.com \
    --cc=parri.andrea@gmail.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=stern@rowland.harvard.edu \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lkml.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lkml.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git