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
