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:   Thu, 18 Apr 2019 10:52:27 +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 09/16] locking/rwsem: Ensure an RT task will not spin
 on reader
Message-ID: <20190418085227.GV12232@hirez.programming.kicks-ass.net>
References: <20190413172259.2740-1-longman@redhat.com>
 <20190413172259.2740-10-longman@redhat.com>
 <20190417131841.GF4038@hirez.programming.kicks-ass.net>
 <1c37b8f2-232e-7ced-0483-31c8c6a4ffd1@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1c37b8f2-232e-7ced-0483-31c8c6a4ffd1@redhat.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Apr 17, 2019 at 02:47:07PM -0400, Waiman Long wrote:
> >> @@ -566,13 +573,28 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
> >>  		}
> >>  
> >>  		/*
> >> -		 * When there's no owner, we might have preempted between the
> >> -		 * owner acquiring the lock and setting the owner field. If
> >> -		 * we're an RT task that will live-lock because we won't let
> >> -		 * the owner complete.
> >> +		 * An RT task cannot do optimistic spinning if it cannot
> >> +		 * be sure the lock holder is running or live-lock may
> >> +		 * happen if the current task and the lock holder happen
> >> +		 * to run in the same CPU.
> >> +		 *
> >> +		 * When there's no owner or is reader-owned, an RT task
> >> +		 * will stop spinning if the owner state is not a writer
> >> +		 * at the previous iteration of the loop. This allows the
> >> +		 * RT task to recheck if the task that steals the lock is
> >> +		 * a spinnable writer. If so, it can keeps on spinning.
> >> +		 *
> >> +		 * If the owner is a writer, the need_resched() check is
> >> +		 * done inside rwsem_spin_on_owner(). If the owner is not
> >> +		 * a writer, need_resched() check needs to be done here.
> >>  		 */
> >> -		if (!sem->owner && (need_resched() || rt_task(current)))
> >> -			break;
> >> +		if (owner_state != OWNER_WRITER) {
> >> +			if (need_resched())
> >> +				break;
> >> +			if (is_rt_task && (prev_owner_state != OWNER_WRITER))
> >> +				break;
> >> +		}
> >> +		prev_owner_state = owner_state;
> >>  
> >>  		/*
> >>  		 * The cpu_relax() call is a compiler barrier which forces
> > This patch confuses me mightily. I mean, I see what it does, but I can't
> > figure out why. The Changelog is just one big source of confusion.
> 
> Sorry for confusing you. If count and owner are separate, there is a
> time lag where the owner is NULL, but the lock is not free yet.

Right.

> Similarly, the lock could be free but another task may have stolen the
> lock if the waiter bit isn't set.

> In the former case,

(free)

> an extra iteration gives it more time for the lock holder to release
> the lock.


> In the latter case,

(stolen)

> if the new lock owner is a writer and set owner in time,
> the RT task can keep on spinning. Will clarify that in the commit log
> and the comment.

Blergh.. so by going around once extra, you hope ->owner will be set
again and we keep spinning. And this is actually measurable.

Yuck yuck yuck. I much prefer getting rid of that hole, as you do later
on in the series, that would avoid this complecity. Let me continue
reading...