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, 29 Nov 2018 13:30:17 -0800
From:   Davidlohr Bueso <dave@stgolabs.net>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Yongji Xie <elohimes@gmail.com>, mingo@redhat.com,
        will.deacon@arm.com, linux-kernel@vger.kernel.org,
        xieyongji@baidu.com, zhangyu31@baidu.com, liuqi16@baidu.com,
        yuanlinsi01@baidu.com, nixun@baidu.com, lilin24@baidu.com
Subject: Re: [RFC] locking/rwsem: Avoid issuing wakeup before setting the
 reader waiter to nil
Message-ID: <20181129213017.v3eljor54lfpoug2@linux-r8p5>
References: <1543495830-2644-1-git-send-email-xieyongji@baidu.com>
 <20181129131232.GN2131@hirez.programming.kicks-ass.net>
 <5598cd71-c3c8-d6ef-eb30-777cf901a2ef@redhat.com>
 <20181129160627.GU2131@hirez.programming.kicks-ass.net>
 <8cc45695-b325-a219-8b46-d5da6ddfdd63@redhat.com>
 <20181129172700.GA11632@hirez.programming.kicks-ass.net>
 <20181129180828.GA11650@hirez.programming.kicks-ass.net>
 <729ceddb-dd9a-ec2a-f74e-03fa4d7e65e8@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <729ceddb-dd9a-ec2a-f74e-03fa4d7e65e8@redhat.com>
1;5002;0cTo: Waiman Long <longman@redhat.com>
User-Agent: NeoMutt/20180323
To:     unlisted-recipients:; (no To-header on input)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, 29 Nov 2018, Waiman Long wrote:

>That can be costly for x86 which will now have 2 locked instructions.

Yeah, and when used as an actual queue we should really start to notice.
Some users just have a single task in the wake_q because avoiding the cost
of wake_up_process() with locks held is significant.

How about instead of adding the barrier before the cmpxchg, we do it
in the failed branch, right before we return. This is the uncommon
path.

Thanks,
Davidlohr

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 091e089063be..0d844a18a9dc 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -408,8 +408,14 @@ void wake_q_add(struct wake_q_head *head, struct task_struct *task)
 	 * This cmpxchg() executes a full barrier, which pairs with the full
 	 * barrier executed by the wakeup in wake_up_q().
 	 */
-	if (cmpxchg(&node->next, NULL, WAKE_Q_TAIL))
+	if (cmpxchg(&node->next, NULL, WAKE_Q_TAIL)) {
+		/*
+		 * Ensure, that when the cmpxchg() fails, the corresponding
+		 * wake_up_q() will observe our prior state.
+		 */
+		smp_mb__after_atomic();
 		return;
+	}
 
 	get_task_struct(task);