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
signature.asc
Description: PGP signature
