Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] x86/hyper-v: remove unnecessary conversions to bool
@ 2020-01-10  7:20 Chen Zhou
  2020-01-10 11:59 ` Vitaly Kuznetsov
  0 siblings, 1 reply; 4+ messages in thread
From: Chen Zhou @ 2020-01-10  7:20 UTC (permalink / raw)
  To: tglx, mingo; +Cc: linux-hyperv, linux-kernel, chenzhou10

The conversions to bool are not needed, remove these.

Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
---
 arch/x86/hyperv/hv_apic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
index 40e0e32..3112cf6 100644
--- a/arch/x86/hyperv/hv_apic.c
+++ b/arch/x86/hyperv/hv_apic.c
@@ -133,7 +133,7 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector)
 
 ipi_mask_ex_done:
 	local_irq_restore(flags);
-	return ((ret == 0) ? true : false);
+	return ret == 0;
 }
 
 static bool __send_ipi_mask(const struct cpumask *mask, int vector)
@@ -186,7 +186,7 @@ static bool __send_ipi_mask(const struct cpumask *mask, int vector)
 
 	ret = hv_do_fast_hypercall16(HVCALL_SEND_IPI, ipi_arg.vector,
 				     ipi_arg.cpu_mask);
-	return ((ret == 0) ? true : false);
+	return ret == 0;
 
 do_ex_hypercall:
 	return __send_ipi_mask_ex(mask, vector);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] x86/hyper-v: remove unnecessary conversions to bool
  2020-01-10  7:20 [PATCH] x86/hyper-v: remove unnecessary conversions to bool Chen Zhou
@ 2020-01-10 11:59 ` Vitaly Kuznetsov
  2020-01-12 16:43   ` Michael Kelley
  0 siblings, 1 reply; 4+ messages in thread
From: Vitaly Kuznetsov @ 2020-01-10 11:59 UTC (permalink / raw)
  To: Chen Zhou; +Cc: linux-hyperv, linux-kernel, chenzhou10, tglx, mingo

Chen Zhou <chenzhou10@huawei.com> writes:

> The conversions to bool are not needed, remove these.
>
> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
> ---
>  arch/x86/hyperv/hv_apic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
> index 40e0e32..3112cf6 100644
> --- a/arch/x86/hyperv/hv_apic.c
> +++ b/arch/x86/hyperv/hv_apic.c
> @@ -133,7 +133,7 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector)
>  
>  ipi_mask_ex_done:
>  	local_irq_restore(flags);
> -	return ((ret == 0) ? true : false);
> +	return ret == 0;
>  }
>  
>  static bool __send_ipi_mask(const struct cpumask *mask, int vector)
> @@ -186,7 +186,7 @@ static bool __send_ipi_mask(const struct cpumask *mask, int vector)
>  
>  	ret = hv_do_fast_hypercall16(HVCALL_SEND_IPI, ipi_arg.vector,
>  				     ipi_arg.cpu_mask);
> -	return ((ret == 0) ? true : false);
> +	return ret == 0;
>  
>  do_ex_hypercall:
>  	return __send_ipi_mask_ex(mask, vector);

I'd suggest we get rid of bool functions completely instead, something
like (untested):

diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
index 40e0e322161d..440bda338763 100644
--- a/arch/x86/hyperv/hv_apic.c
+++ b/arch/x86/hyperv/hv_apic.c
@@ -97,16 +97,16 @@ 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 u16 __send_ipi_mask_ex(const struct cpumask *mask, int vector)
 {
 	struct hv_send_ipi_ex **arg;
 	struct hv_send_ipi_ex *ipi_arg;
 	unsigned long flags;
 	int nr_bank = 0;
-	int ret = 1;
+	u16 ret;
 
 	if (!(ms_hyperv.hints & HV_X64_EX_PROCESSOR_MASKS_RECOMMENDED))
-		return false;
+		return U16_MAX;
 
 	local_irq_save(flags);
 	arg = (struct hv_send_ipi_ex **)this_cpu_ptr(hyperv_pcpu_input_arg);
@@ -129,29 +129,28 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector)
 		ipi_arg->vp_set.format = HV_GENERIC_SET_ALL;
 
 	ret = hv_do_rep_hypercall(HVCALL_SEND_IPI_EX, 0, nr_bank,
-			      ipi_arg, NULL);
+				  ipi_arg, NULL);
 
 ipi_mask_ex_done:
 	local_irq_restore(flags);
-	return ((ret == 0) ? true : false);
+	return ret;
 }
 
-static bool __send_ipi_mask(const struct cpumask *mask, int vector)
+static u16 __send_ipi_mask(const struct cpumask *mask, int vector)
 {
 	int cur_cpu, vcpu;
 	struct hv_send_ipi ipi_arg;
-	int ret = 1;
 
 	trace_hyperv_send_ipi_mask(mask, vector);
 
 	if (cpumask_empty(mask))
-		return true;
+		return 0;
 
 	if (!hv_hypercall_pg)
-		return false;
+		return U16_MAX;
 
 	if ((vector < HV_IPI_LOW_VECTOR) || (vector > HV_IPI_HIGH_VECTOR))
-		return false;
+		return U16_MAX;
 
 	/*
 	 * From the supplied CPU set we need to figure out if we can get away
@@ -172,7 +171,7 @@ static bool __send_ipi_mask(const struct cpumask *mask, int vector)
 	for_each_cpu(cur_cpu, mask) {
 		vcpu = hv_cpu_number_to_vp_number(cur_cpu);
 		if (vcpu == VP_INVAL)
-			return false;
+			return U16_MAX;
 
 		/*
 		 * This particular version of the IPI hypercall can
@@ -184,41 +183,40 @@ static bool __send_ipi_mask(const struct cpumask *mask, int vector)
 		__set_bit(vcpu, (unsigned long *)&ipi_arg.cpu_mask);
 	}
 
-	ret = hv_do_fast_hypercall16(HVCALL_SEND_IPI, ipi_arg.vector,
-				     ipi_arg.cpu_mask);
-	return ((ret == 0) ? true : false);
+	return (u16)hv_do_fast_hypercall16(HVCALL_SEND_IPI, ipi_arg.vector,
+					   ipi_arg.cpu_mask);
 
 do_ex_hypercall:
 	return __send_ipi_mask_ex(mask, vector);
 }
 
-static bool __send_ipi_one(int cpu, int vector)
+static u16 __send_ipi_one(int cpu, int vector)
 {
 	int vp = hv_cpu_number_to_vp_number(cpu);
 
 	trace_hyperv_send_ipi_one(cpu, vector);
 
 	if (!hv_hypercall_pg || (vp == VP_INVAL))
-		return false;
+		return U16_MAX;
 
 	if ((vector < HV_IPI_LOW_VECTOR) || (vector > HV_IPI_HIGH_VECTOR))
-		return false;
+		return U16_MAX;
 
 	if (vp >= 64)
 		return __send_ipi_mask_ex(cpumask_of(cpu), vector);
 
-	return !hv_do_fast_hypercall16(HVCALL_SEND_IPI, vector, BIT_ULL(vp));
+	return (u16)hv_do_fast_hypercall16(HVCALL_SEND_IPI, vector, BIT_ULL(vp));
 }
 
 static void hv_send_ipi(int cpu, int vector)
 {
-	if (!__send_ipi_one(cpu, vector))
+	if (__send_ipi_one(cpu, vector))
 		orig_apic.send_IPI(cpu, 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))
 		orig_apic.send_IPI_mask(mask, vector);
 }
 
@@ -231,7 +229,7 @@ static void hv_send_ipi_mask_allbutself(const struct cpumask *mask, int vector)
 	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(local_mask, vector))
 		orig_apic.send_IPI_mask_allbutself(mask, vector);
 }
 
@@ -242,13 +240,13 @@ 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))
 		orig_apic.send_IPI_all(vector);
 }
 
 static void hv_send_ipi_self(int vector)
 {
-	if (!__send_ipi_one(smp_processor_id(), vector))
+	if (__send_ipi_one(smp_processor_id(), vector))
 		orig_apic.send_IPI_self(vector);
 }

-- 
Vitaly


^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] x86/hyper-v: remove unnecessary conversions to bool
  2020-01-10 11:59 ` Vitaly Kuznetsov
