| Message ID | 20170224195002.28030-3-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 24.02.2017 um 20:49 schrieb Radim Krčmář:
> The leading underscores denote that the call is just a bitop wrapper.
Actually, the leading underscore is misleading
If we want to match the semantics of set/test/clear_bit, using a leading
underscore might feel like using the non-atomic variants like
__clear_bit and friends.
I'd prefer to simply drop the underscore.
Am 27.02.2017 um 11:02 schrieb David Hildenbrand: > Am 24.02.2017 um 20:49 schrieb Radim Krčmář: >> The leading underscores denote that the call is just a bitop wrapper. > > Actually, the leading underscore is misleading > > If we want to match the semantics of set/test/clear_bit, using a leading > underscore might feel like using the non-atomic variants like > __clear_bit and friends. > > I'd prefer to simply drop the underscore. > Okay, this is not really possible for __kvm_request_set(). Hm.....
On Fri, Feb 24, 2017 at 08:49:59PM +0100, Radim Krčmář wrote: > The leading underscores denote that the call is just a bitop wrapper. > > Switch all users of open-coded set/check/test to kvm_request ones. > > Automated by coccinelle script: > @@ > expression VCPU, REQ; > @@ > -set_bit(REQ, &VCPU->requests) > +__kvm_request_set(REQ, VCPU) > > @@ > expression VCPU, REQ; > @@ > -clear_bit(REQ, &VCPU->requests) > +__kvm_request_clear(REQ, VCPU) > > @@ > expression VCPU, REQ; > @@ > -test_bit(REQ, &VCPU->requests) > +__kvm_request_test(REQ, VCPU) > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > v2: clear_bit in __kvm_request_clear [Paolo] > --- > arch/mips/kvm/emulate.c | 2 +- > arch/powerpc/kvm/book3s_pr.c | 2 +- > arch/powerpc/kvm/book3s_pr_papr.c | 2 +- > arch/powerpc/kvm/booke.c | 4 ++-- > arch/powerpc/kvm/powerpc.c | 2 +- > arch/s390/kvm/kvm-s390.c | 2 +- > arch/x86/kvm/vmx.c | 2 +- > arch/x86/kvm/x86.c | 14 +++++++------- > include/linux/kvm_host.h | 23 ++++++++++++++++++++--- > 9 files changed, 35 insertions(+), 18 deletions(-) > > diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c > index ee4af898bcf6..552ae2b5e911 100644 > --- a/arch/mips/kvm/emulate.c > +++ b/arch/mips/kvm/emulate.c > @@ -865,7 +865,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu) > * check if any I/O interrupts are pending. > */ > if (kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) { > - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); > + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); Shall we just remove above line since we cleared it already? -- peterx
On Tue, Feb 28, 2017 at 03:34:24PM +0800, Peter Xu wrote: [...] > > diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c > > index ee4af898bcf6..552ae2b5e911 100644 > > --- a/arch/mips/kvm/emulate.c > > +++ b/arch/mips/kvm/emulate.c > > @@ -865,7 +865,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu) > > * check if any I/O interrupts are pending. > > */ > > if (kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) { > > - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); > > + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); > > Shall we just remove above line since we cleared it already? Please ignore this since I see patch 4. :-) It'll be nice if patch 4 will be before this one, but it's trivial. Thanks, -- peterx
2017-02-27 11:18+0100, David Hildenbrand: > Am 27.02.2017 um 11:02 schrieb David Hildenbrand: >> Am 24.02.2017 um 20:49 schrieb Radim Krčmář: >>> The leading underscores denote that the call is just a bitop wrapper. >> >> Actually, the leading underscore is misleading >> >> If we want to match the semantics of set/test/clear_bit, using a leading >> underscore might feel like using the non-atomic variants like >> __clear_bit and friends. >> >> I'd prefer to simply drop the underscore. >> > > Okay, this is not really possible for __kvm_request_set(). Hm..... Yeah, requests are always atomic, but have some extra cruft on top of bit operations and underscores are similar in the sense of doing less that the non-underscored version. Also, the underscores were something to make its use look undesirable in the code. kvm_request_set and kvm_request_test_and_clear use a barrier and kvm_request_test could be expected to do so as well. I think that a barrier makes no sense in kvm_request_clear, but called it with underscores for consistency with others and also because I think that some callers of could use a second thought.
2017-02-28 15:40+0800, Peter Xu: > On Tue, Feb 28, 2017 at 03:34:24PM +0800, Peter Xu wrote: > > [...] > >> > diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c >> > index ee4af898bcf6..552ae2b5e911 100644 >> > --- a/arch/mips/kvm/emulate.c >> > +++ b/arch/mips/kvm/emulate.c >> > @@ -865,7 +865,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu) >> > * check if any I/O interrupts are pending. >> > */ >> > if (kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) { >> > - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); >> > + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); >> >> Shall we just remove above line since we cleared it already? > > Please ignore this since I see patch 4. :-) > > It'll be nice if patch 4 will be before this one, but it's trivial. I put [4/5] there to demonstrate that this error would have been less likely with the new naming. I didn't expect that reviewers would go through the coccinelle transformation. :)
On Wed, Mar 01, 2017 at 06:02:49PM +0100, Radim Krčmář wrote: > 2017-02-28 15:40+0800, Peter Xu: > > On Tue, Feb 28, 2017 at 03:34:24PM +0800, Peter Xu wrote: > > > > [...] > > > >> > diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c > >> > index ee4af898bcf6..552ae2b5e911 100644 > >> > --- a/arch/mips/kvm/emulate.c > >> > +++ b/arch/mips/kvm/emulate.c > >> > @@ -865,7 +865,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu) > >> > * check if any I/O interrupts are pending. > >> > */ > >> > if (kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) { > >> > - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); > >> > + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); > >> > >> Shall we just remove above line since we cleared it already? > > > > Please ignore this since I see patch 4. :-) > > > > It'll be nice if patch 4 will be before this one, but it's trivial. > > I put [4/5] there to demonstrate that this error would have been less > likely with the new naming. I didn't expect that reviewers would go > through the coccinelle transformation. :) Yeah, I noticed it mostly because it's the first one touched. Meanwhile, I think it's still worthwhile to go through the patch even it's from cocinelle since sometimes coccinelle might do something that we (or only me?) didn't expect. E.g., afaik it cannot handle well with over-80-chars lines, so we need to wrap them on our own (I got a patch from the author though to fix this, but not yet tested). And also since patches are going to be merged changes, it just feel unsafe if we merge something without reading it. :) Thanks, -- peterx
On Thu, Mar 02, 2017 at 09:15:33AM +0800, Peter Xu wrote: [...] > Meanwhile, I think it's still worthwhile to go through the patch even > it's from cocinelle since sometimes coccinelle might do something that > we (or only me?) didn't expect. E.g., afaik it cannot handle well with > over-80-chars lines, so we need to wrap them on our own (I got a patch > from the author though to fix this, but not yet tested). Sorry I should definitely mention that that issue only exists in special conditions, iirc like printf("string1" "string2") and when the strings are very long, since I believe in most cases coccinelle works perfectly well. Thanks, -- peterx
diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c index ee4af898bcf6..552ae2b5e911 100644 --- a/arch/mips/kvm/emulate.c +++ b/arch/mips/kvm/emulate.c @@ -865,7 +865,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu) * check if any I/O interrupts are pending. */ if (kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) { - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN; } } diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c index 7af5154e848b..f5894d27a8a9 100644 --- a/arch/powerpc/kvm/book3s_pr.c +++ b/arch/powerpc/kvm/book3s_pr.c @@ -349,7 +349,7 @@ static void kvmppc_set_msr_pr(struct kvm_vcpu *vcpu, u64 msr) if (msr & MSR_POW) { if (!vcpu->arch.pending_exceptions) { kvm_vcpu_block(vcpu); - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); vcpu->stat.halt_wakeup++; /* Unset POW bit after we woke up */ diff --git a/arch/powerpc/kvm/book3s_pr_papr.c b/arch/powerpc/kvm/book3s_pr_papr.c index f102616febc7..86847220811a 100644 --- a/arch/powerpc/kvm/book3s_pr_papr.c +++ b/arch/powerpc/kvm/book3s_pr_papr.c @@ -344,7 +344,7 @@ int kvmppc_h_pr(struct kvm_vcpu *vcpu, unsigned long cmd) case H_CEDE: kvmppc_set_msr_fast(vcpu, kvmppc_get_msr(vcpu) | MSR_EE); kvm_vcpu_block(vcpu); - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); vcpu->stat.halt_wakeup++; return EMULATE_DONE; case H_LOGICAL_CI_LOAD: diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index 806caaf60e10..e9098af0ab2a 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -579,7 +579,7 @@ static void arm_next_watchdog(struct kvm_vcpu *vcpu) * userspace, so clear the KVM_REQ_WATCHDOG request. */ if ((vcpu->arch.tsr & (TSR_ENW | TSR_WIS)) != (TSR_ENW | TSR_WIS)) - clear_bit(KVM_REQ_WATCHDOG, &vcpu->requests); + __kvm_request_clear(KVM_REQ_WATCHDOG, vcpu); spin_lock_irqsave(&vcpu->arch.wdt_lock, flags); nr_jiffies = watchdog_next_timeout(vcpu); @@ -690,7 +690,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu) if (vcpu->arch.shared->msr & MSR_WE) { local_irq_enable(); kvm_vcpu_block(vcpu); - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); hard_irq_disable(); kvmppc_set_exit_type(vcpu, EMULATED_MTMSRWE_EXITS); diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 03f563000160..2bcc03548c03 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -231,7 +231,7 @@ int kvmppc_kvm_pv(struct kvm_vcpu *vcpu) case EV_HCALL_TOKEN(EV_IDLE): r = EV_SUCCESS; kvm_vcpu_block(vcpu); - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); break; default: r = EV_UNIMPLEMENTED; diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 8fb210974c42..b10acac8facf 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2444,7 +2444,7 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) } /* nothing to do, just clear the request */ - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); return 0; } diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 398d2d5f6d5c..4b005beaaa8e 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -6343,7 +6343,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu) if (intr_window_requested && vmx_interrupt_allowed(vcpu)) return handle_interrupt_window(&vmx->vcpu); - if (test_bit(KVM_REQ_EVENT, &vcpu->requests)) + if (__kvm_request_test(KVM_REQ_EVENT, vcpu)) return 1; err = emulate_instruction(vcpu, EMULTYPE_NO_REEXECUTE); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 3ceb1339ed86..16493795ab21 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1767,7 +1767,7 @@ static void kvm_gen_update_masterclock(struct kvm *kvm) /* guest entries allowed */ kvm_for_each_vcpu(i, vcpu, kvm) - clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests); + __kvm_request_clear(KVM_REQ_MCLOCK_INPROGRESS, vcpu); spin_unlock(&ka->pvclock_gtod_sync_lock); #endif @@ -2228,8 +2228,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) bool tmp = (msr == MSR_KVM_SYSTEM_TIME); if (ka->boot_vcpu_runs_old_kvmclock != tmp) - set_bit(KVM_REQ_MASTERCLOCK_UPDATE, - &vcpu->requests); + __kvm_request_set(KVM_REQ_MASTERCLOCK_UPDATE, + vcpu); ka->boot_vcpu_runs_old_kvmclock = tmp; } @@ -2816,7 +2816,7 @@ static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu) static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu) { - set_bit(KVM_REQ_MIGRATE_TIMER, &vcpu->requests); + __kvm_request_set(KVM_REQ_MIGRATE_TIMER, vcpu); } void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) @@ -7049,7 +7049,7 @@ static int vcpu_run(struct kvm_vcpu *vcpu) if (r <= 0) break; - clear_bit(KVM_REQ_PENDING_TIMER, &vcpu->requests); + __kvm_request_clear(KVM_REQ_PENDING_TIMER, vcpu); if (kvm_cpu_has_pending_timer(vcpu)) kvm_inject_pending_timer_irqs(vcpu); @@ -7177,7 +7177,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) { kvm_vcpu_block(vcpu); kvm_apic_accept_events(vcpu); - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); r = -EAGAIN; goto out; } @@ -8382,7 +8382,7 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu) if (atomic_read(&vcpu->arch.nmi_queued)) return true; - if (test_bit(KVM_REQ_SMI, &vcpu->requests)) + if (__kvm_request_test(KVM_REQ_SMI, vcpu)) return true; if (kvm_arch_interrupt_allowed(vcpu) && diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 4cf61ee84b2e..6d637dbbb82f 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1099,10 +1099,17 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) * * TODO: * - completely encapsulate vcpu->requests + * - do not use __kvm_request* outside request helpers * - do not use memory barrier in (1) and (2) * - let architectures define custom vcpu kick * - add kick when setting remote request */ + +static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) +{ + set_bit(req, &vcpu->requests); +} + static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) { /* @@ -1111,13 +1118,23 @@ static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) * Paired with the smp_mb__after_atomic in kvm_request_test_and_clear. */ smp_wmb(); - set_bit(req, &vcpu->requests); + __kvm_request_set(req, vcpu); +} + +static inline bool __kvm_request_test(unsigned req, struct kvm_vcpu *vcpu) +{ + return test_bit(req, &vcpu->requests); +} + +static inline void __kvm_request_clear(unsigned req, struct kvm_vcpu *vcpu) +{ + clear_bit(req, &vcpu->requests); } static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcpu) { - if (test_bit(req, &vcpu->requests)) { - clear_bit(req, &vcpu->requests); + if (__kvm_request_test(req, vcpu)) { + __kvm_request_clear(req, vcpu); /* * Ensure the rest of the request is visible to
The leading underscores denote that the call is just a bitop wrapper. Switch all users of open-coded set/check/test to kvm_request ones. Automated by coccinelle script: @@ expression VCPU, REQ; @@ -set_bit(REQ, &VCPU->requests) +__kvm_request_set(REQ, VCPU) @@ expression VCPU, REQ; @@ -clear_bit(REQ, &VCPU->requests) +__kvm_request_clear(REQ, VCPU) @@ expression VCPU, REQ; @@ -test_bit(REQ, &VCPU->requests) +__kvm_request_test(REQ, VCPU) Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- v2: clear_bit in __kvm_request_clear [Paolo] --- arch/mips/kvm/emulate.c | 2 +- arch/powerpc/kvm/book3s_pr.c | 2 +- arch/powerpc/kvm/book3s_pr_papr.c | 2 +- arch/powerpc/kvm/booke.c | 4 ++-- arch/powerpc/kvm/powerpc.c | 2 +- arch/s390/kvm/kvm-s390.c | 2 +- arch/x86/kvm/vmx.c | 2 +- arch/x86/kvm/x86.c | 14 +++++++------- include/linux/kvm_host.h | 23 ++++++++++++++++++++--- 9 files changed, 35 insertions(+), 18 deletions(-)