| Message ID | 1383805375-14766-2-git-send-email-pingfank@linux.vnet.ibm.com |
|---|---|
| State | New, archived |
| Headers | show |
On 07.11.2013, at 07:22, Liu Ping Fan <kernelfans@gmail.com> wrote: > ret is assigned twice with the same value, so remove the later one. > > Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > Acked-by: Paul Mackerras <paulus@samba.org> I suppose my last request for a patch description was slightly too abbreviated :). Sorry about that. Imagine you are a Linux-stable maintainer. You have about 5000 patches in front of you and you want to figure out whether a patch should get backported into a stable tree or not. It's very easy to read through the patch description. It's reasonably easy to do a git show on the patch. It's very hard to look at the actual surrounding code that was changed. If I open a text editor on the file, I immediately see what you're saying: > ret = RESUME_GUEST; > preempt_disable(); > while (!try_lock_hpte(hptep, HPTE_V_HVLOCK)) > cpu_relax(); > if ((hptep[0] & ~HPTE_V_HVLOCK) != hpte[0] || hptep[1] != hpte[1] || > rev->guest_rpte != hpte[2]) > /* HPTE has been changed under us; let the guest retry */ > goto out_unlock; > hpte[0] = (hpte[0] & ~HPTE_V_ABSENT) | HPTE_V_VALID; > > rmap = &memslot->arch.rmap[gfn - memslot->base_gfn]; > lock_rmap(rmap); > > /* Check if we might have been invalidated; let the guest retry if so */ > ret = RESUME_GUEST; However, that scope is not given in the actual patch itself. If you look at the diff below, you have no idea whether the patch is fixing a bug or just removes duplication and doesn't actually have any effect. In fact, the compiled assembly should be the same with this patch and without. But you can't tell from the diff below. So what I would like to see in the patch description is something that makes it easy to understand what's going on without the need to check out the source file. Something like > We redundantly set ret to RESUME_GUEST twice without changing it in between. Only do it once: > > ret = RESUME_GUEST; > preempt_disable(); > while (!try_lock_hpte(hptep, HPTE_V_HVLOCK)) > cpu_relax(); > if ((hptep[0] & ~HPTE_V_HVLOCK) != hpte[0] || hptep[1] != hpte[1] || > rev->guest_rpte != hpte[2]) > /* HPTE has been changed under us; let the guest retry */ > goto out_unlock; > hpte[0] = (hpte[0] & ~HPTE_V_ABSENT) | HPTE_V_VALID; > > rmap = &memslot->arch.rmap[gfn - memslot->base_gfn]; > lock_rmap(rmap); > > /* Check if we might have been invalidated; let the guest retry if so */ > ret = RESUME_GUEST; If I look at that patch description it immediately tells me "Ah, no need to worry, it's not a critical bug I need to backport". If you have a better idea how to express that I'm more than happy to take that too. Otherwise just let me know whether you like the description above and I'll modify it to the one that includes the code snippet when applying the patch. Thanks a lot, Alex > --- > arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c > index dbc1478..9b97b42 100644 > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c > @@ -733,7 +733,6 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu, > lock_rmap(rmap); > > /* Check if we might have been invalidated; let the guest retry if so */ > - ret = RESUME_GUEST; > if (mmu_notifier_retry(vcpu->kvm, mmu_seq)) { > unlock_rmap(rmap); > goto out_unlock; > -- > 1.8.1.4 > -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 7, 2013 at 6:06 PM, Alexander Graf <agraf@suse.de> wrote: > > On 07.11.2013, at 07:22, Liu Ping Fan <kernelfans@gmail.com> wrote: > >> ret is assigned twice with the same value, so remove the later one. >> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >> Acked-by: Paul Mackerras <paulus@samba.org> > > I suppose my last request for a patch description was slightly too abbreviated :). Sorry about that. > > Imagine you are a Linux-stable maintainer. You have about 5000 patches in front of you and you want to figure out whether a patch should get backported into a stable tree or not. > > It's very easy to read through the patch description. > It's reasonably easy to do a git show on the patch. > It's very hard to look at the actual surrounding code that was changed. > > If I open a text editor on the file, I immediately see what you're saying: > >> ret = RESUME_GUEST; >> preempt_disable(); >> while (!try_lock_hpte(hptep, HPTE_V_HVLOCK)) >> cpu_relax(); >> if ((hptep[0] & ~HPTE_V_HVLOCK) != hpte[0] || hptep[1] != hpte[1] || >> rev->guest_rpte != hpte[2]) >> /* HPTE has been changed under us; let the guest retry */ >> goto out_unlock; >> hpte[0] = (hpte[0] & ~HPTE_V_ABSENT) | HPTE_V_VALID; >> >> rmap = &memslot->arch.rmap[gfn - memslot->base_gfn]; >> lock_rmap(rmap); >> >> /* Check if we might have been invalidated; let the guest retry if so */ >> ret = RESUME_GUEST; > > However, that scope is not given in the actual patch itself. If you look at the diff below, you have no idea whether the patch is fixing a bug or just removes duplication and doesn't actually have any effect. In fact, the compiled assembly should be the same with this patch and without. But you can't tell from the diff below. > > So what I would like to see in the patch description is something that makes it easy to understand what's going on without the need to check out the source file. Something like > >> We redundantly set ret to RESUME_GUEST twice without changing it in between. Only do it once: >> >> ret = RESUME_GUEST; >> preempt_disable(); >> while (!try_lock_hpte(hptep, HPTE_V_HVLOCK)) >> cpu_relax(); >> if ((hptep[0] & ~HPTE_V_HVLOCK) != hpte[0] || hptep[1] != hpte[1] || >> rev->guest_rpte != hpte[2]) >> /* HPTE has been changed under us; let the guest retry */ >> goto out_unlock; >> hpte[0] = (hpte[0] & ~HPTE_V_ABSENT) | HPTE_V_VALID; >> >> rmap = &memslot->arch.rmap[gfn - memslot->base_gfn]; >> lock_rmap(rmap); >> >> /* Check if we might have been invalidated; let the guest retry if so */ >> ret = RESUME_GUEST; > > > If I look at that patch description it immediately tells me "Ah, no need to worry, it's not a critical bug I need to backport". If you have a better idea how to express that I'm more than happy to take that too. Otherwise just let me know whether you like the description above and I'll modify it to the one that includes the code snippet when applying the patch. > Oh, yes. Thank you very much.. And I had a better understanding of the heavy work of maintainers :) Will keep this in mind. Best regards, Pingfan -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c index dbc1478..9b97b42 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c @@ -733,7 +733,6 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu, lock_rmap(rmap); /* Check if we might have been invalidated; let the guest retry if so */ - ret = RESUME_GUEST; if (mmu_notifier_retry(vcpu->kvm, mmu_seq)) { unlock_rmap(rmap); goto out_unlock;