Remove on-stack cpumask in hv_apic.c

This is a new variant which removes `self' cpu from the vpset. It will be used in Hyper-V enlightened IPI code.

Signed-off-by Wei Liu <[email protected]>

---
Provide a new variant instead of adding a new parameter because it makes it easier to backport -- we don't need to fix the users of cpumask_to_vpset.

v2:
1. Rename function
2. Add preemptible check

---
 include/asm-generic/mshyperv.h | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

include/asm-generic/mshyperv.h +19 -3

index 9a000ba2bb75..9a134806f1d5 100644
@@ -184,10 +184,12 @@ static inline int hv_cpu_number_to_vp_number(int cpu_number)
 	return hv_vp_index[cpu_number];
 }
 
-static inline int cpumask_to_vpset(struct hv_vpset *vpset,
-				    const struct cpumask *cpus)
+static inline int __cpumask_to_vpset(struct hv_vpset *vpset,
+				    const struct cpumask *cpus,
+				    bool exclude_self)
 {
 	int cpu, vcpu, vcpu_bank, vcpu_offset, nr_bank = 1;
+	int this_cpu = smp_processor_id();
 
 	/* valid_bank_mask can represent up to 64 banks */
 	if (hv_max_vp_index / 64 >= 64)

@@ -205,6 +207,8 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset,
 	 * Some banks may end up being empty but this is acceptable.
 	 */
 	for_each_cpu(cpu, cpus) {
+		if (exclude_self && cpu == this_cpu)
+			continue;
 		vcpu = hv_cpu_number_to_vp_number(cpu);
 		if (vcpu == VP_INVAL)
 			return -1;

@@ -219,6 +223,19 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset,
 	return nr_bank;
 }
 
+static inline int cpumask_to_vpset(struct hv_vpset *vpset,
+				    const struct cpumask *cpus)
+{
+	return __cpumask_to_vpset(vpset, cpus, false);
+}
+
+static inline int cpumask_to_vpset_noself(struct hv_vpset *vpset,
+				    const struct cpumask *cpus)
+{
+	WARN_ON_ONCE(preemptible());
+	return __cpumask_to_vpset(vpset, cpus, true);
+}
+
 void hyperv_report_panic(struct pt_regs *regs, long err, bool in_die);
 bool hv_is_hyperv_initialized(void);
 bool hv_is_hibernation_supported(void);
--

2.30.2

It is not a good practice to allocate a cpumask on stack, given it may consume up to 1 kilobytes of stack space if the kernel is configured to have 8192 cpus.

The internal helper functions __send_ipi_mask{,_ex} need to loop over the provided mask anyway, so it is not too difficult to skip `self' there. We can thus do away with the on-stack cpumask in hv_send_ipi_mask_allbutself.

Adjust call sites of __send_ipi_mask as needed.

Reported-by Linus Torvalds <[email protected]>

Suggested-by Michael Kelley <[email protected]>

Suggested-by Linus Torvalds <[email protected]>

Fixes 68bb7bfb7985d ("X86/Hyper-V: Enable IPI enlightenments")

Signed-off-by Wei Liu <[email protected]>

---

v2: more robust check in __send_ipi_mask

---
 arch/x86/hyperv/hv_apic.c | 43 +++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

arch/x86/hyperv/hv_apic.c +10 -4

index 90e682a92820..48aefcea724b 100644
@@ -99,7 +99,8 @@ static void hv_apic_eoi_write(u32 reg, u32 val)
 /*
  * IPI implementation on Hyper-V.
  */
-static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector)
+static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector,
+		bool exclude_self)
 {
 	struct hv_send_ipi_ex **arg;
 	struct hv_send_ipi_ex *ipi_arg;

@@ -123,7 +124,10 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector)
 
 	if (!cpumask_equal(mask, cpu_present_mask)) {
 		ipi_arg->vp_set.format = HV_GENERIC_SET_SPARSE_4K;
-		nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask);
+		if (exclude_self)
+			nr_bank = cpumask_to_vpset_noself(&(ipi_arg->vp_set), mask);
+		else
+			nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask);
 	}
 	if (nr_bank < 0)
 		goto ipi_mask_ex_done;

@@ -138,15 +142,25 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector)
 	return hv_result_success(status);
 }
 
