[PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Saurabh Sengar]

Currently on a very large system with 1780 CPUs, hv_acpi_init takes
around 3 seconds to complete for all the CPUs. This is because of
sequential synic initialization for each CPU.

Defer these tasks so that each CPU executes hv_acpi_init in parallel
to take full advantage of multiple CPUs.

This solution saves around 2 seconds of boot time on a 1780 CPU system,
that around 66% improvement in the existing logic.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
 drivers/hv/vmbus_drv.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index c857dc3975be..3395526ad0d0 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1306,6 +1306,13 @@ static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void vmbus_percpu_work(struct work_struct *work)
+{
+	unsigned int cpu = smp_processor_id();
+
+	hv_synic_init(cpu);
+}
+
 /*
  * vmbus_bus_init -Main vmbus driver initialization routine.
  *
@@ -1316,7 +1323,8 @@ static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
  */
 static int vmbus_bus_init(void)
 {
-	int ret;
+	int ret, cpu;
+	struct work_struct __percpu *works;
 
 	ret = hv_init();
 	if (ret != 0) {
@@ -1355,12 +1363,31 @@ static int vmbus_bus_init(void)
 	if (ret)
 		goto err_alloc;
 
+	works = alloc_percpu(struct work_struct);
+	if (!works) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
+
 	/*
 	 * Initialize the per-cpu interrupt state and stimer state.
 	 * Then connect to the host.
 	 */
-	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
-				hv_synic_init, hv_synic_cleanup);
+	cpus_read_lock();
+	for_each_online_cpu(cpu) {
+		struct work_struct *work = per_cpu_ptr(works, cpu);
+
+		INIT_WORK(work, vmbus_percpu_work);
+		schedule_work_on(cpu, work);
+	}
+
+	for_each_online_cpu(cpu)
+		flush_work(per_cpu_ptr(works, cpu));
+
+	ret = __cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online", false,
+					     hv_synic_init, hv_synic_cleanup, false);
+	cpus_read_unlock();
+	free_percpu(works);
 	if (ret < 0)
 		goto err_alloc;
 	hyperv_cpuhp_online = ret;
-- 
2.43.0

Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Nuno Das Neves]

On 7/24/2024 10:26 PM, Saurabh Sengar wrote:
> Currently on a very large system with 1780 CPUs, hv_acpi_init takes
> around 3 seconds to complete for all the CPUs. This is because of
> sequential synic initialization for each CPU.
> 
> Defer these tasks so that each CPU executes hv_acpi_init in parallel
> to take full advantage of multiple CPUs.

I think you mean hv_synic_init() here, not hv_acpi_init()?

> 
> This solution saves around 2 seconds of boot time on a 1780 CPU system,
> that around 66% improvement in the existing logic.
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 

LGTM otherwise.

Reviewed-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>

Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Saurabh Singh Sengar]

On Thu, Jul 25, 2024 at 08:23:21AM -0700, Nuno Das Neves wrote:
> On 7/24/2024 10:26 PM, Saurabh Sengar wrote:
> > Currently on a very large system with 1780 CPUs, hv_acpi_init takes
> > around 3 seconds to complete for all the CPUs. This is because of
> > sequential synic initialization for each CPU.
> > 
> > Defer these tasks so that each CPU executes hv_acpi_init in parallel
> > to take full advantage of multiple CPUs.
> 
> I think you mean hv_synic_init() here, not hv_acpi_init()?

Thanks, will send v2 correcting this.

- Saurabh

> 
> > 
> > This solution saves around 2 seconds of boot time on a 1780 CPU system,
> > that around 66% improvement in the existing logic.
> > 
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> >  drivers/hv/vmbus_drv.c | 33 ++++++++++++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> 
> LGTM otherwise.
> 
> Reviewed-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>

RE: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Dexuan Cui]

> From: Saurabh Singh Sengar <ssengar@linux.microsoft.com>
> Sent: Thursday, July 25, 2024 8:35 AM
> Subject: Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

Without the patch, I think the current CPU uses IPIs to let the other
CPUs, one by one,  run the function calls, and synchronously waits
for the function calls to finish.

IMO the patch is not "Deferring per cpu tasks". "Defer" means "let it
happen later". Here it schedules work items to different CPUs, and
the work items immediately start to run on these CPUs.

I would suggest a more accurate subject:
Drivers: hv: vmbus: Run hv_synic_init() concurrently

