Re: [PATCH] break_lock forever broken

Hugh Dickins <hugh@veritas.com> wrote:
>
> lock->break_lock is set when a lock is contended, but cleared only in
>  cond_resched_lock.  Users of need_lockbreak (journal_commit_transaction,
>  copy_pte_range, unmap_vmas) don't necessarily use cond_resched_lock on it.
> 
>  So, if the lock has been contended at some time in the past, break_lock
>  remains set thereafter, and the fastpath keeps dropping lock unnecessarily.
>  Hanging the system if you make a change like I did, forever restarting a
>  loop before making any progress.
> 
>  Should it be cleared when contending to lock, just the other side of the
>  cpu_relax loop?  No, that loop is preemptible, we don't want break_lock
>  set all the while the contender has been preempted.  It should be cleared
>  when we unlock - any remaining contenders will quickly set it again.
> 
>  So cond_resched_lock's spin_unlock will clear it, no need for it to do
>  that; and use need_lockbreak there too, preferring optimizer to #ifdefs.
> 
>  Or would you prefer the few need_lockbreak users to clear it in advance?
>  Less overhead, more errorprone.

This patch causes a CONFIG_PREEMPT=y, CONFIG_PREEMPT_BKL=y,
CONFIG_DEBUG_PREEMPT=y kernel on a ppc64 G5 to hang immediately after
displaying the penguins, but apparently not before having set the hardware
clock backwards 101 years.

After having carefully reviewed the above description and having decided
that these effects were not a part of the patch's design intent I have
temporarily set it aside, thanks.