Re: [PATCH v4 20/20] KVM: x86/mmu: Extend Eager Page Splitting to nested MMUs

[Sean Christopherson <seanjc@google.com>]

On Mon, May 09, 2022, David Matlack wrote:
> On Mon, May 9, 2022 at 9:48 AM Sean Christopherson <seanjc@google.com> wrote:
> > > +static void kvm_nested_mmu_try_split_huge_pages(struct kvm *kvm,
> > > +                                             const struct kvm_memory_slot *slot,
> > > +                                             gfn_t start, gfn_t end,
> > > +                                             int target_level)
> > > +{
> > > +     int level;
> > > +
> > > +     /*
> > > +      * Split huge pages starting with KVM_MAX_HUGEPAGE_LEVEL and working
> > > +      * down to the target level. This ensures pages are recursively split
> > > +      * all the way to the target level. There's no need to split pages
> > > +      * already at the target level.
> > > +      */
> > > +     for (level = KVM_MAX_HUGEPAGE_LEVEL; level > target_level; level--) {
> >
> > Unnecessary braces.
> 
> The brace is unnecessary, but when the inner statement is split across
> multiple lines I tend to prefer using braces. (That's why I did the
> same in the other patch and you had the same feedback.) I couldn't
> find any guidance about this in CodingStyle so I'm fine with getting
> rid of the braces if that's what you prefer.

The style varies by subsystem, e.g. I believe perf requires braces in this case.
Absent a "hard" rule, I value consistency above all else, e.g. because KVM doesn't
(usually) include the braces, I started looking for the second statement, i.e. the
lack of an opening brace is an indicator (to me at elast) that a loop/if contains
a single statement.

I actually like Golang's forced braces, but mostly because they are 100% mandatory
and so all code is consistent.
 
> > > +             slot_handle_level_range(kvm, slot,
> > > +                                     nested_mmu_try_split_huge_pages,
> > > +                                     level, level, start, end - 1,
> > > +                                     true, false);
> >
> > IMO it's worth running over by 4 chars to drop 2 lines:
> 
> Will do.
> 
> >
> >         for (level = KVM_MAX_HUGEPAGE_LEVEL; level > target_level; level--)
> >                 slot_handle_level_range(kvm, slot, nested_mmu_try_split_huge_pages,
> >                                         level, level, start, end - 1, true, false);
> > > +     }
> > > +}
> > > +
> > >  /* Must be called with the mmu_lock held in write-mode. */
> >
> > Add a lockdep assertion, not a comment.
> 
> Agreed but this is an existing comment, so better left to a separate patch.

Doh, I mistook the /* for a +.