The sched-domain of this cpu is only used when SIS_PROP is enabled,
and it should be irrelevant whether the local sd_llc is valid or
not, since all we care about is target sd_llc if !SIS_PROP.
Signed-off-by: Abel Wu <[email protected]>
---
kernel/sched/fair.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 23b020c3d3a0..3561b18bfe9f 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6399,16 +6399,16 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
struct sched_domain *this_sd;
u64 time = 0;
- this_sd = rcu_dereference(*this_cpu_ptr(&sd_llc));
- if (!this_sd)
- return -1;
-
cpumask_and(cpus, sched_domain_span(sd), p->cpus_ptr);
if (sched_feat(SIS_PROP) && !has_idle_core) {
u64 avg_cost, avg_idle, span_avg;
unsigned long now = jiffies;
+ this_sd = rcu_dereference(*this_cpu_ptr(&sd_llc));
+ if (!this_sd)
+ return -1;
+
/*
* If we're busy, the assumption that the last idle period
* predicts the future is flawed; age away the remaining
--
2.31.1
On Thu, Sep 01, 2022 at 09:11:07PM +0800, Abel Wu wrote:
> The sched-domain of this cpu is only used when SIS_PROP is enabled,
> and it should be irrelevant whether the local sd_llc is valid or
> not, since all we care about is target sd_llc if !SIS_PROP.
>
> Signed-off-by: Abel Wu <[email protected]>
This could conceivably result in an uninitialised memory access if
SIS_PROP was enabled while select_idle_cpu is running. I'm not sure if
it can happen when jump labels are in use but I think it could happen
for !CONFIG_JUMP_LABEL updating the sysctl_sched_features bitmap updated
via sysctl.
The patch is still a good idea because it moves an unlikely rcu_deference
out of the default path for sched features but either this_sd needs to
be initialised to NULL and checked or the this_sd lookup needs to happen
twice at a slight additional cost to the default-disabled SIS_PROP path.
--
Mel Gorman
SUSE Labs
On 9/1/22 10:03 PM, Mel Gorman Wrote:
> On Thu, Sep 01, 2022 at 09:11:07PM +0800, Abel Wu wrote:
>> The sched-domain of this cpu is only used when SIS_PROP is enabled,
>> and it should be irrelevant whether the local sd_llc is valid or
>> not, since all we care about is target sd_llc if !SIS_PROP.
>>
>> Signed-off-by: Abel Wu <[email protected]>
>
> This could conceivably result in an uninitialised memory access if
> SIS_PROP was enabled while select_idle_cpu is running. I'm not sure if
> it can happen when jump labels are in use but I think it could happen
> for !CONFIG_JUMP_LABEL updating the sysctl_sched_features bitmap updated
> via sysctl.
Nice catch!
>
> The patch is still a good idea because it moves an unlikely rcu_deference
> out of the default path for sched features but either this_sd needs to
> be initialised to NULL and checked or the this_sd lookup needs to happen
> twice at a slight additional cost to the default-disabled SIS_PROP path.
>
I'd prefer the former.
Thanks & BR,
Abel
On 9/1/22 10:03 PM, Mel Gorman wrote:
> On Thu, Sep 01, 2022 at 09:11:07PM +0800, Abel Wu wrote:
>> The sched-domain of this cpu is only used when SIS_PROP is enabled,
>> and it should be irrelevant whether the local sd_llc is valid or
>> not, since all we care about is target sd_llc if !SIS_PROP.
>>
>> Signed-off-by: Abel Wu <[email protected]>
>
> This could conceivably result in an uninitialised memory access if
> SIS_PROP was enabled while select_idle_cpu is running. I'm not sure if
> it can happen when jump labels are in use but I think it could happen
> for !CONFIG_JUMP_LABEL updating the sysctl_sched_features bitmap updated
> via sysctl.
>
> The patch is still a good idea because it moves an unlikely rcu_deference
> out of the default path for sched features but either this_sd needs to
> be initialised to NULL and checked or the this_sd lookup needs to happen
> twice at a slight additional cost to the default-disabled SIS_PROP path.
>
Hi Mel, please check the following resent patch, Thanks!
https://lore.kernel.org/lkml/[email protected]/
On Wed, Sep 07, 2022 at 03:59:33PM +0800, Abel Wu wrote:
> On 9/1/22 10:03 PM, Mel Gorman wrote:
> > On Thu, Sep 01, 2022 at 09:11:07PM +0800, Abel Wu wrote:
> > > The sched-domain of this cpu is only used when SIS_PROP is enabled,
> > > and it should be irrelevant whether the local sd_llc is valid or
> > > not, since all we care about is target sd_llc if !SIS_PROP.
> > >
> > > Signed-off-by: Abel Wu <[email protected]>
> >
> > This could conceivably result in an uninitialised memory access if
> > SIS_PROP was enabled while select_idle_cpu is running. I'm not sure if
> > it can happen when jump labels are in use but I think it could happen
> > for !CONFIG_JUMP_LABEL updating the sysctl_sched_features bitmap updated
> > via sysctl.
> >
> > The patch is still a good idea because it moves an unlikely rcu_deference
> > out of the default path for sched features but either this_sd needs to
> > be initialised to NULL and checked or the this_sd lookup needs to happen
> > twice at a slight additional cost to the default-disabled SIS_PROP path.
> >
>
> Hi Mel, please check the following resent patch, Thanks!
>
> https://lore.kernel.org/lkml/[email protected]/
Weird, I don't remember seeing this patch even though I'm cc'd on it. It
looks fine so even though it's the wrong thread;
Acked-by: Mel Gorman <[email protected]>
--
Mel Gorman
SUSE Labs