* [PATCH] virt: kvm: arm: vgic: Return failure code '-EBUSY' when mutex_trylock() fails @ 2014-11-12 15:04 Chen Gang 2014-11-12 19:41 ` Christoffer Dall 0 siblings, 1 reply; 3+ messages in thread From: Chen Gang @ 2014-11-12 15:04 UTC (permalink / raw) To: linux-arm-kernel When mutex_trylock() fails, kvm_vgic_create() will not create 'vgic', so it need return failure code '-EBUSY' instead of '0' to let outside know about it. Also simplify the code within kvm_vgic_create(): - kvm_for_each_vcpu() scanning once is enough for current case. - Remove redundant variable 'vcpu_lock_idx'. Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- virt/kvm/arm/vgic.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 3aaca49..5846725 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1933,7 +1933,7 @@ out: int kvm_vgic_create(struct kvm *kvm) { - int i, vcpu_lock_idx = -1, ret = 0; + int i, ret = 0; struct kvm_vcpu *vcpu; mutex_lock(&kvm->lock); @@ -1949,13 +1949,12 @@ int kvm_vgic_create(struct kvm *kvm) * that no other VCPUs are run while we create the vgic. */ kvm_for_each_vcpu(i, vcpu, kvm) { - if (!mutex_trylock(&vcpu->mutex)) + if (!mutex_trylock(&vcpu->mutex)) { + ret = -EBUSY; goto out_unlock; - vcpu_lock_idx = i; - } - - kvm_for_each_vcpu(i, vcpu, kvm) { + } if (vcpu->arch.has_run_once) { + mutex_unlock(&vcpu->mutex); ret = -EBUSY; goto out_unlock; } @@ -1968,8 +1967,8 @@ int kvm_vgic_create(struct kvm *kvm) kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF; out_unlock: - for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) { - vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx); + while (i-- > 0) { + vcpu = kvm_get_vcpu(kvm, i); mutex_unlock(&vcpu->mutex); } -- 1.9.3 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH] virt: kvm: arm: vgic: Return failure code '-EBUSY' when mutex_trylock() fails 2014-11-12 15:04 [PATCH] virt: kvm: arm: vgic: Return failure code '-EBUSY' when mutex_trylock() fails Chen Gang @ 2014-11-12 19:41 ` Christoffer Dall 2014-11-13 2:34 ` Chen Gang 0 siblings, 1 reply; 3+ messages in thread From: Christoffer Dall @ 2014-11-12 19:41 UTC (permalink / raw) To: linux-arm-kernel On Wed, Nov 12, 2014 at 11:04:23PM +0800, Chen Gang wrote: > When mutex_trylock() fails, kvm_vgic_create() will not create 'vgic', so > it need return failure code '-EBUSY' instead of '0' to let outside know > about it. I already sent a patch for the -EBUSY: https://lists.cs.columbia.edu/pipermail/kvmarm/2014-November/011936.html > > Also simplify the code within kvm_vgic_create(): > > - kvm_for_each_vcpu() scanning once is enough for current case. > > - Remove redundant variable 'vcpu_lock_idx'. I don't like using the iterator variable for this kind of thing because it can really break in languages where i is out-of-scope after the loop and it is too easy to reuse the iterator variable in later versions of the code. That being said, the scanning once is slightly prettier I guess, but I'd rather not introduce the churn unless others (Marc) think this is a big win. -Christoffer > > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > virt/kvm/arm/vgic.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > index 3aaca49..5846725 100644 > --- a/virt/kvm/arm/vgic.c > +++ b/virt/kvm/arm/vgic.c > @@ -1933,7 +1933,7 @@ out: > > int kvm_vgic_create(struct kvm *kvm) > { > - int i, vcpu_lock_idx = -1, ret = 0; > + int i, ret = 0; > struct kvm_vcpu *vcpu; > > mutex_lock(&kvm->lock); > @@ -1949,13 +1949,12 @@ int kvm_vgic_create(struct kvm *kvm) > * that no other VCPUs are run while we create the vgic. > */ > kvm_for_each_vcpu(i, vcpu, kvm) { > - if (!mutex_trylock(&vcpu->mutex)) > + if (!mutex_trylock(&vcpu->mutex)) { > + ret = -EBUSY; > goto out_unlock; > - vcpu_lock_idx = i; > - } > - > - kvm_for_each_vcpu(i, vcpu, kvm) { > + } > if (vcpu->arch.has_run_once) { > + mutex_unlock(&vcpu->mutex); > ret = -EBUSY; > goto out_unlock; > } > @@ -1968,8 +1967,8 @@ int kvm_vgic_create(struct kvm *kvm) > kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF; > > out_unlock: > - for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) { > - vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx); > + while (i-- > 0) { > + vcpu = kvm_get_vcpu(kvm, i); > mutex_unlock(&vcpu->mutex); > } > > -- > 1.9.3 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] virt: kvm: arm: vgic: Return failure code '-EBUSY' when mutex_trylock() fails 2014-11-12 19:41 ` Christoffer Dall @ 2014-11-13 2:34 ` Chen Gang 0 siblings, 0 replies; 3+ messages in thread From: Chen Gang @ 2014-11-13 2:34 UTC (permalink / raw) To: linux-arm-kernel On 11/13/14 3:41, Christoffer Dall wrote: > On Wed, Nov 12, 2014 at 11:04:23PM +0800, Chen Gang wrote: >> When mutex_trylock() fails, kvm_vgic_create() will not create 'vgic', so >> it need return failure code '-EBUSY' instead of '0' to let outside know >> about it. > > I already sent a patch for the -EBUSY: > https://lists.cs.columbia.edu/pipermail/kvmarm/2014-November/011936.html > Yeah, really it is. >> >> Also simplify the code within kvm_vgic_create(): >> >> - kvm_for_each_vcpu() scanning once is enough for current case. >> >> - Remove redundant variable 'vcpu_lock_idx'. > > I don't like using the iterator variable for this kind of thing because > it can really break in languages where i is out-of-scope after the loop > and it is too easy to reuse the iterator variable in later versions of > the code. > For me, what you said is OK, we can still keep it no touch. > That being said, the scanning once is slightly prettier I guess, > but I'd rather not introduce the churn unless others (Marc) think this > is a big win. > If only merge the 2 scanning loops, it will not change much. And also can let code simpler and clearer for readers (both are for processing and checking '-EBUSY'). If possible, after your patch merges into linux next tree, I will send the related improving patch for it. Thanks. > -Christoffer > >> >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> --- >> virt/kvm/arm/vgic.c | 15 +++++++-------- >> 1 file changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c >> index 3aaca49..5846725 100644 >> --- a/virt/kvm/arm/vgic.c >> +++ b/virt/kvm/arm/vgic.c >> @@ -1933,7 +1933,7 @@ out: >> >> int kvm_vgic_create(struct kvm *kvm) >> { >> - int i, vcpu_lock_idx = -1, ret = 0; >> + int i, ret = 0; >> struct kvm_vcpu *vcpu; >> >> mutex_lock(&kvm->lock); >> @@ -1949,13 +1949,12 @@ int kvm_vgic_create(struct kvm *kvm) >> * that no other VCPUs are run while we create the vgic. >> */ >> kvm_for_each_vcpu(i, vcpu, kvm) { >> - if (!mutex_trylock(&vcpu->mutex)) >> + if (!mutex_trylock(&vcpu->mutex)) { >> + ret = -EBUSY; >> goto out_unlock; >> - vcpu_lock_idx = i; >> - } >> - >> - kvm_for_each_vcpu(i, vcpu, kvm) { >> + } >> if (vcpu->arch.has_run_once) { >> + mutex_unlock(&vcpu->mutex); >> ret = -EBUSY; >> goto out_unlock; >> } >> @@ -1968,8 +1967,8 @@ int kvm_vgic_create(struct kvm *kvm) >> kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF; >> >> out_unlock: >> - for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) { >> - vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx); >> + while (i-- > 0) { >> + vcpu = kvm_get_vcpu(kvm, i); >> mutex_unlock(&vcpu->mutex); >> } >> >> -- >> 1.9.3 -- Chen Gang Open, share, and attitude like air, water, and life which God blessed ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-11-13 2:34 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-12 15:04 [PATCH] virt: kvm: arm: vgic: Return failure code '-EBUSY' when mutex_trylock() fails Chen Gang 2014-11-12 19:41 ` Christoffer Dall 2014-11-13 2:34 ` Chen Gang
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).