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

[Ahmed S. Darwish]

On Mon, Jul 06, 2020 at 11:21:48PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 30, 2020 at 07:44:38AM +0200, Ahmed S. Darwish wrote:
> > +#include <linux/seqlock_types_internal.h>
>
> Why? why not include those lines directly here? Having it in a separate
> file actually makes it harder to read.
>

The seqlock_types_internal.h file contains mostly a heavy sequence of
typeof() branching macros, which is not related to the core logic of
sequence counters or sequential locks:

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

#define __associated_lock_exists_and_is_preemptible(s)
({
	bool ret;

	/* No associated lock for these */
	if (__same_type(*(s), seqcount_t) ||
	    __same_type(*(s), seqcount_irq_t)) {
		ret = false;
	} else if (__same_type(*(s), seqcount_spinlock_t)	||
		   __same_type(*(s), seqcount_raw_spinlock_t)	||
		   __same_type(*(s), seqcount_rwlock_t)) {
		ret = false;
	} else if (__same_type(*(s), seqcount_mutex_t)		||
		   __same_type(*(s), seqcount_ww_mutex_t)) {
		ret = true;
	} else
		BUILD_BUG_ON_MSG(1, "Unknown seqcount type");

	ret;
})

#define __assert_seqcount_write_section_is_protected(s)
do {
	/* Can't assert anything with plain seqcount_t */
	if (__same_type(*(s), seqcount_t))
		break;

	if (__same_type(*(s), seqcount_spinlock_t))
		lockdep_assert_held(((seqcount_spinlock_t *)(s))->lock);
	else if (__same_type(*(s), seqcount_raw_spinlock_t))
		lockdep_assert_held(((seqcount_raw_spinlock_t *)(s))->lock);
	else if (__same_type(*(s), seqcount_rwlock_t))
		lockdep_assert_held_write(((seqcount_rwlock_t *)(s))->lock);
	else if (__same_type(*(s), seqcount_mutex_t))
		lockdep_assert_held(((seqcount_mutex_t *)(s))->lock);
	else if (__same_type(*(s), seqcount_ww_mutex_t))
		lockdep_assert_held(&((seqcount_ww_mutex_t *)(s))->lock->base);
	else if (__same_type(*(s), seqcount_irq_t))
		lockdep_assert_irqs_disabled();
	else
		BUILD_BUG_ON_MSG(1, "Unknown seqcount type");
} while (0)

and so on and so forth. With the final patch that enables PREEMPT_RT
support (not yet submitted upstream), this file gets even more fugly:

#define __rt_lock_unlock_associated_sleeping_lock(s)
do {
	if (__same_type(*(s), seqcount_t)  ||
	    __same_type(*(s), seqcount_raw_spinlock_t))	{
		break;
	}

	if (__same_type(*(s), seqcount_spinlock_t)) {
		spin_lock(((seqcount_spinlock_t *) s)->lock);
		spin_unlock(((seqcount_spinlock_t *) s)->lock);
	} else if (__same_type(*(s), seqcount_rwlock_t)) {
		read_lock(((seqcount_rwlock_t *) s)->lock);
		read_unlock(((seqcount_rwlock_t *) s)->lock);
	} else if (__same_type(*(s), seqcount_mutex_t)) {
		mutex_lock(((seqcount_mutex_t *) s)->lock);
		mutex_unlock(((seqcount_mutex_t *) s)->lock);
	} else if (__same_type(*(s), seqcount_ww_mutex_t)) {
		ww_mutex_lock(((seqcount_ww_mutex_t *) s)->lock, NULL);
		ww_mutex_unlock(((seqcount_ww_mutex_t *) s)->lock);
	} else
		BUILD_BUG_ON_MSG(1, "Unknown seqcount type");
} while (0)

and even more macros not included here for brevity.

IMHO it makes sense to isolate these "not core logic" parts. Adding all
of this to plain seqlock.h makes it almost unreadable.

Thanks,

--
Ahmed S. Darwish
Linutronix GmbH