> -	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> "hyperv/vmbus:online",
> -			hv_synic_init, hv_synic_cleanup);
> +	cpus_read_lock();
> +	for_each_online_cpu(cpu) {
> +		struct work_struct *work = per_cpu_ptr(works, cpu);
> +
> +		INIT_WORK(work, vmbus_percpu_work);
> +		schedule_work_on(cpu, work);
> +	}
> +
> +	for_each_online_cpu(cpu)
> +		flush_work(per_cpu_ptr(works, cpu));
> +

Can you please add a comment to explain we need this for CPU online/offline'ing:
> +	ret = __cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
> "hyperv/vmbus:online", false,
> +					     hv_synic_init, hv_synic_cleanup,
> false);
> +	cpus_read_unlock();

Add an empty line here to make it slightly more readable? :-)
> +	free_percpu(works);
>  	if (ret < 0)
>  		goto err_alloc;

Thanks,
Dexuan

Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Saurabh Singh Sengar]

On Fri, Jul 26, 2024 at 12:01:33AM +0000, Dexuan Cui wrote:
> > From: Saurabh Singh Sengar <ssengar@linux.microsoft.com>
> > Sent: Thursday, July 25, 2024 8:35 AM
> > Subject: Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks
> 
> Without the patch, I think the current CPU uses IPIs to let the other
> CPUs, one by one,  run the function calls, and synchronously waits
> for the function calls to finish.
> 
> IMO the patch is not "Deferring per cpu tasks". "Defer" means "let it
> happen later". Here it schedules work items to different CPUs, and
> the work items immediately start to run on these CPUs.
> 
> I would suggest a more accurate subject:
> Drivers: hv: vmbus: Run hv_synic_init() concurrently

Agree, this explains the change better.

> 
> > -	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > "hyperv/vmbus:online",
> > -			hv_synic_init, hv_synic_cleanup);
> > +	cpus_read_lock();
> > +	for_each_online_cpu(cpu) {
> > +		struct work_struct *work = per_cpu_ptr(works, cpu);
> > +
> > +		INIT_WORK(work, vmbus_percpu_work);
> > +		schedule_work_on(cpu, work);
> > +	}
> > +
> > +	for_each_online_cpu(cpu)
> > +		flush_work(per_cpu_ptr(works, cpu));
> > +
> 
> Can you please add a comment to explain we need this for CPU online/offline'ing:

ok

> > +	ret = __cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
> > "hyperv/vmbus:online", false,
> > +					     hv_synic_init, hv_synic_cleanup,
> > false);
> > +	cpus_read_unlock();
> 
> Add an empty line here to make it slightly more readable? :-)

My personal preference was to have empty line as well here, but then I looked the
other places in this file where we used cpus_read_unlock, hence I maintained that
style consistent.

Please let me know if you have strong opinion about this empty line, I can add in V2.

- Saurabh

> > +	free_percpu(works);
> >  	if (ret < 0)
> >  		goto err_alloc;
> 
> Thanks,
> Dexuan
> 
> 

RE: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Dexuan Cui]

> From: Saurabh Singh Sengar <ssengar@linux.microsoft.com>
> Sent: Thursday, July 25, 2024 10:27 PM
> [...]
> > > +	ret = __cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
> > > "hyperv/vmbus:online", false,
> > > +					     hv_synic_init, hv_synic_cleanup,
> > > false);
> > > +	cpus_read_unlock();
> >
> > Add an empty line here to make it slightly more readable? :-)
> 
> My personal preference was to have empty line as well here, but then I
> looked the
> other places in this file where we used cpus_read_unlock, hence I
> maintained that style consistent.
> 
> Please let me know if you have strong opinion about this empty line, I can
> add in V2.
> 
> - Saurabh

I have no strong opinion here :-)

Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Srivatsa S. Bhat]

On Fri, Jul 26, 2024 at 12:01:33AM +0000, Dexuan Cui wrote:
> > From: Saurabh Singh Sengar <ssengar@linux.microsoft.com>
> > Sent: Thursday, July 25, 2024 8:35 AM
> > Subject: Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks
> 
> Without the patch, I think the current CPU uses IPIs to let the other
> CPUs, one by one,  run the function calls, and synchronously waits
> for the function calls to finish.
> 
> IMO the patch is not "Deferring per cpu tasks". "Defer" means "let it
> happen later". Here it schedules work items to different CPUs, and
> the work items immediately start to run on these CPUs.
> 
> I would suggest a more accurate subject:
> Drivers: hv: vmbus: Run hv_synic_init() concurrently
> 

