* [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 related [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 related [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, other threads:[~2020-01-13 8:07 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).