linux-kernel - Re: [PATCH v4 07/16] locking/rwsem: Implement lock handoff to prevent lock starvation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190416183216.GV4038@hirez.programming.kicks-ass.net>
Date:   Tue, 16 Apr 2019 20:32:16 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Waiman Long <longman@...hat.com>
Cc:     Ingo Molnar <mingo@...hat.com>, Will Deacon <will.deacon@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        Davidlohr Bueso <dave@...olabs.net>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        huang ying <huang.ying.caritas@...il.com>
Subject: Re: [PATCH v4 07/16] locking/rwsem: Implement lock handoff to
 prevent lock starvation

On Tue, Apr 16, 2019 at 02:16:11PM -0400, Waiman Long wrote:

> >> @@ -665,9 +751,34 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
> >>  			lockevent_inc(rwsem_sleep_writer);
> >>  			set_current_state(state);
> >>  			count = atomic_long_read(&sem->count);
> >> +
> >> +			if ((wstate == WRITER_NOT_FIRST) &&
> >> +			    rwsem_waiter_is_first(sem, &waiter))
> >> +				wstate = WRITER_FIRST;
> >> +
> >> +			if (!RWSEM_COUNT_LOCKED(count))
> >> +				break;
> >> +
> >> +			/*
> >> +			 * An RT task sets the HANDOFF bit immediately.
> >> +			 * Non-RT task will wait a while before doing so.
> > Again, this describes what we already read the code to do; but doesn't
> > add anything.
> 
> Will remove that.
> 
> >> +			 *
> >> +			 * The setting of the handoff bit is deferred
> >> +			 * until rwsem_try_write_lock() is called.
> >> +			 */
> >> +			if ((wstate == WRITER_FIRST) && (rt_task(current) ||
> >> +			    time_after(jiffies, waiter.timeout))) {
> >> +				wstate = WRITER_HANDOFF;
> >> +				lockevent_inc(rwsem_wlock_handoff);
> >> +				/*
> >> +				 * Break out to call rwsem_try_write_lock().
> >> +				 */
> > Another exceedingly useful comment.
> >
> >> +				break;
> >> +			}
> >> +		}
> >>  
> >>  		raw_spin_lock_irq(&sem->wait_lock);
> >> +		count = atomic_long_read(&sem->count);
> >>  	}
> >>  	__set_current_state(TASK_RUNNING);
> >>  	list_del(&waiter.list);
> >> @@ -680,6 +791,12 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
> >>  	__set_current_state(TASK_RUNNING);
> >>  	raw_spin_lock_irq(&sem->wait_lock);
> >>  	list_del(&waiter.list);
> >> +	/*
> >> +	 * If handoff bit has been set by this waiter, make sure that the
> >> +	 * clearing of it is seen by others before proceeding.
> >> +	 */
> >> +	if (unlikely(wstate == WRITER_HANDOFF))
> >> +		atomic_long_add_return(-RWSEM_FLAG_HANDOFF,  &sem->count);
> > _AGAIN_ no explanation what so ff'ing ever.
> >
> > And why add_return() if you ignore the return value.
> >
> 
> OK, will remove those.

I'm not saying to remove them, although for at least the break one that
is fine. But do try and write comments that _add_ something to the code,
explain _why_ instead of state what (which we can trivially see from the
code).

Locking code in general is tricky enough, and comments are good, and
more comments are more good, but only if they explain why.