Date:   Sun, 29 Jan 2023 01:27:00 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Roman Kagan <rkagan@amazon.de>
CC:     <linux-kernel@vger.kernel.org>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Ben Segall <bsegall@google.com>,
        Zhang Qiao <zhangqiao22@huawei.com>,
        Ingo Molnar <mingo@redhat.com>,
        Waiman Long <longman@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        "Peter Zijlstra" <peterz@infradead.org>,
        Valentin Schneider <vschneid@redhat.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Mel Gorman <mgorman@suse.de>,
        Juri Lelli <juri.lelli@redhat.com>
Subject: Re: [PATCH] sched/fair: sanitize vruntime of entity being placed
Message-ID: <Y9Va5HJgEDteceg3@chenyu5-mobl1>
References: <20230127163230.3339408-1-rkagan@amazon.de>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230127163230.3339408-1-rkagan@amazon.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?jXp6AFa6FrTSpwAg/Fknipw9x6S6pxClGBL/xTxMPsN9iH4uJ3x+vFhBgBdj?=
 =?us-ascii?Q?w3/3IJyZJwGJUV+brXbGH2e0Dwh/s6d+z2iRQbvRP1k6ZE58pHjYtC80QPzW?=
 =?us-ascii?Q?buXbaT6Yq7g/7f5wVPUWsVlHMphRB70e6RRU0i0OLEhTQU8xXunDIy73El2O?=
 =?us-ascii?Q?kbGB8HnNGlR+f0DISD0B+9qMkwTO8wV3a6k7z17vw4fm7zPZFNuHBiotC6HQ?=
 =?us-ascii?Q?hncHBLGWUi17lLDCEwDvLdsHEkhiqPxmaNSP0KW6Z2zI5NUgMyOQcn4l6wjW?=
 =?us-ascii?Q?tT1h5ts7/aT0vTzZoQrlYsv0SmDxCCHJ4HwsfcV7dS1weFA2LrLX7rWbJs0B?=
 =?us-ascii?Q?luXL0C9vD7Pwh9WdJPbxH/aLEiD3pYPugVCsxr0zmRnkEYogluvlM525M80f?=
 =?us-ascii?Q?vvLZYPpYgFEgUo4TVAFlSHUQr+YQreRKDCNDy7QMNd9JFz9/CuRr90JJ16Bu?=
 =?us-ascii?Q?SHWMytN1wegI2zpHcyuU6qVFpFAyYEUYCho0/3gdfYGxC1Wyw/hWh9VgFMmJ?=
 =?us-ascii?Q?EUK+aQXrjFp/twcsDzmhOCeQkl/PHrOYC0ITojwSEbcBPF7YOPP5pXQCe5oE?=
 =?us-ascii?Q?G5DjUUTH+U0OoDJy3k8bEybxPhLs/i539cV2/m1x73QtTCDoTDI5PLGQUeHp?=
 =?us-ascii?Q?NTBgiu3qr3rlh6msFmrA1yPMSLg4YAhtwIsSK4/F7V9cXVfhzV4VeBCJDu9m?=
 =?us-ascii?Q?p7uJnK3YeMWqeIUpPkQBIVYe3ZDQF0RybjGu8emjF5E9Hu7uwXbzFAYihl8Q?=
 =?us-ascii?Q?MyCFqi22nh4c6zYeuk0q3Ou3hrG961wyEvHkxT1cUtf19Jd9c8mMEpu8t5bi?=
 =?us-ascii?Q?INRyO7hTyyxnvaYKgBWUMMypMzdepUXCYSaX8Vt4Ylh2drDv0h4pYfikCkly?=
 =?us-ascii?Q?CaSBlgl/E25eGfJXOe25k9ZnueqiFu+2A6VOFT6cp/obURWxlcjSCj907AAD?=
 =?us-ascii?Q?9sgEbl9LNKkybUwRrIcGzpX00u7r3330Mr9/JfudfPEg1JLHf4pARFF10yv9?=
 =?us-ascii?Q?/o0MpLWy1FEXzQkB5RQTXN7fKZ+mZcvcB3JNLFtDI+NX6fcbrlweqqXkr3Za?=
 =?us-ascii?Q?QPKeKjxP4KNOy7hGqOAm///MdFgheqY6+nXheFkvJ05WNTNiKfnTCixIyPLe?=
 =?us-ascii?Q?ihhWkUiRQOo6Xk39YjifaPigF2HGBwtm+w5U3w44nMk/iwf7gR03qdH12B8V?=
 =?us-ascii?Q?mu8qP2NQogaIxNRAslQCkIGQ4Rs4P1RWohi5XeKCY5IqnRg/WWJ8ZlsOR1PQ?=
 =?us-ascii?Q?Mr4Q0GqK02dI2azOcbs89Y99h8Zmv4KUnIa4nZ7+lwdpzE0hGg6yW8reEj6Q?=
 =?us-ascii?Q?wI0KMFD70DK4FfkfnO5zLLQ/xgXCnOB9Tv+Inw9rAIGUPRnonUXI/2e9DK9t?=
 =?us-ascii?Q?qVMIRKj4s39kzZhfEJb01ESW2jaxgQP2GcRQK8ZK0tWVk/BifOkOncz+sm5Y?=
 =?us-ascii?Q?R5xu6LJXnbID/37hTiCMQ16LaNXCZQHviIgC025p2+JEo+vUxHo7Wnuo6jYc?=
 =?us-ascii?Q?DuPnoqsIUWmDqJ5EZef1U5+6gncRMMY16QZulnXxW9JE+LFVqZc5/IqQHNTq?=
 =?us-ascii?Q?3WjgrUbAmG/Ir71pmRC1iIQlXGYbOACz0JwdvhcO?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a0bdc846-0ecf-4ed4-759f-08db0154ee9b
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jan 2023 17:27:29.7915
 (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: K7wWAd7HzpAECFWNdleamYvDRyBQmiaCtzHns2CzeglJ1OdJhNPKfgYRLum4+JpLGfYBt6P3BkPevKpiMclRmg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7145
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Roman, Qiao,
On 2023-01-27 at 17:32:30 +0100, Roman Kagan wrote:
> From: Zhang Qiao <zhangqiao22@huawei.com>
> 
> When a scheduling entity is placed onto cfs_rq, its vruntime is pulled
> to the base level (around cfs_rq->min_vruntime), so that the entity
> doesn't gain extra boost when placed backwards.
> 
> However, if the entity being placed wasn't executed for a long time, its
> vruntime may get too far behind (e.g. while cfs_rq was executing a
> low-weight hog), which can inverse the vruntime comparison due to s64
> overflow.  This results in the entity being placed with its original
> vruntime way forwards, so that it will effectively never get to the cpu.
>
Looks interesting,
case 1:
  se->vruntime = 1, cfs_rq->min_vruntime = ULONG_MAX
  ==> max = 1
case 2:
  se->vruntime = 1, cfs_rq->min_vruntime = LONG_MAX
  ==> max = LONG_MAX

May I know if the issue you described above is in case 1? We want
the max to be ULONG_MAX but it returns 1 because of s64
comparison? Then max = 1 is incorrectly used as se's vruntime?
Could you please elaborate a little more about this issue?
> To prevent that, ignore the vruntime of the entity being placed if it
> didn't execute for much longer than the characteristic sheduler time
> scale.
> 
> [rkagan: formatted, adjusted commit log, comments, cutoff value]
> Co-developed-by: Roman Kagan <rkagan@amazon.de>
> Signed-off-by: Roman Kagan <rkagan@amazon.de>
> ---
> @zhangqiao22, I took the liberty to put you as the author of the patch,
> as this is essentially what you posted for discussion, with minor
> tweaks.  Please stamp with your s-o-b if you're ok with it.
> 
>  kernel/sched/fair.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 0f8736991427..d6cf131ebb0b 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4656,6 +4656,7 @@ static void
>  place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int initial)
>  {
>  	u64 vruntime = cfs_rq->min_vruntime;
> +	u64 sleep_time;
>  
>  	/*
>  	 * The 'current' period is already promised to the current tasks,
> @@ -4685,8 +4686,18 @@ place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int initial)
>  		vruntime -= thresh;
>  	}
>  
> -	/* ensure we never gain time by being placed backwards. */
> -	se->vruntime = max_vruntime(se->vruntime, vruntime);
> +	/*
> +	 * Pull vruntime of the entity being placed to the base level of
> +	 * cfs_rq, to prevent boosting it if placed backwards.  If the entity
> +	 * slept for a long time, don't even try to compare its vruntime with
> +	 * the base as it may be too far off and the comparison may get
> +	 * inversed due to s64 overflow.
> +	 */
> +	sleep_time = rq_clock_task(rq_of(cfs_rq)) - se->exec_start;
If I understand correctly, se->exec_start is just updated by enqueue_entity()->update_curr(cfs_rq),
then place_entity() in invoked here, I'm not sure if sleep_time above
could reflect the real sleep time. Maybe something like:
rq_clock_task(rq_of(cfs_rq)) - se->time_stamp_dequeued ?

thanks,
Chenyu