It would be great to call out the "why" as well in the title,
something like:

Drivers: hv: vmbus: Speed-up booting by running hv_synic_init() concurrently

Regards,
Srivatsa
Microsoft Linux Systems Group

Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Saurabh Singh Sengar]

On Fri, Jul 26, 2024 at 06:34:53AM +0000, Srivatsa S. Bhat wrote:
> On Fri, Jul 26, 2024 at 12:01:33AM +0000, Dexuan Cui wrote:
> > > From: Saurabh Singh Sengar <ssengar@linux.microsoft.com>
> > > Sent: Thursday, July 25, 2024 8:35 AM
> > > Subject: Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks
> > 
> > Without the patch, I think the current CPU uses IPIs to let the other
> > CPUs, one by one,  run the function calls, and synchronously waits
> > for the function calls to finish.
> > 
> > IMO the patch is not "Deferring per cpu tasks". "Defer" means "let it
> > happen later". Here it schedules work items to different CPUs, and
> > the work items immediately start to run on these CPUs.
> > 
> > I would suggest a more accurate subject:
> > Drivers: hv: vmbus: Run hv_synic_init() concurrently
> > 
> 
> It would be great to call out the "why" as well in the title,
> something like:
> 
> Drivers: hv: vmbus: Speed-up booting by running hv_synic_init() concurrently

Thanks, I can also rephrase it like below if sounds fine to you:

Drivers: hv: vmbus: Optimize boot time by concurrent execution of hv_synic_init()

- Saurabh

> 
> Regards,
> Srivatsa
> Microsoft Linux Systems Group

Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Srivatsa S. Bhat]

On Fri, Jul 26, 2024 at 04:26:19AM -0700, Saurabh Singh Sengar wrote:
> On Fri, Jul 26, 2024 at 06:34:53AM +0000, Srivatsa S. Bhat wrote:
> > On Fri, Jul 26, 2024 at 12:01:33AM +0000, Dexuan Cui wrote:
> > > > From: Saurabh Singh Sengar <ssengar@linux.microsoft.com>
> > > > Sent: Thursday, July 25, 2024 8:35 AM
> > > > Subject: Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks
> > > 
> > > Without the patch, I think the current CPU uses IPIs to let the other
> > > CPUs, one by one,  run the function calls, and synchronously waits
> > > for the function calls to finish.
> > > 
> > > IMO the patch is not "Deferring per cpu tasks". "Defer" means "let it
> > > happen later". Here it schedules work items to different CPUs, and
> > > the work items immediately start to run on these CPUs.
> > > 
> > > I would suggest a more accurate subject:
> > > Drivers: hv: vmbus: Run hv_synic_init() concurrently
> > > 
> > 
> > It would be great to call out the "why" as well in the title,
> > something like:
> > 
> > Drivers: hv: vmbus: Speed-up booting by running hv_synic_init() concurrently
> 
> Thanks, I can also rephrase it like below if sounds fine to you:
> 
> Drivers: hv: vmbus: Optimize boot time by concurrent execution of hv_synic_init()
> 

Sure, that sounds good too.

Regards,
Srivatsa
Microsoft Linux Systems Group

RE: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Michael Kelley]

From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Wednesday, July 24, 2024 10:26 PM
> 
> Currently on a very large system with 1780 CPUs, hv_acpi_init takes
> around 3 seconds to complete for all the CPUs. This is because of
> sequential synic initialization for each CPU.
> 
> Defer these tasks so that each CPU executes hv_acpi_init in parallel
> to take full advantage of multiple CPUs.
> 
> This solution saves around 2 seconds of boot time on a 1780 CPU system,
> that around 66% improvement in the existing logic.
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index c857dc3975be..3395526ad0d0 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1306,6 +1306,13 @@ static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
> 
> +static void vmbus_percpu_work(struct work_struct *work)
> +{
> +	unsigned int cpu = smp_processor_id();
> +
> +	hv_synic_init(cpu);
> +}
> +
>  /*
>   * vmbus_bus_init -Main vmbus driver initialization routine.
>   *
> @@ -1316,7 +1323,8 @@ static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
>   */
>  static int vmbus_bus_init(void)
>  {
> -	int ret;
> +	int ret, cpu;
> +	struct work_struct __percpu *works;
> 
>  	ret = hv_init();
>  	if (ret != 0) {
> @@ -1355,12 +1363,31 @@ static int vmbus_bus_init(void)
>  	if (ret)
>  		goto err_alloc;
> 
> +	works = alloc_percpu(struct work_struct);
> +	if (!works) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
>  	/*
>  	 * Initialize the per-cpu interrupt state and stimer state.
>  	 * Then connect to the host.
>  	 */
> -	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
> -				hv_synic_init, hv_synic_cleanup);
> +	cpus_read_lock();
> +	for_each_online_cpu(cpu) {
> +		struct work_struct *work = per_cpu_ptr(works, cpu);
> +
> +		INIT_WORK(work, vmbus_percpu_work);
> +		schedule_work_on(cpu, work);
> +	}
> +
> +	for_each_online_cpu(cpu)
> +		flush_work(per_cpu_ptr(works, cpu));
> +
> +	ret = __cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online", false,
> +					     hv_synic_init, hv_synic_cleanup, false);