-static bool __send_ipi_mask(const struct cpumask *mask, int vector)
+static bool __send_ipi_mask(const struct cpumask *mask, int vector,
+		bool exclude_self)
 {
-	int cur_cpu, vcpu;
+	int cur_cpu, vcpu, this_cpu = smp_processor_id();
 	struct hv_send_ipi ipi_arg;
 	u64 status;
+	unsigned int weight;
 
 	trace_hyperv_send_ipi_mask(mask, vector);
 

arch/x86/hyperv/hv_apic.c +7 -13

 		return true;
 
 	if (!hv_hypercall_pg)

@@ -172,6 +186,8 @@ static bool __send_ipi_mask(const struct cpumask *mask, int vector)
 	ipi_arg.cpu_mask = 0;
 
 	for_each_cpu(cur_cpu, mask) {
+		if (exclude_self && cur_cpu == this_cpu)
+			continue;
 		vcpu = hv_cpu_number_to_vp_number(cur_cpu);
 		if (vcpu == VP_INVAL)
 			return false;

@@ -191,7 +207,7 @@ static bool __send_ipi_mask(const struct cpumask *mask, int vector)
 	return hv_result_success(status);
 
 do_ex_hypercall:
-	return __send_ipi_mask_ex(mask, vector);
+	return __send_ipi_mask_ex(mask, vector, exclude_self);
 }
 
 static bool __send_ipi_one(int cpu, int vector)

@@ -208,7 +224,7 @@ static bool __send_ipi_one(int cpu, int vector)
 		return false;
 
 	if (vp >= 64)
-		return __send_ipi_mask_ex(cpumask_of(cpu), vector);
+		return __send_ipi_mask_ex(cpumask_of(cpu), vector, false);
 
 	status = hv_do_fast_hypercall16(HVCALL_SEND_IPI, vector, BIT_ULL(vp));
 	return hv_result_success(status);

@@ -222,20 +238,13 @@ static void hv_send_ipi(int cpu, int vector)
 
 static void hv_send_ipi_mask(const struct cpumask *mask, int vector)
 {
-	if (!__send_ipi_mask(mask, vector))
+	if (!__send_ipi_mask(mask, vector, false))
 		orig_apic.send_IPI_mask(mask, vector);
 }
 
 static void hv_send_ipi_mask_allbutself(const struct cpumask *mask, int vector)
 {
-	unsigned int this_cpu = smp_processor_id();
-	struct cpumask new_mask;
-	const struct cpumask *local_mask;
-
-	cpumask_copy(&new_mask, mask);
-	cpumask_clear_cpu(this_cpu, &new_mask);
-	local_mask = &new_mask;
-	if (!__send_ipi_mask(local_mask, vector))
+	if (!__send_ipi_mask(mask, vector, true))
 		orig_apic.send_IPI_mask_allbutself(mask, vector);
 }
 

@@ -246,7 +255,7 @@ static void hv_send_ipi_allbutself(int vector)
 
 static void hv_send_ipi_all(int vector)
 {
-	if (!__send_ipi_mask(cpu_online_mask, vector))
+	if (!__send_ipi_mask(cpu_online_mask, vector, false))
 		orig_apic.send_IPI_all(vector);
 }
 
--

2.30.2

From: Wei Liu <[email protected]> Sent: Friday, September 10, 2021 11:57 AM

Quoted 12 lines

arch/x86/hyperv/hv_apic.c +4 -2

-static bool __send_ipi_mask(const struct cpumask *mask, int vector)
+static bool __send_ipi_mask(const struct cpumask *mask, int vector,
+		bool exclude_self)
 {
-	int cur_cpu, vcpu;
+	int cur_cpu, vcpu, this_cpu = smp_processor_id();
 	struct hv_send_ipi ipi_arg;
 	u64 status;
+	unsigned int weight;
 
 	trace_hyperv_send_ipi_mask(mask, vector);

arch/x86/hyperv/hv_apic.c static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) · line 163

Quoted 10 lines

arch/x86/hyperv/hv_apic.c +9 -1

-	if (cpumask_empty(mask))
+	weight = cpumask_weight(mask);
+
+	/*
+	 * Do nothing if
+	 *   1. the mask is empty
+	 *   2. the mask only contains self when exclude_self is true
+	 */
+	if (weight == 0 ||
+	    (exclude_self && weight == 1 && cpumask_first(mask) == this_cpu))

Nit: cpumask_test_cpu(this_cpu, mask) would seem to be a better fit for this use case than cpumask_first(). But either works.

Wei!

Quoted 8 lines

(quoted diff) +2 -1

-static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector)
+static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector,
+		bool exclude_self)
 {
 	struct hv_send_ipi_ex **arg;
 	struct hv_send_ipi_ex *ipi_arg;

@@ -123,7 +124,10 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector)
 

