| Message ID | 20151112205019.611e8216adebb465b689876e@lab.ntt.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/12/2015 07:50 PM, Takuya Yoshikawa wrote: > + if (!ret) { > + clear_unsync_child_bit(sp, i); > + continue; > + } else if (ret > 0) { > nr_unsync_leaf += ret; Just a single line here, braces are unnecessary. > - else > + } else > return ret; > } else if (child->unsync) { > nr_unsync_leaf++; > if (mmu_pages_add(pvec, child, i)) > return -ENOSPC; > } else > - goto clear_child_bitmap; > - > - continue; > - > -clear_child_bitmap: > - __clear_bit(i, sp->unsync_child_bitmap); > - sp->unsync_children--; > - WARN_ON((int)sp->unsync_children < 0); > + clear_unsync_child_bit(sp, i); > } -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/11/18 11:44, Xiao Guangrong wrote: > On 11/12/2015 07:50 PM, Takuya Yoshikawa wrote: >> + if (!ret) { >> + clear_unsync_child_bit(sp, i); >> + continue; >> + } else if (ret > 0) { >> nr_unsync_leaf += ret; > > Just a single line here, braces are unnecessary. > >> - else >> + } else >> return ret; I know we can eliminate the braces, but that does not mean we should do so: there seems to be no consensus about this style issue and checkpatch accepts both ways. Actually, some people prefer to put braces when one of the if/else-if/else cases has multiple lines. You can see some examples in kernel/sched/core.c: see hrtick_start(), sched_fork(), free_sched_domain(). In our case, I thought putting braces would align the else-if and else and make the code look a bit nicer, but I know this may be just a matter of personal feeling. In short, unless the maintainer, Paolo for this file, has any preference, both ways will be accepted. Takuya -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/19/2015 08:59 AM, Takuya Yoshikawa wrote: > On 2015/11/18 11:44, Xiao Guangrong wrote: > >> On 11/12/2015 07:50 PM, Takuya Yoshikawa wrote: >>> + if (!ret) { >>> + clear_unsync_child_bit(sp, i); >>> + continue; >>> + } else if (ret > 0) { >>> nr_unsync_leaf += ret; >> >> Just a single line here, braces are unnecessary. >> >>> - else >>> + } else >>> return ret; > > I know we can eliminate the braces, but that does not mean > we should do so: there seems to be no consensus about this > style issue and checkpatch accepts both ways. > > Actually, some people prefer to put braces when one of the > if/else-if/else cases has multiple lines. You can see > some examples in kernel/sched/core.c: see hrtick_start(), > sched_fork(), free_sched_domain(). > > In our case, I thought putting braces would align the else-if > and else and make the code look a bit nicer, but I know this > may be just a matter of personal feeling. > > In short, unless the maintainer, Paolo for this file, has any > preference, both ways will be accepted. The reason why i pointed this out is that it is the style documented in Documentation/CodingStyle: | Do not unnecessarily use braces where a single statement will do. | | if (condition) | action(); | Actually, Ingo Molnar hated this braces-style too much and blamed many developers who used this style (include me, that why i was nervous to see this style :( ). If this style is commonly accepted now, it is worth making a patch to update Documentation/CodingStyle. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/11/19 11:46, Xiao Guangrong wrote: >> Actually, some people prefer to put braces when one of the >> if/else-if/else cases has multiple lines. You can see >> some examples in kernel/sched/core.c: see hrtick_start(), >> sched_fork(), free_sched_domain(). >> >> In our case, I thought putting braces would align the else-if >> and else and make the code look a bit nicer, but I know this >> may be just a matter of personal feeling. >> >> In short, unless the maintainer, Paolo for this file, has any >> preference, both ways will be accepted. > > The reason why i pointed this out is that it is the style documented > in Documentation/CodingStyle: > | Do not unnecessarily use braces where a single statement will do. > | > | if (condition) > | action(); > | Ah, this is a different thing. For this case, there is a consensus and checkpatch will complain if we don't obey the rule. What I explained was: if (condition) { line1; line2; // multiple lines } else if { single-line-statement; -- (*1) } else single-line-statement; -- (*2) For (*1) and (*2), especially for (*1), some people put braces. > Actually, Ingo Molnar hated this braces-style too much and blamed > many developers who used this style (include me, that why i was > nervous to see this style :( ). I think he likes the coding style of kernel/sched/core.c very much, as you know. Actually that is one reason why I took it as an example. Let's just choose the way which Paolo prefers for this time, I don't know which is better. Thank you, Takuya -- To unsubscribe from this list: send the line "unsubscribe kvm" 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/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index c3bbc82..f3120aa 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -1806,6 +1806,13 @@ static int mmu_pages_add(struct kvm_mmu_pages *pvec, struct kvm_mmu_page *sp, return (pvec->nr == KVM_PAGE_ARRAY_NR); } +static inline void clear_unsync_child_bit(struct kvm_mmu_page *sp, int idx) +{ + --sp->unsync_children; + WARN_ON((int)sp->unsync_children < 0); + __clear_bit(idx, sp->unsync_child_bitmap); +} + static int __mmu_unsync_walk(struct kvm_mmu_page *sp, struct kvm_mmu_pages *pvec) { @@ -1815,8 +1822,10 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp, struct kvm_mmu_page *child; u64 ent = sp->spt[i]; - if (!is_shadow_present_pte(ent) || is_large_pte(ent)) - goto clear_child_bitmap; + if (!is_shadow_present_pte(ent) || is_large_pte(ent)) { + clear_unsync_child_bit(sp, i); + continue; + } child = page_header(ent & PT64_BASE_ADDR_MASK); @@ -1825,28 +1834,21 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp, return -ENOSPC; ret = __mmu_unsync_walk(child, pvec); - if (!ret) - goto clear_child_bitmap; - else if (ret > 0) + if (!ret) { + clear_unsync_child_bit(sp, i); + continue; + } else if (ret > 0) { nr_unsync_leaf += ret; - else + } else return ret; } else if (child->unsync) { nr_unsync_leaf++; if (mmu_pages_add(pvec, child, i)) return -ENOSPC; } else - goto clear_child_bitmap; - - continue; - -clear_child_bitmap: - __clear_bit(i, sp->unsync_child_bitmap); - sp->unsync_children--; - WARN_ON((int)sp->unsync_children < 0); + clear_unsync_child_bit(sp, i); } - return nr_unsync_leaf; } @@ -2009,9 +2011,7 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents) if (!sp) return; - --sp->unsync_children; - WARN_ON((int)sp->unsync_children < 0); - __clear_bit(idx, sp->unsync_child_bitmap); + clear_unsync_child_bit(sp, idx); level++; } while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children); }
Both __mmu_unsync_walk() and mmu_pages_clear_parents() have three line code which clears a bit in the unsync child bitmap; the former places it inside a loop block and uses a few goto statements to jump to it. A new helper function, clear_unsync_child_bit(), makes the code cleaner. Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> --- arch/x86/kvm/mmu.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)