@ 2020-01-12 16:43   ` Michael Kelley
  2020-01-13  8:07     ` Vitaly Kuznetsov
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Kelley @ 2020-01-12 16:43 UTC (permalink / raw)
  To: vkuznets, Chen Zhou; +Cc: linux-hyperv, linux-kernel, chenzhou10, tglx, mingo

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Friday, January 10, 2020 4:00 AM
> 
> Chen Zhou <chenzhou10@huawei.com> writes:
> 
> > The conversions to bool are not needed, remove these.
> >
> > Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
> > ---
> >  arch/x86/hyperv/hv_apic.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
> > index 40e0e32..3112cf6 100644
> > --- a/arch/x86/hyperv/hv_apic.c
> > +++ b/arch/x86/hyperv/hv_apic.c
> > @@ -133,7 +133,7 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int
> vector)
> >
> >  ipi_mask_ex_done:
> >  	local_irq_restore(flags);
> > -	return ((ret == 0) ? true : false);
> > +	return ret == 0;
> >  }
> >
> >  static bool __send_ipi_mask(const struct cpumask *mask, int vector)
> > @@ -186,7 +186,7 @@ static bool __send_ipi_mask(const struct cpumask *mask, int
> vector)
> >
> >  	ret = hv_do_fast_hypercall16(HVCALL_SEND_IPI, ipi_arg.vector,
> >  				     ipi_arg.cpu_mask);
> > -	return ((ret == 0) ? true : false);
> > +	return ret == 0;
> >
> >  do_ex_hypercall:
> >  	return __send_ipi_mask_ex(mask, vector);
> 
> I'd suggest we get rid of bool functions completely instead, something
> like (untested):

