On Thu, Nov 7, 2013 at 6:06 PM, Alexander Graf <ag...@suse.de> wrote: > > 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. > 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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