I'd suggest using cpuhp_setup_state_nocalls_cpuslocked().  It appears to be
the interface intended for users outside the cpuhotplug code, whereas
__cpuhp_setup_state_cpuslocked() should be private to the cpuhotplug code.

Michael

> +	cpus_read_unlock();
> +	free_percpu(works);
>  	if (ret < 0)
>  		goto err_alloc;
>  	hyperv_cpuhp_online = ret;
> --
> 2.43.0
> 

Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Saurabh Singh Sengar]

On Sun, Jul 28, 2024 at 04:32:23AM +0000, Michael Kelley wrote:
> From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Wednesday, July 24, 2024 10:26 PM
> > 
> > Currently on a very large system with 1780 CPUs, hv_acpi_init takes
> > around 3 seconds to complete for all the CPUs. This is because of
> > sequential synic initialization for each CPU.
> > 
> > Defer these tasks so that each CPU executes hv_acpi_init in parallel
> > to take full advantage of multiple CPUs.
> > 
> > This solution saves around 2 seconds of boot time on a 1780 CPU system,
> > that around 66% improvement in the existing logic.
> > 
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> >  drivers/hv/vmbus_drv.c | 33 ++++++++++++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index c857dc3975be..3395526ad0d0 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1306,6 +1306,13 @@ static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> > 
> > +static void vmbus_percpu_work(struct work_struct *work)
> > +{
> > +	unsigned int cpu = smp_processor_id();
> > +
> > +	hv_synic_init(cpu);
> > +}
> > +
> >  /*
> >   * vmbus_bus_init -Main vmbus driver initialization routine.
> >   *
> > @@ -1316,7 +1323,8 @@ static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
> >   */
> >  static int vmbus_bus_init(void)
> >  {
> > -	int ret;
> > +	int ret, cpu;
> > +	struct work_struct __percpu *works;
> > 
> >  	ret = hv_init();
> >  	if (ret != 0) {
> > @@ -1355,12 +1363,31 @@ static int vmbus_bus_init(void)
> >  	if (ret)
> >  		goto err_alloc;
> > 
> > +	works = alloc_percpu(struct work_struct);
> > +	if (!works) {
> > +		ret = -ENOMEM;
> > +		goto err_alloc;
> > +	}
> > +
> >  	/*
> >  	 * Initialize the per-cpu interrupt state and stimer state.
> >  	 * Then connect to the host.
> >  	 */
> > -	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
> > -				hv_synic_init, hv_synic_cleanup);
> > +	cpus_read_lock();
> > +	for_each_online_cpu(cpu) {
> > +		struct work_struct *work = per_cpu_ptr(works, cpu);
> > +
> > +		INIT_WORK(work, vmbus_percpu_work);
> > +		schedule_work_on(cpu, work);
> > +	}
> > +
> > +	for_each_online_cpu(cpu)
> > +		flush_work(per_cpu_ptr(works, cpu));
> > +
> > +	ret = __cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online", false,
> > +					     hv_synic_init, hv_synic_cleanup, false);
> 
> I'd suggest using cpuhp_setup_state_nocalls_cpuslocked().  It appears to be
> the interface intended for users outside the cpuhotplug code, whereas
> __cpuhp_setup_state_cpuslocked() should be private to the cpuhotplug code.
>

Thanks for your review.
 
The function cpuhp_setup_state_nocalls_cpuslocked() is commonly used across the kernel
drivers hence it was a first choice for me as well. However, it includes a
cpus_read_lock that we already introduced separately in above code. To avoid recursive
locking, I opted for __cpuhp_setup_state_cpuslocked.

