Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 8 Jul 2020 12:33:14 +0200
From:   "Ahmed S. Darwish" <a.darwish@linutronix.de>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        "Sebastian A. Siewior" <bigeasy@linutronix.de>,
        Steven Rostedt <rostedt@goodmis.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated
 locks
Message-ID: <20200708103314.GB4151780@debian-buster-darwi.lab.linutronix.de>
References: <20200630054452.3675847-1-a.darwish@linutronix.de>
 <20200630054452.3675847-7-a.darwish@linutronix.de>
 <20200706212148.GE5523@worktop.programming.kicks-ass.net>
 <20200707084024.GA4097637@debian-buster-darwi.lab.linutronix.de>
 <20200707130410.GO4800@hirez.programming.kicks-ass.net>
 <20200707143726.GO117543@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200707143726.GO117543@hirez.programming.kicks-ass.net>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 07, 2020 at 04:37:26PM +0200, Peter Zijlstra wrote:
>
> How's this? it removes a level of indirection and a bunch of repetition.

ACK, for the extra level of indirection removed.

> It's also more than 200 lines shorter.
...
> +#define __to_seqcount_t(s)	(seqcount_t *)(s)
...
> +#define read_seqcount_begin(s)	do_read_seqcount_begin(__to_seqcount_t(s))
> +
> +static inline unsigned do_read_seqcount_begin(const seqcount_t *s)
> +{
...

Hmm, the __to_seqcount_t(s) force cast is not good. It will break the
arguments type-safety of seqcount macros that do not have either:

    __associated_lock_is_preemptible() or
    __assert_associated_lock_held()

in their path. This basically includes all the read path macros, and
even some others (e.g. write_seqcount_invalidate()).

With the suggested force cast above, I can literally *pass anything* to
read_seqcount_begin() and friends, and the compiler won't say a thing.

So, I'll restore __to_seqcount_t(s) that to its original implementation:

/*
 * @s: pointer to seqcount_t or any of the seqcount_locktype_t variants
 */
#define __to_seqcount_t(s)						\
({									\
	seqcount_t *seq;						\
									\
	if (__same_type(*(s), seqcount_t))				\
		seq = (seqcount_t *)(s);				\
	else if (__same_type(*(s), seqcount_spinlock_t))		\
		seq = &((seqcount_spinlock_t *)(s))->seqcount;		\
	else if (__same_type(*(s), seqcount_raw_spinlock_t))		\
		seq = &((seqcount_raw_spinlock_t *)(s))->seqcount;	\
	else if (__same_type(*(s), seqcount_rwlock_t))			\
		seq = &((seqcount_rwlock_t *)(s))->seqcount;		\
	else if (__same_type(*(s), seqcount_mutex_t))			\
		seq = &((seqcount_mutex_t *)(s))->seqcount;		\
	else if (__same_type(*(s), seqcount_ww_mutex_t))		\
		seq = &((seqcount_ww_mutex_t *)(s))->seqcount;		\
	else								\
		BUILD_BUG_ON_MSG(1, "Unknown seqcount type");		\
									\
	seq;								\
})

Yes, I know, it's not the prettiest thing in the world, but I'd take
ugly over breaking the compiler type checks any day.

>
> It doesn't provide SEQCNT_LOCKTYPE_ZERO() for each LOCKTYPE, but you can
> use this one macro for any LOCKTYPE.
>

From call-sites perspectives, SEQCNT_SPINLOCK_ZERO() is much more
readable than SEQCNT_LOCKTYPE_ZERO() and so on. It only costs us 5 lines
*for all* the seqcount lock types. IMHO it's worth the trade-off.

>
> So I applied it all yesterday and tried to make sense of the resulting
> indirections and couldn't find stuff -- because it was hidding in
> another file.
>
> Specifically I disliked that *seqcount_t* naming and didn't see how it
> all connected.
>

Hmm, the idea was that write_seqcount_t_begin() and friends applies only
to plain old "seqcount_t". But, yes, I agree, it's confusing as hell.

The way you've organized the macros is much more readable, so thanks a
lot, I'll incorporate that in v4.

Kind regards,

--
Ahmed S. Darwish
Linutronix GmbH