From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752593AbbKQLVz (ORCPT <rfc822;w@1wt.eu>);
	Tue, 17 Nov 2015 06:21:55 -0500
Received: from casper.infradead.org ([85.118.1.10]:58761 "EHLO
	casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751160AbbKQLVy (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 17 Nov 2015 06:21:54 -0500
Date: Tue, 17 Nov 2015 12:21:49 +0100
From: Peter Zijlstra <peterz@infradead.org>
To: Byungchul Park <byungchul.park@lge.com>
Cc: mingo@kernel.org, linux-kernel@vger.kernel.org, yuyang.du@intel.com,
        pjt@google.com, efault@gmx.de, tglx@linutronix.de
Subject: Re: [PATCH v4 3/3] sched: optimize migration by forcing rmb() and
 updating to be called once
Message-ID: <20151117112149.GV3816@twins.programming.kicks-ass.net>
References: <1445616981-29904-1-git-send-email-byungchul.park@lge.com>
 <1445616981-29904-4-git-send-email-byungchul.park@lge.com>
 <20151109132914.GK17308@twins.programming.kicks-ass.net>
 <20151110010905.GD4164@byungchulpark-X58A-UD3R>
 <20151110121647.GZ17308@twins.programming.kicks-ass.net>
 <20151110235147.GE4164@byungchulpark-X58A-UD3R>
 <20151116125351.GT17308@twins.programming.kicks-ass.net>
 <20151117004416.GC18234@byungchulpark-X58A-UD3R>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20151117004416.GC18234@byungchulpark-X58A-UD3R>
User-Agent: Mutt/1.5.21 (2012-12-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Nov 17, 2015 at 09:44:16AM +0900, Byungchul Park wrote:

> > So currently, set_task_cpu() is serialized by:
> > 
> >  - p->pi_lock; on wakeup
> >  - rq->lock; otherwise
> > 
> > (see the #ifdef CONFIG_LOCKDEP comment in set_task_cpu())
> 
> I already read the comment.. Then do you mean the comment above
> migrate_task_rq_fair() is wrong and should be fixed? 

Looks that way, I'm not sure we always hold pi_lock there. But I'm low
on sleep, so I could have overlooked something.

See for example move_queued_task(), we call set_task_cpu() with rq->lock
held, but no pi_lock.

> I thought the comment above migrate_task_rq_fair() is correct rather
> than CONFIG_LOCKDEP comment in set_task_cpu(), when I read it. I think
> these two comments are conflict each other a little bit, so one of
> those should be fixed.

Agreed.

> * the comment above migrate_task_rq_fair() describes it like,
> Caller SHOULD HOLD (&p->pi_lock)
> 
> * the CONFIG_LOCKDEP comment in set_task_cpu() describes it like,
> Caller SHOULD HOLD (&p->pi_lock || &rq->lock)

Indeed.

> > 
> > This means that sched_class::migrate_task() cannot indeed rely on
> > rq->lock for full serialization, however it still means that
> > task_rq_lock() will fully serialize against the thing.
> 
> Yes I also think this is true.
> 
> > 
> > By changing this, it no longer will.
> 
> ???

I meant, if you call __set_task_cpu() before
sched_class::migrate_task_rq(), in that case task_rq_lock() will no
longer fully serialize against set_task_cpu().

Because once you've called __set_task_cpu(), task_rq_lock() will acquire
the _other_ rq->lock. And we cannot rely on our rq->lock to serialize
things.