Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] cpu/hotplug: Wait for cpu_hotplug to be enabled in
 cpu_up/down
To:     Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>,
        tglx@linutronix.de, linux-kernel@vger.kernel.org
References: <9bf397db-0eb8-ac30-b0ff-f8970d8b21be@nokia.com>
From:   Alexander Sverdlin <alexander.sverdlin@nokia.com>
Message-ID: <18207c49-fa25-81e2-4ef3-df1b232809b8@nokia.com>
Date:   Fri, 24 Jan 2020 12:51:09 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.1
In-Reply-To: <9bf397db-0eb8-ac30-b0ff-f8970d8b21be@nokia.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Received-SPF: None (protection.outlook.com: nokia.com does not designate
 permitted sender hosts)
X-MS-Exchange-CrossTenant-Network-Message-Id: 56ee1ea0-aa0d-48bc-8a6e-08d7a0c3bcb6
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jan 2020 11:51:24.3729
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5d471751-9675-428d-917b-70f44f9630b0
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: VtNaRixlhDbLJ1Jsa6h92zNMWtcj/zlpUjcJvioMfaj8M7IVr2PVkXZTc4Xcy20f2789QEhG2XEO4RpOpYJiSmDiww1O48AobUHw7pxYtVw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR07MB4237
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi!

On 24/01/2020 09:18, Matija Glavinic Pecotic wrote:
> cpu hotplug may be disabled via cpu_hotplug_enable/cpu_hotplug_disable.
> When disabled, cpu_down and cpu_up will fail with -EBUSY. Users of the
> cpu_up/cpu_down should handle this situation as this is mostly temporal
> disablement and exception should be made for EBUSY, assuming that EBUSY
> always stands for this situation and is worth repeating execution. One
> of the users of cpu_hotplug_enable/disable is pci_device_probe yielding
> errors on bringing cpu cores up/down if pci devices are getting probed.
> 
> Problem was observed on x86 board by having partitioning of the system
> to RT/NRT cpu sets failing (of which part is to bring cpus down/up via
> sysfs) if pci devices would be getting probed at the same time. This is
> confusing for userspace as dependency to pci devices is not clear.
> 
> Fix this behavior by waiting for cpu hotplug to be ready. Return -EBUSY
> only after hotplugging was not enabled for about 10 seconds.
> 
> Fixes: 1ddd45f8d76f ("PCI: Use cpu_hotplug_disable() instead of get_online_cpus()")
> Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> ---
>   kernel/cpu.c | 50 ++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 40 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 4dc279e..2e06ca9 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -31,6 +31,7 @@
>   #include <linux/relay.h>
>   #include <linux/slab.h>
>   #include <linux/percpu-rwsem.h>
> +#include <linux/wait.h>
>   
>   #include <trace/events/power.h>
>   #define CREATE_TRACE_POINTS
> @@ -278,11 +279,22 @@ void cpu_maps_update_done(void)
>   }
>   
>   /*
> - * If set, cpu_up and cpu_down will return -EBUSY and do nothing.
> + * If set, cpu_up and cpu_down will retry for cpu_hotplug_retries and
> + * eventually return -EBUSY if unsuccessful.
>    * Should always be manipulated under cpu_add_remove_lock
>    */
>   static int cpu_hotplug_disabled;
>   
> +/*
> + * waitqueue for waiting on cpu_hotplug_disabled
> + */
> +static DECLARE_WAIT_QUEUE_HEAD(wait_cpu_hp_enabled);
> +
> +/*
> + * Retries for cpu_hotplug to be enabled by cpu_up/cpu_down.
> + */
> +static int cpu_hotplug_retries = 10;
> +
>   #ifdef CONFIG_HOTPLUG_CPU
>   
>   DEFINE_STATIC_PERCPU_RWSEM(cpu_hotplug_lock);
> @@ -341,7 +353,7 @@ static void lockdep_release_cpus_lock(void)
>   
>   /*
>    * Wait for currently running CPU hotplug operations to complete (if any) and
> - * disable future CPU hotplug (from sysfs). The 'cpu_add_remove_lock' protects
> + * briefly disable CPU hotplug (from sysfs). The 'cpu_add_remove_lock' protects
>    * the 'cpu_hotplug_disabled' flag. The same lock is also acquired by the
>    * hotplug path before performing hotplug operations. So acquiring that lock
>    * guarantees mutual exclusion from any currently running hotplug operations.
> @@ -366,6 +378,7 @@ void cpu_hotplug_enable(void)
>   	cpu_maps_update_begin();
>   	__cpu_hotplug_enable();
>   	cpu_maps_update_done();
> +	wake_up(&wait_cpu_hp_enabled);
>   }
>   EXPORT_SYMBOL_GPL(cpu_hotplug_enable);
>   
> @@ -1044,11 +1057,21 @@ static int cpu_down_maps_locked(unsigned int cpu, enum cpuhp_state target)
>   
>   static int do_cpu_down(unsigned int cpu, enum cpuhp_state target)
>   {
> -	int err;
> +	int err = -EBUSY, retries = cpu_hotplug_retries;
>   
> -	cpu_maps_update_begin();
> -	err = cpu_down_maps_locked(cpu, target);
> -	cpu_maps_update_done();
> +	while (retries--) {
> +		wait_event_timeout(wait_cpu_hp_enabled,
> +				!cpu_hotplug_disabled,
> +				HZ);
> +		cpu_maps_update_begin();
> +		if (cpu_hotplug_disabled) {
> +			cpu_maps_update_done();
> +			continue;
> +		}
> +		err = _cpu_down(cpu, 0, target);
> +		cpu_maps_update_done();
> +		break;
> +	}
>   	return err;
>   }
>   
> @@ -1166,7 +1189,7 @@ static int _cpu_up(unsigned int cpu, int tasks_frozen, enum cpuhp_state target)
>   
>   static int do_cpu_up(unsigned int cpu, enum cpuhp_state target)
>   {
> -	int err = 0;
> +	int err = 0, retries = cpu_hotplug_retries;
>   
>   	if (!cpu_possible(cpu)) {
>   		pr_err("can't online cpu %d because it is not configured as may-hotadd at boot time\n",
> @@ -1181,9 +1204,16 @@ static int do_cpu_up(unsigned int cpu, enum cpuhp_state target)
>   	if (err)
>   		return err;
>   
> -	cpu_maps_update_begin();
> -
> -	if (cpu_hotplug_disabled) {
> +	while (--retries) {
> +		wait_event_timeout(wait_cpu_hp_enabled,
> +				   !cpu_hotplug_disabled,
> +				   HZ);
> +		cpu_maps_update_begin();
> +		if (!cpu_hotplug_disabled)
> +			break;
> +		cpu_maps_update_done();
> +	}
> +	if (!retries) {
>   		err = -EBUSY;
>   		goto out;
>   	}
> 

-- 
Best regards,
Alexander Sverdlin.