Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Wed, 4 Oct 2023 23:46:21 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Daniel Jordan <daniel.m.jordan@oracle.com>
CC:     <peterz@infradead.org>, <bristot@redhat.com>, <bsegall@google.com>,
        <chris.hyser@oracle.com>, <corbet@lwn.net>,
        <dietmar.eggemann@arm.com>, <efault@gmx.de>,
        <joel@joelfernandes.org>, <joshdon@google.com>,
        <juri.lelli@redhat.com>, <kprateek.nayak@amd.com>,
        <linux-kernel@vger.kernel.org>, <mgorman@suse.de>,
        <mingo@kernel.org>, <patrick.bellasi@matbug.net>, <pavel@ucw.cz>,
        <pjt@google.com>, <qperret@google.com>, <qyousef@layalina.io>,
        <rostedt@goodmis.org>, <tglx@linutronix.de>,
        <tim.c.chen@linux.intel.com>, <timj@gnu.org>,
        <vincent.guittot@linaro.org>, <youssefesmat@chromium.org>,
        <yu.chen.surf@gmail.com>
Subject: Re: [PATCH v2] sched/fair: Preserve PLACE_DEADLINE_INITIAL deadline
Message-ID: <ZR2IzepAXhxnsqLp@chenyu5-mobl2.ccr.corp.intel.com>
References: <20231004011737.188682-1-daniel.m.jordan@oracle.com>
 <20231004130908.238992-1-daniel.m.jordan@oracle.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20231004130908.238992-1-daniel.m.jordan@oracle.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Ew0+BmjGpQq0q5rdxwRuxg2j8Y3UseD/cbr2FllXBXQ+bAvtC3GvexfND745?=
 =?us-ascii?Q?v5bVpuF5naUk8GDecvoNjfa9cK9tFlSOX5j7zrG8zLXsVexSDMl7Jd2TYnko?=
 =?us-ascii?Q?A0ULDMR+3/OdekUa35IBUfECAgNkITh2XkmicFhVExPVTjlzOtB/yBwF9YwK?=
 =?us-ascii?Q?VT5D2sbbufjln0b80TRgHqfj5GkRHnAGNwndohaYfnqSV/J0ciiFrADjW9a2?=
 =?us-ascii?Q?W1p+YsKaG6OXiZ3c/3zTHjtv2wxv2bAqFVh4pVfebk8aC6ewfYpI4/VnXAp3?=
 =?us-ascii?Q?fiq0V8K7uK7qbQAVYwzTVe/7PBhL6JtEnzl1TDQ/yj+PfS5mZUi+R+Cd6s+w?=
 =?us-ascii?Q?Mr6kASguLhZ9SNDKpKt+KXNV41+0UxK9umJMARV73Qrmp1AHp3dFURbfI9kR?=
 =?us-ascii?Q?k6N10bIhb2qxc6H0BLiOT8a7rNkl/DBoJ1br6umXMhlwHd4vkEmn6APC3Y5T?=
 =?us-ascii?Q?fkn/ZQpFYpgLKVJh0FYvvCqKtQsnDekYL4X18D0ZM7fnq8aeIRGmwIoTMO8g?=
 =?us-ascii?Q?a6/JR1oYuJ5UDvof95Qol6QnDj8fLBJJdahbSi/UYBJym55sJwxNZUk/XjH3?=
 =?us-ascii?Q?n1yzthmKbOvCXiAdcKB+9yQDPXYOgPfZS5Hm/cpj2NWbuISjaL6ai7ejjyzy?=
 =?us-ascii?Q?B5vAoWkJ+WXfsM1a+C9NTpppvyrthjxcpTvabmbW7jK0kzc43wB/r9AWfHA5?=
 =?us-ascii?Q?y6A9Hz7YdTIjQK9zgQeQ4cltfapa+xWfG/RzskoIuZtd37c5Z0gdJLszjey1?=
 =?us-ascii?Q?cnZkRVXLIGPU9oP9X4FSHNNW7pDE0+h4qiZTvmn1CinLPQHTYXU9zb/YGKan?=
 =?us-ascii?Q?AuRy2XV+yVuapCXHsFRISizhSWk+KYe8t/Dv0xrqPxcHF3gweyfMQzue6GrP?=
 =?us-ascii?Q?F5FfIn9IZ1CpmLfRMNi9Rli7SxJvQ1DtY95MG9wGutNK9nshi8q1pV2QniHk?=
 =?us-ascii?Q?tDkmKgBT0JkXMBSnt0X4elNwVJl5jlqn01VuZQNGlNJHf/UdfsoNEl1LLV5w?=
 =?us-ascii?Q?SByIYnOYIWCWHudStvnwManK5IRTRvc5NuDuBi7Tcv/FVFCikRI15t2oaP4w?=
 =?us-ascii?Q?dw9IPfy2NbAVGzpl1u72iYKyIms8cm+5hzfmt/NNBfuraUeV0FW3ldWe7fk7?=
 =?us-ascii?Q?I1MqFi6i5EgW7pmndVv/2OH7tN3fpa6SIxZRwuM8oKyYIXna5wSuJR53FYoy?=
 =?us-ascii?Q?Htx3D6xkUcBoGc5ZDL3Zx0RKizFLkUs7NyMT9O76ng76H6hRBXksDmpgbnuz?=
 =?us-ascii?Q?qh3D67W4NHLumIEzAb+LA9IgFebTaUXgH6iKS8Ld5c0EAO3WneyH0YytxxrH?=
 =?us-ascii?Q?+c5wxfoaf1hP1ZWlpp8hkkjIevfsKoEBKN40MAC9JMaYg60YbxUjtg7yceZS?=
 =?us-ascii?Q?pbQDDqJfqnyWH8UW/CH/Y/yqAV0Wd2tE1ahcpuEHfB5OEmabofWUuvtv03pG?=
 =?us-ascii?Q?kNoJZENJVrzA0Xr1VfNdqML9/Pz+6tRIQwYBmbcNMq2F6LIF2BaEq89JOQ1N?=
 =?us-ascii?Q?xfi93mlbW9UD568VwIl8RW+/EKkv+P/kPTeYVzbXemLq+iJJaHRfo/V/of+t?=
 =?us-ascii?Q?TATDYnFCNcmOOSN9kgFF72mwACsYUcYjsP7BqBzP?=
