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:   Tue, 24 May 2022 15:37:11 -0400
From:   Phil Auld <pauld@redhat.com>
To:     Valentin Schneider <vschneid@redhat.com>
Cc:     linux-kernel@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] cpuhp: make target_store() a nop when target == state
Message-ID: <Yo0z56Fqgj3gqYlG@lorien.usersys.redhat.com>
References: <20220523144728.32414-1-pauld@redhat.com>
 <xhsmh35gzj77s.mognet@vschneid.remote.csb>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <xhsmh35gzj77s.mognet@vschneid.remote.csb>
Precedence: bulk

Hi Valentin,

On Tue, May 24, 2022 at 04:11:51PM +0100 Valentin Schneider wrote:
> On 23/05/22 10:47, Phil Auld wrote:
> > writing the current state back into hotplug/target calls cpu_down()
> > which will set cpu dying even when it isn't and then nothing will
> > ever clear it. A stress test that reads values and writes them back
> > for all cpu device files in sysfs will trigger the BUG() in
> > select_fallback_rq once all cpus are marked as dying.
> >
> > kernel/cpu.c::target_store()
> > 	...
> >         if (st->state < target)
> >                 ret = cpu_up(dev->id, target);
> >         else
> >                 ret = cpu_down(dev->id, target);
> >
> > cpu_down() -> cpu_set_state()
> > 	 bool bringup = st->state < target;
> > 	 ...
> > 	 if (cpu_dying(cpu) != !bringup)
> > 		set_cpu_dying(cpu, !bringup);
> >
> > Make this safe by catching the case where target == state
> > and bailing early.
> >
> > Signed-off-by: Phil Auld <pauld@redhat.com>
> > ---
> >
> > Yeah, I know... don't do that. But it's still messy.
> >
> > !< != > 
> >
> >  kernel/cpu.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index d0a9aa0b42e8..8a71b1149c60 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -2302,6 +2302,9 @@ static ssize_t target_store(struct device *dev, struct device_attribute *attr,
> >  		return -EINVAL;
> >  #endif
> >  
> > +	if (target == st->state)
> > +		return count;
> > +
> 
> The current checks are against static boundaries, this has to compare
> against st->state - AFAICT this could race with another hotplug operation
> to the same CPU, e.g.
> 
>   CPU42.cpuhp_state
>     ->state  == CPUHP_AP_SCHED_STARTING
>     ->target == CPUHP_ONLINE
> 
>   <write CPUHP_ONLINE via sysfs, OK because current state != CPUHP_ONLINE>
> 
>   CPU42.cpuhp_state == CPUHP_ONLINE
>   
>   <issues ensue>
> 
> 
> _cpu_up() has:
> 
> 	/*
> 	 * The caller of cpu_up() might have raced with another
> 	 * caller. Nothing to do.
> 	 */
> 	if (st->state >= target)
> 		goto out;
> 
> Looks like we want an equivalent in _cpu_down(), what do you think?
>

I did it like this (shown below) and from my test it also works for
this case.

I could move it below the lock and goto out;  instead if you think
that is better. It still seems better to me to stop this higher up
because there's work being done in the out path too.  We're not
actually doing any hot(un)plug so doing post unplug cleanup seems
iffy.

_cpu_down()
...
out:
        cpus_write_unlock();
        /*
         * Do post unplug cleanup. This is still protected against
         * concurrent CPU hotplug via cpu_add_remove_lock.
         */
        lockup_detector_cleanup();
        arch_smt_update();
        cpu_up_down_serialize_trainwrecks(tasks_frozen);
	return ret;
}

----------

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 8a71b1149c60..e36788742d18 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1130,6 +1130,13 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen,
 	if (!cpu_present(cpu))
 		return -EINVAL;
 
+	/*
+	 * The caller of cpu_down() might have raced with another
+	 * caller. Nothing to do.
+	 */
+	if (st->state <= target)
+		return 0;
+
 	cpus_write_lock();
 
 	cpuhp_tasks_frozen = tasks_frozen;


Cheers,
Phil

--