One might argue that unlocking and then calling cpuhp_setup_state_nocalls_cpuslocked
could be a solution, but I am concerned about potential race conditions, as CPUs could
come online during this interval and in such case synic initialization for those CPUs
would be missed.

- Saurabh

RE: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Michael Kelley]

From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Sunday, July 28, 2024 2:18 AM
> 
> On Sun, Jul 28, 2024 at 04:32:23AM +0000, Michael Kelley wrote:
> > From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Wednesday, July 24,
> 2024 10:26 PM
> > >
> > > Currently on a very large system with 1780 CPUs, hv_acpi_init takes
> > > around 3 seconds to complete for all the CPUs. This is because of
> > > sequential synic initialization for each CPU.
> > >
> > > Defer these tasks so that each CPU executes hv_acpi_init in parallel
> > > to take full advantage of multiple CPUs.
> > >
> > > This solution saves around 2 seconds of boot time on a 1780 CPU system,
> > > that around 66% improvement in the existing logic.
> > >
> > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > > ---
> > >  drivers/hv/vmbus_drv.c | 33 ++++++++++++++++++++++++++++++---
> > >  1 file changed, 30 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > > index c857dc3975be..3395526ad0d0 100644
> > > --- a/drivers/hv/vmbus_drv.c
> > > +++ b/drivers/hv/vmbus_drv.c
> > > @@ -1306,6 +1306,13 @@ static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
> > >  	return IRQ_HANDLED;
> > >  }
> > >
> > > +static void vmbus_percpu_work(struct work_struct *work)
> > > +{
> > > +	unsigned int cpu = smp_processor_id();
> > > +
> > > +	hv_synic_init(cpu);
> > > +}
> > > +
> > >  /*
> > >   * vmbus_bus_init -Main vmbus driver initialization routine.
> > >   *
> > > @@ -1316,7 +1323,8 @@ static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
> > >   */
> > >  static int vmbus_bus_init(void)
> > >  {
> > > -	int ret;
> > > +	int ret, cpu;
> > > +	struct work_struct __percpu *works;
> > >
> > >  	ret = hv_init();
> > >  	if (ret != 0) {
> > > @@ -1355,12 +1363,31 @@ static int vmbus_bus_init(void)
> > >  	if (ret)
> > >  		goto err_alloc;
> > >
> > > +	works = alloc_percpu(struct work_struct);
> > > +	if (!works) {
> > > +		ret = -ENOMEM;
> > > +		goto err_alloc;
> > > +	}
> > > +
> > >  	/*
> > >  	 * Initialize the per-cpu interrupt state and stimer state.
> > >  	 * Then connect to the host.
> > >  	 */
> > > -	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
> > > -				hv_synic_init, hv_synic_cleanup);
> > > +	cpus_read_lock();
> > > +	for_each_online_cpu(cpu) {
> > > +		struct work_struct *work = per_cpu_ptr(works, cpu);
> > > +
> > > +		INIT_WORK(work, vmbus_percpu_work);
> > > +		schedule_work_on(cpu, work);
> > > +	}
> > > +
> > > +	for_each_online_cpu(cpu)
> > > +		flush_work(per_cpu_ptr(works, cpu));
> > > +
> > > +	ret = __cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online", false,
> > > +					     hv_synic_init, hv_synic_cleanup, false);
> >
> > I'd suggest using cpuhp_setup_state_nocalls_cpuslocked().  It appears to be
> > the interface intended for users outside the cpuhotplug code, whereas
> > __cpuhp_setup_state_cpuslocked() should be private to the cpuhotplug code.
> >
> 
> Thanks for your review.
> 
> The function cpuhp_setup_state_nocalls_cpuslocked() is commonly used across the
> kernel drivers hence it was a first choice for me as well. However, it includes a
> cpus_read_lock that we already introduced separately in above code. To avoid recursive
> locking, I opted for __cpuhp_setup_state_cpuslocked.

cpuhp_setup_state_nocalls() includes the cpus_read_lock() as you describe.
But cpuhp_setup_state_nocalls_cpuslocked() explicitly assumes that the
cpus_read_lock() is already held, so is suitable for use in this case.  There are
several variants with the _cpuslocked suffix, which indicates that the caller
is responsible for the cpus_read_lock().

Michael

> 
> One might argue that unlocking and then calling cpuhp_setup_state_nocalls_cpuslocked
> could be a solution, but I am concerned about potential race conditions, as CPUs could
> come online during this interval and in such case synic initialization for those CPUs
> would be missed.
> 
> - Saurabh

