Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Apr 2023 22:35:32 +0800
From:   Aaron Lu <aaron.lu@intel.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC:     Peter Zijlstra <peterz@infradead.org>,
        <linux-kernel@vger.kernel.org>, Olivier Dion <odion@efficios.com>,
        <michael.christie@oracle.com>
Subject: Re: [RFC PATCH v7] sched: Fix performance regression introduced by
 mm_cid
Message-ID: <20230417143532.GA491276@ziqianlu-desk2>
References: <20230416223217.191261-1-mathieu.desnoyers@efficios.com>
 <20230417101855.GA485797@ziqianlu-desk2>
 <5abacbf4-8101-7931-6dee-84f4f004240a@efficios.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <5abacbf4-8101-7931-6dee-84f4f004240a@efficios.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?/KVZPl5eBJRjTi3OcajpbDKV0hQQ5xO5GvHg/X/T6Cta36CE9/X/wTwXzyZr?=
 =?us-ascii?Q?TF//oD10JUlEiCTHRSVU3zp2Z/jgKndlcJcFBl1p/FTevRPtoaa+PfczPG5t?=
 =?us-ascii?Q?DdfF9wIYqfQjx3dgMZy8D45PEBX3bs+63SIMw4R7pAEBt7P9WQLHGtd9KNkH?=
 =?us-ascii?Q?jd2a7IrJ74DsY3mdCDuLD7U1Vy6Zt1qC6kmMRaBUVlgdo1IpCEUh7r7kUMg3?=
 =?us-ascii?Q?j6bPVDooNmlhkcEfIIg+5ZXjaoBMyyPzye0kdm1RVKNJo7+SU+H2pzKZ1Beg?=
 =?us-ascii?Q?43HGuiK/0hVbw8zBKRIppIBaB50LD0iKLvghxgv8ztfC15TEfbbEBgMo79S/?=
 =?us-ascii?Q?I1o+F7NQ2Wp8gvJFBIFFxIHd8BLcl7s+frlGqSommU31wRhYLi0A9+ipUNGY?=
 =?us-ascii?Q?/gVXZdKHDY+66kfZil387sUESGaMsk6hshrNsv/cBDk5f1wHRX7XpYYCihEv?=
 =?us-ascii?Q?FFCSKYSknS3Inay0qhibhY/Eq8T5wKC9XmWeWz9AlKYmDxFiKOVaKUQAg65y?=
 =?us-ascii?Q?QWO5NwYBgsLBNpSSGW1u+sq8OhPPSoR5Jmdh9L+il4dYZS8p9vhNZPmbTvQm?=
 =?us-ascii?Q?lYREacWht2HGQF62FCVSXQ6pcQCKIyyMESspFBppS1aHgG8NdqUPLu6B3Ano?=
 =?us-ascii?Q?zhmZTVUhnoFzOH6AKOXQJKhrMrkdzLJW2dMmTZldMPyU0VqpdC6Cx30aLpwB?=
 =?us-ascii?Q?t63vRbt1rePd44lsofapysFSSCDUXLQPdX7Vm4MIS2diKaDIUh9Xf2/+fT8t?=
 =?us-ascii?Q?H+D2NlDSMxLsPyCxEtURTxSJHtWcflHWjb6ZWueAEW8BOjq56vxJrrMH5KYb?=
 =?us-ascii?Q?AaFpB2imAWnDs7x3eHuHNVNRQDYUu2UkqbVEXTvzkDsIWQqcvrMYHZc82AEY?=
 =?us-ascii?Q?57NKCBQ+BdMVyilBvOFZ0M5nPO0xjrNdx/UvwLOxlKHO1eaX6wmoWruRrisa?=
 =?us-ascii?Q?wIUEEmHf6rKZKTBFYbbkQUAMkPQR6iGfwDv895DrSVzJD7DvQDJLLHgCcnUv?=
 =?us-ascii?Q?qjNxNeHLu2rt4IhuTl9e9v5AprdJzqf227hE0DtMfcLJ1RqhTSRAMTK0yWT9?=
 =?us-ascii?Q?qDpnHG/YKLVDL9dWeNXppK59WNBBp7uiiUpoHmIyhNpMpAWF2vpce7GAMPic?=
 =?us-ascii?Q?VogcYNCtduH6tQrLEpPQmZOR7oJ2S6yTFbPr0Ne3KG1nLFBcMDREEgy/k23E?=
 =?us-ascii?Q?QwJfWs2XDpbozEEiostm607ez+t4y+/wenCPt8If4Ek0C4ERnJBcmFu3Q5LP?=
 =?us-ascii?Q?jfUwFyxZxYHA0iPllK+oF56i2/zdsVSn7mVovk+I62vGi65odjOiXc9cuKwc?=
 =?us-ascii?Q?Qg9guQuDKV3td2oY3p7Q/jIWb4d/pPjAM8jXYLbsn0OY4dQt3a8KdmXecbKM?=
 =?us-ascii?Q?CjZzlboEgN/8djdk9CcZJJYLfi0WxmudL2Gnk2pHV2ZA6Jhv5IX0Dy+8D/BO?=
 =?us-ascii?Q?zYIWDpIY/OB8ygemsKk9cHwosFkBkBC6oYRLNOZOBncxHb4XahRnG0VQjV5P?=
 =?us-ascii?Q?/19ThHBFnbZpTw9F23yIyUGLkqRq5ClrWohfdWJq9/FPvxnxZIuaC4gTMxm9?=
 =?us-ascii?Q?SUJicSrJbify5Ma7aIG0myzjXr7+aH9ldkao3H4H?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f3cb4d4b-966d-4c13-47d7-08db3f510527
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3062.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Apr 2023 14:35:41.6681
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ClDc3kPcrgmYJUhHrCbTlbB9NHbok8VxKR2+P3GEz82VEHW3nDyS30tPOp6DyQF2xU21xwlpVETSYeXNdzSFvA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5103
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Apr 17, 2023 at 09:28:54AM -0400, Mathieu Desnoyers wrote:
> On 2023-04-17 06:18, Aaron Lu wrote:
> > On Sun, Apr 16, 2023 at 06:32:17PM -0400, Mathieu Desnoyers wrote:
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index 0d18c3969f90..9532cf1a2a44 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -2084,8 +2084,10 @@ static inline void dequeue_task(struct rq *rq, struct task_struct *p, int flags)
> > >   void activate_task(struct rq *rq, struct task_struct *p, int flags)
> > >   {
> > > -	if (task_on_rq_migrating(p))
> > > +	if (task_on_rq_migrating(p)) {
> > >   		flags |= ENQUEUE_MIGRATED;
> > > +		sched_mm_cid_migrate_to(rq, p);
> > 
> > 
> > I noticed you did this in previous version too but forgot to ask:
> > is it your intention to only invoke sched_mm_cid_migrate_to() for queued
> > tasks, i.e. tasks that are being migrated due to load/idle balance etc,
> > but not those tasks that migrated on wakeup?
> 
> My intent is to also cover tasks migrated on wakeup.
> 
> > 
> > I saw you did the corresonding sched_mm_cid_migrate_from() in
> > set_task_cpu(), which on the other hand includes tasks that migrated due
> > to wakeup, so it kind of feel weird.
> 
> I'm probably missing something here. AFAIU, when try_to_wake_up() moves the
> target process to a different cpu:
> 
>         cpu = select_task_rq(p, p->wake_cpu, wake_flags | WF_TTWU);
>         if (task_cpu(p) != cpu) {
> 
> it ends up calling ttwu_queue() with wake_flags |= WF_MIGRATED bit set.
> 
> Then ttwu_queue() ends up calling ttwu_queue_wakelist(), which may initiate
> an IPI to the target cpu which executes sched_ttwu_pending().
> This function will take the target cpu's runqueue lock and call
> ttwu_do_activate() with wake_flags=WF_MIGRATED.
> 
> The other path that ttwu_queue() can take is to issue ttwu_do_activate()
> with the target cpu's rq lock held.
> 
> ttwu_do_activate() calls activate_task() with flags having ENQUEUE_MIGRATED
> set.
> 
> OK I think I see what I missed here, I should change this to:
> 
> void activate_task(struct rq *rq, struct task_struct *p, int flags)
> {
>         if (task_on_rq_migrating(p))
>                 flags |= ENQUEUE_MIGRATED;
> 	if (flags & ENQUEUE_MIGRATED)
>                 sched_mm_cid_migrate_to(rq, p);
> [...]
> 
> Because flags is received as input parameter as well.
> 
> Do I get your meaning correctly ?

Yes, that's what I meant.

Tasks that have on_rq equals to TASK_ON_RQ_MIGRATING are only those
migrated by load balance etc., it doesn't apply to tasks that migrated
on wake up.

I think your new change looks correct.

Thanks,
Aaron