Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Eric Biggers <ebiggers@kernel.org>
Cc: Matthew Wilcox <willy@infradead.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>,
	Alan Stern <stern@rowland.harvard.edu>,
	Andrea Parri <parri.andrea@gmail.com>,
	Boqun Feng <boqun.feng@gmail.com>,
	Daniel Lustig <dlustig@nvidia.com>,
	"Darrick J . Wong" <darrick.wong@oracle.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, 20 Jul 2020 12:07:31 +1000	[thread overview]
Message-ID: <20200720020731.GQ5369@dread.disaster.area> (raw)
In-Reply-To: <20200718052818.GF2183@sol.localdomain>

On Fri, Jul 17, 2020 at 10:28:18PM -0700, Eric Biggers wrote:
> What do people think about the following instead?  (Not proofread / tested yet,
> so please comment on the high-level approach, not minor mistakes :-) )

No huge long macros, please.

We don't accept people writing long complex static inline functions,
so for the same reasons it is not acceptable to write long complex
macros.  Especially ones that use variable arguments and embed
invisible locking within them.

The whole serialisation/atomic/ordering APIs have fallen badly off
the macro cliff, to the point where finding out something as simple
as the order of parameters passed to cmpxchg and what semantics it
provides requires macro-spelunking 5 layers deep to find the generic
implementation function that contains a comment describing what it
does....

That's yet another barrier to understanding what all the different
synchronisation primitives do.

....

> In the fs/direct-io.c case we'd use:
> 
> int sb_init_dio_done_wq(struct super_block *sb)
> {
> 	static DEFINE_MUTEX(sb_init_dio_done_mutex);
> 
> 	return INIT_ONCE_PTR(&sb->s_dio_done_wq, &sb_init_dio_done_mutex,
> 			     alloc_workqueue,
> 			     "dio/%s", WQ_MEM_RECLAIM, 0, sb->s_id);
> }

Yeah, that's pretty horrible...

I *much* prefer an API like Willy's suggested to somethign like
this. Just because you can hide all sorts of stuff in macros doesn't
mean you should.

> The only part I really don't like is the way arguments are passed to the
> alloc_func.  We could also make it work like the following, though it would
> break the usual rules since it looks like the function call is always executed,
> but it's not:
> 
> 	return INIT_ONCE_PTR(&sb->s_dio_done_wq, &sb_init_dio_done_mutex,
> 			     alloc_workqueue("dio/%s", WQ_MEM_RECLAIM, 0,
> 					     sb->s_id));

Yeah, that's even worse. Code that does not do what it looks like it
should needs to be nuked from orbit.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  parent reply	other threads:[~2020-07-20  2:07 UTC|newest]

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 [this message]
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
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=20200720020731.GQ5369@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=akiyks@gmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=darrick.wong@oracle.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 \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH] tools/memory-model: document the "one-time init" pattern' \
    /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

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox