RE: [PATCH] x86/hyper-v: remove unnecessary conversions to bool

[Michael Kelley]

From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> Sent: Friday, January 10, 2020 4:00 AM
> 
> Chen Zhou <chenzhou10@xxxxxxxxxx> writes:
> 
> > The conversions to bool are not needed, remove these.
> >
> > Signed-off-by: Chen Zhou <chenzhou10@xxxxxxxxxx>
> > ---
> >  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