On Thu, Apr 12, 2018 at 11:12:17AM +0200, Peter Zijlstra wrote:
> On Thu, Apr 12, 2018 at 10:12:33AM +0800, Boqun Feng wrote:
> > A trivial fix/hack would be adding local_irq_disable() and
> > local_irq_enable() around srcu_lock_sync() like:
> > 
> > 	static inline void srcu_lock_sync(struct lockdep_map *map)
> > 	{
> > 		local_irq_disable();
> > 		lock_map_acquire(map);
> > 		lock_map_release(map);
> > 		local_irq_enable();
> > 	}
> > 
> > However, it might be better, if lockdep could provide some annotation
> > API for such an empty critical section to say the grap-and-drop is
> > atomic. Something like:
> > 
> > 	/*
> > 	 * Annotate a wait point for all previous critical section to
> > 	 * go out.
> > 	 * 
> > 	 * This won't make @map a irq unsafe lock, no matter it's called
> > 	 * w/ or w/o irq disabled.
> > 	 */
> > 	lock_wait_unlock(struct lockdep_map *map, ..)
> > 
> > And in this primitive, we do something similar like
> > lock_acquire()+lock_release(). This primitive could be used elsewhere,
> > as I bebieve we have several empty grab-and-drop critical section for
> > lockdep annotations, e.g. in start_flush_work().
> > 
> > Thoughts?
> > 
> > This cerntainly requires a bit more work, in the meanwhile, I will add
> > another self testcase which has a srcu_read_lock() called in irq.
> 
> Yeah, I've never really bothered to clean those things up, but I don't
> see any reason to stop you from doing it ;-)
> 
> As to the initial pattern with disabling IRQs, I think I've seen code
> like that before, and in general performance isn't a top priority

Yeah, I saw we used that pattern in del_timer_sync()

> (within reason) when you're running lockdep kernels, so I've usually let
> it be.

Turns out it's not very hard to write a working version of
lock_wait_unlock() ;-) Just call __lock_acquire() and __lock_release()
back-to-back with the @hardirqoff for __lock_acquire() to be 1:

	/*
	 * lock_sync() - synchronize with all previous critical sections to finish.
	 *
	 * Simply a acquire+release annotation with hardirqoff is true, because no lock
	 * is actually held, so this annotaion alone is safe to be interrupted as if
	 * irqs are off
	 */
	void lock_sync(struct lockdep_map *lock, unsigned subclass, int read,
		       int check, struct lockdep_map *nest_lock, unsigned long ip)
	{
		unsigned long flags;

		if (unlikely(current->lockdep_recursion))
			return;

		raw_local_irq_save(flags);
		check_flags(flags);

		current->lockdep_recursion = 1;
		__lock_acquire(lock, subclass, 0, read, check, 1, nest_lock, ip, 0, 0);
		if (__lock_release(lock, 0, ip))
			check_chain_key(current);

		current->lockdep_recursion = 0;
		raw_local_irq_restore(flags);
	}
	EXPORT_SYMBOL_GPL(lock_sync);

I rename as lock_sync(), because most of the time, we annotate with this
for a "sync point" with other critical sections. We can avoid some
overhead if we refactor __lock_acquire() and __lock_release() with some
helper functions, but I think this version is good enough for now, at
least better than disabling IRQs around lock_map_acquire() +
lock_map_release() ;-)

Thoughts?

Regards,
Boqun