Review static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) · line 125

Quoted 1 line

(quoted diff) +0 -0

 	if (!cpumask_equal(mask, cpu_present_mask)) {

Not part of that patch, but is checking cpu_present_mask correct here? If so then this really lacks a comment for the casual reader.

Review static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) · line 131

Quoted 8 lines

(quoted diff) +4 -1

 		ipi_arg->vp_set.format = HV_GENERIC_SET_SPARSE_4K;
-		nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask);
+		if (exclude_self)
+			nr_bank = cpumask_to_vpset_noself(&(ipi_arg->vp_set), mask);
+		else
+			nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask);
 	}

But, what happens in the case that mask == cpu_present_mask and exclude_self == true?

AFAICT it ends up sending the IPI to all CPUs including self:

	if (!nr_bank)
		ipi_arg->vp_set.format = HV_GENERIC_SET_ALL;

Not entirely correct, right?

Quoted 2 lines · Thomas Gleixner

Wei!

Not Wei here but I don't see the question answered on the mailing list so let me give my thoughts.

Quoted 9 lines · Wei Liu

Quoted 8 lines

(quoted diff) +2 -1

-static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector)
+static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector,
+		bool exclude_self)
 {
 	struct hv_send_ipi_ex **arg;
 	struct hv_send_ipi_ex *ipi_arg;

@@ -123,7 +124,10 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector)
 

Quoted 4 lines

Review static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) · line 125

Quoted 1 line

(quoted diff) +0 -0

 	if (!cpumask_equal(mask, cpu_present_mask)) {

Not part of that patch, but is checking cpu_present_mask correct here? If so then this really lacks a comment for the casual reader.

It seems it *was* correct prior to 'exclude_self': the idea is that for everything but 'cpu_present_mask' we use HV_GENERIC_SET_SPARSE_4K format, for 'cpu_present_mask' we just use 'all' (HV_GENERIC_SET_ALL) to avoid specifying individual CPUs.

Quoted 13 lines

Review static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) · line 131

Quoted 9 lines

(quoted diff) +4 -1

 		ipi_arg->vp_set.format = HV_GENERIC_SET_SPARSE_4K;
-		nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask);
+		if (exclude_self)
+			nr_bank = cpumask_to_vpset_noself(&(ipi_arg->vp_set), mask);
+		else
+			nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask);
 	}

But, what happens in the case that mask == cpu_present_mask and exclude_self == true?

Quoted 6 lines

AFAICT it ends up sending the IPI to all CPUs including self:

	if (!nr_bank)
		ipi_arg->vp_set.format = HV_GENERIC_SET_ALL;

Not entirely correct, right?

It's not, I think we need something like (completely untested)

arch/x86/hyperv/hv_apic.c +5 -5

index 32a1ad356c18..80b7660208e4 100644
@@ -122,17 +122,17 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector,
        ipi_arg->reserved = 0;
        ipi_arg->vp_set.valid_bank_mask = 0;
 
-       if (!cpumask_equal(mask, cpu_present_mask)) {
+       if (!cpumask_equal(mask, cpu_present_mask) || exclude_self) {
                ipi_arg->vp_set.format = HV_GENERIC_SET_SPARSE_4K;
                if (exclude_self)
                        nr_bank = cpumask_to_vpset_noself(&(ipi_arg->vp_set), mask);
                else
                        nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask);
-       }
-       if (nr_bank < 0)
-               goto ipi_mask_ex_done;
-       if (!nr_bank)
+               if (nr_bank =< 0)
+                       goto ipi_mask_ex_done;
+       } else {
                ipi_arg->vp_set.format = HV_GENERIC_SET_ALL;
+       }
 
        status = hv_do_rep_hypercall(HVCALL_SEND_IPI_EX, 0, nr_bank,
                              ipi_arg, NULL);

here. Wei, I can test and send this out if you're not on it already.

Signature · 1 line

Vitaly

Wei Liu (2):
asm-generic/hyperv: provide cpumask_to_vpset_noself x86/hyperv: remove on-stack cpumask from hv_send_ipi_mask_allbutself

 arch/x86/hyperv/hv_apic.c      | 43 ++++++++++++++++++++--------------
 include/asm-generic/mshyperv.h | 21 +++++++++++++++--
 2 files changed, 45 insertions(+), 19 deletions(-)