Re: [PATCH] Drivers: hv: vmbus: Deferring per cpu tasks

[Saurabh Singh Sengar]

On Sun, Jul 28, 2024 at 02:06:41PM +0000, Michael Kelley wrote:
> From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Sunday, July 28, 2024 2:18 AM
> > 
> > On Sun, Jul 28, 2024 at 04:32:23AM +0000, Michael Kelley wrote:
> > > From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Wednesday, July 24,
> > 2024 10:26 PM
> > > >
> > > > Currently on a very large system with 1780 CPUs, hv_acpi_init takes
> > > > around 3 seconds to complete for all the CPUs. This is because of
> > > > sequential synic initialization for each CPU.
> > > >
> > > > Defer these tasks so that each CPU executes hv_acpi_init in parallel
> > > > to take full advantage of multiple CPUs.
> > > >
> > > > This solution saves around 2 seconds of boot time on a 1780 CPU system,
> > > > that around 66% improvement in the existing logic.
> > > >
> > > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > > > ---
> > > >  drivers/hv/vmbus_drv.c | 33 ++++++++++++++++++++++++++++++---
> > > >  1 file changed, 30 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > > > index c857dc3975be..3395526ad0d0 100644
> > > > --- a/drivers/hv/vmbus_drv.c
> > > > +++ b/drivers/hv/vmbus_drv.c
> > > > @@ -1306,6 +1306,13 @@ static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
> > > >  	return IRQ_HANDLED;
> > > >  }
> > > >
> > > > +static void vmbus_percpu_work(struct work_struct *work)
> > > > +{
> > > > +	unsigned int cpu = smp_processor_id();
> > > > +
> > > > +	hv_synic_init(cpu);
> > > > +}
> > > > +
> > > >  /*
> > > >   * vmbus_bus_init -Main vmbus driver initialization routine.
> > > >   *
> > > > @@ -1316,7 +1323,8 @@ static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
> > > >   */
> > > >  static int vmbus_bus_init(void)
> > > >  {
> > > > -	int ret;
> > > > +	int ret, cpu;
> > > > +	struct work_struct __percpu *works;
> > > >
> > > >  	ret = hv_init();
> > > >  	if (ret != 0) {
> > > > @@ -1355,12 +1363,31 @@ static int vmbus_bus_init(void)
> > > >  	if (ret)
> > > >  		goto err_alloc;
> > > >
> > > > +	works = alloc_percpu(struct work_struct);
> > > > +	if (!works) {
> > > > +		ret = -ENOMEM;
> > > > +		goto err_alloc;
> > > > +	}
> > > > +
> > > >  	/*
> > > >  	 * Initialize the per-cpu interrupt state and stimer state.
> > > >  	 * Then connect to the host.
> > > >  	 */
> > > > -	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
> > > > -				hv_synic_init, hv_synic_cleanup);
> > > > +	cpus_read_lock();
> > > > +	for_each_online_cpu(cpu) {
> > > > +		struct work_struct *work = per_cpu_ptr(works, cpu);
> > > > +
> > > > +		INIT_WORK(work, vmbus_percpu_work);
> > > > +		schedule_work_on(cpu, work);
> > > > +	}
> > > > +
> > > > +	for_each_online_cpu(cpu)
> > > > +		flush_work(per_cpu_ptr(works, cpu));
> > > > +
> > > > +	ret = __cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online", false,
> > > > +					     hv_synic_init, hv_synic_cleanup, false);
> > >
> > > I'd suggest using cpuhp_setup_state_nocalls_cpuslocked().  It appears to be
> > > the interface intended for users outside the cpuhotplug code, whereas
> > > __cpuhp_setup_state_cpuslocked() should be private to the cpuhotplug code.
> > >
> > 
> > Thanks for your review.
> > 
> > The function cpuhp_setup_state_nocalls_cpuslocked() is commonly used across the
> > kernel drivers hence it was a first choice for me as well. However, it includes a
> > cpus_read_lock that we already introduced separately in above code. To avoid recursive
> > locking, I opted for __cpuhp_setup_state_cpuslocked.
> 
> cpuhp_setup_state_nocalls() includes the cpus_read_lock() as you describe.
> But cpuhp_setup_state_nocalls_cpuslocked() explicitly assumes that the
> cpus_read_lock() is already held, so is suitable for use in this case.  There are
> several variants with the _cpuslocked suffix, which indicates that the caller
> is responsible for the cpus_read_lock().
>

Thank you for the clarification. I will fix this up in v2.

- Saurabh