Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 16 Apr 2019 20:32:16 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Waiman Long <longman@redhat.com>
Cc:     Ingo Molnar <mingo@redhat.com>, Will Deacon <will.deacon@arm.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org, x86@kernel.org,
        Davidlohr Bueso <dave@stgolabs.net>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        huang ying <huang.ying.caritas@gmail.com>
Subject: Re: [PATCH v4 07/16] locking/rwsem: Implement lock handoff to
 prevent lock starvation
Message-ID: <20190416183216.GV4038@hirez.programming.kicks-ass.net>
References: <20190413172259.2740-1-longman@redhat.com>
 <20190413172259.2740-8-longman@redhat.com>
 <20190416154937.GL12232@hirez.programming.kicks-ass.net>
 <d10586ae-cd86-2992-766b-1ba4a4807b6a@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d10586ae-cd86-2992-766b-1ba4a4807b6a@redhat.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.