X-MS-Exchange-CrossTenant-Network-Message-Id: efbc482a-f8b2-44a6-7e0c-08dbc4f11d5f
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Oct 2023 15:46:46.4156
 (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: UHj5yMklLxw6ixXZD9Puv03UuzMJCfpVrOEIjinSfoGf/BFOprwYBHuQHFDZVjH4yODbFYFXs7DgvxwS+Wy6oA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7808
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Wed, 04 Oct 2023 08:47:44 -0700 (PDT)

Hi Daniel,

On 2023-10-04 at 09:09:08 -0400, Daniel Jordan wrote:
> An entity is supposed to get an earlier deadline with
> PLACE_DEADLINE_INITIAL when it's forked, but the deadline gets
> overwritten soon after in enqueue_entity() the first time a forked
> entity is woken so that PLACE_DEADLINE_INITIAL is effectively a no-op.
> 
> Placing in task_fork_fair() seems unnecessary since none of the values
> that get set (slice, vruntime, deadline) are used before they're set
> again at enqueue time, so get rid of that (and with it all of
> task_fork_fair()) and just pass ENQUEUE_INITIAL to enqueue_entity() via
> wake_up_new_task().
> 
> Fixes: e8f331bcc270 ("sched/smp: Use lag to simplify cross-runqueue placement")
> Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> ---
> 
> v2
>  - place_entity() seems like the only reason for task_fork_fair() to exist
>    after the recent removal of sysctl_sched_child_runs_first, so take out
>    the whole function.

At first glance I thought if we remove task_fork_fair(), do we lose one chance to
update the parent task's statistic in update_curr()?  We might get out-of-date
parent task's deadline and make preemption decision based on the stale data in
wake_up_new_task() -> wakeup_preempt() -> pick_eevdf(). But after a second thought,
I found that wake_up_new_task() -> enqueue_entity() itself would invoke update_curr(),
so this should not be a problem.

Then I was wondering why can't we just skip place_entity() in enqueue_entity()
if ENQUEUE_WAKEUP is not set, just like the code before e8f331bcc270? In this
way the new fork task's deadline will not be overwritten by wake_up_new_task()->
enqueue_entity(). Then I realized that, after e8f331bcc270, the task's vruntime
and deadline are all calculated by place_entity() rather than being renormalised
to cfs_rq->min_vruntime in enqueue_entity(), so we can not simply skip place_entity()
in enqueue_entity().

Per my understanding, this patch looks good,

Reviewed-by: Chen Yu <yu.c.chen@intel.com>

thanks,
Chenyu