On 07.11.2013, at 07:22, Liu Ping Fan <kernelf...@gmail.com> wrote:
> ret is assigned twice with the same value, so remove the later one.
>
> Signed-off-by: Liu Ping Fan <pingf...@linux.vnet.ibm.com>
> Acked-by: Paul Mackerras <pau...@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
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
