Re: [PATCH v3 06/20] seqlock: Extend seqcount API with associated locks

Ahmed S. Darwish
2020-07-08T10:33:20Z

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