Re: [patch V4 05/13] cpu/hotplug: Provide knobs to control SMT

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, 21 Jun 2018, speck for Greg KH wrote:
> On Wed, Jun 20, 2018 at 10:19:12PM +0200, speck for Thomas Gleixner wrote:
> > +static int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval)
> > +{
> > +	int cpu, ret = 0;
> > +
> > +	cpu_maps_update_begin();
> > +	for_each_online_cpu(cpu) {
> > +		struct device *dev;
> > +
> > +		if (topology_is_primary_thread(cpu))
> > +			continue;
> > +		ret = cpu_down_maps_locked(cpu, CPUHP_OFFLINE);
> > +		if (ret)
> > +			break;
> > +		/*
> > +		 * As this needs to hold the cpu maps lock it's impossible
> > +		 * to call device_offline(), so nothing would update
> > +		 * device:offline state. That would leave the sysfs entry
> > +		 * stale and prevent onlining after smt control has been
> > +		 * changed to 'off' again. This is called under the sysfs
> > +		 * hotplug lock, so it is properly serialized.
> > +		 *
> > +		 * Temporary workaround until Greg has a smarter idea to do
> > +		 * that.
> > +		 */
> > +		dev = get_cpu_device(cpu);
> > +		dev->offline = true;
> 
> You are right to call me out here.  Ugh.
> 
> I'm missing where device_offline() wants to call any lock other than the
> lock on the device you are offlineing.  Why can't that be called here?

Because it ends up cpu_maps_update_begin() at the end, which is already
held.

> If you do just want to "open code" this, you should also tell userspace
> what just happened, so should you add this line as well:
> 		kobject_uevent(&dev->kobj, KOBJ_OFFLINE);

Ok.

Thanks,

	tglx