Just curious:  Why prefer returning a u16 instead of a bool?  To avoid
having to test 'ret' for zero in the return statements, or is there some
broader reason?

> 
> diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
> index 40e0e322161d..440bda338763 100644
> --- a/arch/x86/hyperv/hv_apic.c
> +++ b/arch/x86/hyperv/hv_apic.c
> @@ -97,16 +97,16 @@ 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 u16 __send_ipi_mask_ex(const struct cpumask *mask, int vector)
>  {
>  	struct hv_send_ipi_ex **arg;
>  	struct hv_send_ipi_ex *ipi_arg;
>  	unsigned long flags;
>  	int nr_bank = 0;
> -	int ret = 1;
> +	u16 ret;
> 
>  	if (!(ms_hyperv.hints & HV_X64_EX_PROCESSOR_MASKS_RECOMMENDED))
> -		return false;
> +		return U16_MAX;
> 
>  	local_irq_save(flags);
>  	arg = (struct hv_send_ipi_ex **)this_cpu_ptr(hyperv_pcpu_input_arg);
> @@ -129,29 +129,28 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int
> vector)
>  		ipi_arg->vp_set.format = HV_GENERIC_SET_ALL;
> 
>  	ret = hv_do_rep_hypercall(HVCALL_SEND_IPI_EX, 0, nr_bank,
> -			      ipi_arg, NULL);
> +				  ipi_arg, NULL);
> 
>  ipi_mask_ex_done:
>  	local_irq_restore(flags);
> -	return ((ret == 0) ? true : false);
> +	return ret;
>  }
> 
> -static bool __send_ipi_mask(const struct cpumask *mask, int vector)
> +static u16 __send_ipi_mask(const struct cpumask *mask, int vector)
>  {
>  	int cur_cpu, vcpu;
>  	struct hv_send_ipi ipi_arg;
> -	int ret = 1;
> 
>  	trace_hyperv_send_ipi_mask(mask, vector);
> 
>  	if (cpumask_empty(mask))
> -		return true;
> +		return 0;
> 
>  	if (!hv_hypercall_pg)
> -		return false;
> +		return U16_MAX;
> 
>  	if ((vector < HV_IPI_LOW_VECTOR) || (vector > HV_IPI_HIGH_VECTOR))
> -		return false;
> +		return U16_MAX;
> 
>  	/*
>  	 * From the supplied CPU set we need to figure out if we can get away
> @@ -172,7 +171,7 @@ static bool __send_ipi_mask(const struct cpumask *mask, int
> vector)
>  	for_each_cpu(cur_cpu, mask) {
>  		vcpu = hv_cpu_number_to_vp_number(cur_cpu);
>  		if (vcpu == VP_INVAL)
> -			return false;
> +			return U16_MAX;
> 
>  		/*
>  		 * This particular version of the IPI hypercall can
> @@ -184,41 +183,40 @@ static bool __send_ipi_mask(const struct cpumask *mask, int
> vector)
>  		__set_bit(vcpu, (unsigned long *)&ipi_arg.cpu_mask);
>  	}
> 
> -	ret = hv_do_fast_hypercall16(HVCALL_SEND_IPI, ipi_arg.vector,
> -				     ipi_arg.cpu_mask);
> -	return ((ret == 0) ? true : false);
> +	return (u16)hv_do_fast_hypercall16(HVCALL_SEND_IPI, ipi_arg.vector,
> +					   ipi_arg.cpu_mask);

The cast to u16 seems a bit dangerous. The hypercall status code is indeed
returned in the low 16 bits of the hypercall result value, so it works, and
maybe that is why you suggested u16 as the function return value.  But it
is a non-obvious assumption.  

Michael

> 
>  do_ex_hypercall:
>  	return __send_ipi_mask_ex(mask, vector);
>  }
> 
> -static bool __send_ipi_one(int cpu, int vector)
> +static u16 __send_ipi_one(int cpu, int vector)
>  {
>  	int vp = hv_cpu_number_to_vp_number(cpu);
> 
>  	trace_hyperv_send_ipi_one(cpu, vector);
> 
>  	if (!hv_hypercall_pg || (vp == VP_INVAL))
> -		return false;
> +		return U16_MAX;
> 
>  	if ((vector < HV_IPI_LOW_VECTOR) || (vector > HV_IPI_HIGH_VECTOR))
> -		return false;
> +		return U16_MAX;
> 
>  	if (vp >= 64)
>  		return __send_ipi_mask_ex(cpumask_of(cpu), vector);
> 
> -	return !hv_do_fast_hypercall16(HVCALL_SEND_IPI, vector, BIT_ULL(vp));
> +	return (u16)hv_do_fast_hypercall16(HVCALL_SEND_IPI, vector, BIT_ULL(vp));
>  }
> 
>  static void hv_send_ipi(int cpu, int vector)
>  {
> -	if (!__send_ipi_one(cpu, vector))
> +	if (__send_ipi_one(cpu, vector))
>  		orig_apic.send_IPI(cpu, 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))
>  		orig_apic.send_IPI_mask(mask, vector);
>  }
> 
> @@ -231,7 +229,7 @@ static void hv_send_ipi_mask_allbutself(const struct cpumask
> *mask, int vector)
>  	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(local_mask, vector))
>  		orig_apic.send_IPI_mask_allbutself(mask, vector);
>  }
> 
> @@ -242,13 +240,13 @@ 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))
>  		orig_apic.send_IPI_all(vector);
>  }
> 
>  static void hv_send_ipi_self(int vector)
>  {
> -	if (!__send_ipi_one(smp_processor_id(), vector))
> +	if (__send_ipi_one(smp_processor_id(), vector))
>  		orig_apic.send_IPI_self(vector);
>  }
> 
> --
> Vitaly


^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] x86/hyper-v: remove unnecessary conversions to bool
  2020-01-12 16:43   ` Michael Kelley
@ 2020-01-13  8:07     ` Vitaly Kuznetsov
  0 siblings, 0 replies; 4+ messages in thread
From: Vitaly Kuznetsov @ 2020-01-13  8:07 UTC (permalink / raw)
  To: Michael Kelley, Chen Zhou
  Cc: linux-hyperv\, linux-kernel\, chenzhou10\, tglx\, mingo\

Michael Kelley <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Friday, January 10, 2020 4:00 AM
>> 
>> 
>> I'd suggest we get rid of bool functions completely instead, something
>> like (untested):
>
> Just curious:  Why prefer returning a u16 instead of a bool?  To avoid
> having to test 'ret' for zero in the return statements, or is there some
> broader reason?

Basically to preserve hypercall failure code and not hide it under 'false'.

>> -				     ipi_arg.cpu_mask);
>> -	return ((ret == 0) ? true : false);
>> +	return (u16)hv_do_fast_hypercall16(HVCALL_SEND_IPI, ipi_arg.vector,
>> +					   ipi_arg.cpu_mask);
>
> The cast to u16 seems a bit dangerous. The hypercall status code is indeed
> returned in the low 16 bits of the hypercall result value, so it works, and
> maybe that is why you suggested u16 as the function return value.  But it
> is a non-obvious assumption.  

This is not obvious, I agree, and we can create a wrapper for it but we
more or less must convert it to 'u16': uppper bits don't indicate a
failure (e.g. 'reps complete').

-- 
Vitaly


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-10  7:20 [PATCH] x86/hyper-v: remove unnecessary conversions to bool Chen Zhou
2020-01-10 11:59 ` Vitaly Kuznetsov
2020-01-12 16:43   ` Michael Kelley
2020-01-13  8:07     ` Vitaly Kuznetsov

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git