Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210511171610.170160-1-bgardon@google.com> <20210511171610.170160-3-bgardon@google.com>
 <YJrFOXW3mM3WjGT5@google.com>
In-Reply-To: <YJrFOXW3mM3WjGT5@google.com>
From:   Ben Gardon <bgardon@google.com>
Date:   Tue, 11 May 2021 11:17:09 -0700
Message-ID: <CANgfPd9ekAidRzAWi-i=7h0pUpoHADSFJdAB5AWAzwm_Uk3dSA@mail.gmail.com>
Subject: Re: [PATCH v4 2/7] KVM: x86/mmu: Factor out allocating memslot rmap
To:     Sean Christopherson <seanjc@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, kvm <kvm@vger.kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Peter Xu <peterx@redhat.com>, Peter Shier <pshier@google.com>,
        Yulei Zhang <yulei.kernel@gmail.com>,
        Wanpeng Li <kernellwp@gmail.com>,
        Xiao Guangrong <xiaoguangrong.eric@gmail.com>,
        Kai Huang <kai.huang@intel.com>,
        Keqian Zhu <zhukeqian1@huawei.com>,
        David Hildenbrand <david@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 11, 2021 at 10:56 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, May 11, 2021, Ben Gardon wrote:
> > Small refactor to facilitate allocating rmaps for all memslots at once.
> >
> > No functional change expected.
> >
> > Signed-off-by: Ben Gardon <bgardon@google.com>
> > ---
> >  arch/x86/kvm/x86.c | 39 ++++++++++++++++++++++++++++++---------
> >  1 file changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 1e1f4f31e586..cc0440b5b35d 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10911,10 +10911,35 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
> >       kvm_page_track_free_memslot(slot);
> >  }
> >
> > +static int memslot_rmap_alloc(struct kvm_memory_slot *slot,
> > +                           unsigned long npages)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
> > +             int lpages;
> > +             int level = i + 1;
> > +
> > +             lpages = gfn_to_index(slot->base_gfn + npages - 1,
> > +                                   slot->base_gfn, level) + 1;
>
> Might as well assign lpages at its declaration, i.e.
>
>                 int lpages = gfn_to_index(slot->base_gfn + npages - 1,
>                                           slot->base_gfn, level) + 1;

I'll do this if I end up sending out a v5.

> > +
> > +             slot->arch.rmap[i] =
> > +                     kvcalloc(lpages, sizeof(*slot->arch.rmap[i]),
> > +                              GFP_KERNEL_ACCOUNT);
>
> Eh, I don't think avoiding a 3 char overrun is worth splitting across three lines.
> E.g. this is perfectly readable
>
>                 slot->arch.rmap[i] = kvcalloc(lpages, sizeof(*slot->arch.rmap[i]),
>                                               GFP_KERNEL_ACCOUNT);
>
> Alternatively, the rmap size could be captured in a local var, e.g.
>
>         const int sz = sizeof(*slot->arch.rmap[0]);
>
>         ...
>
>                 slot->arch.rmap[i] = kvcalloc(lpages, sz, GFP_KERNEL_ACCOUNT);

I like this suggestion. Much nicer. Will incorporate if I send a v5.

>                 if (!slot->arch.rmap[i]) {
>                         memslot_rmap_free(slot);
>                         return -ENOMEM;
>                 }
>
> > +             if (!slot->arch.rmap[i]) {
> > +                     memslot_rmap_free(slot);
> > +                     return -ENOMEM;
>
> Reaaaally getting into nitpicks, what do you think about changing this to a goto
> with the error handling at the bottom?  Obviously not necessary by any means,
> but for me it makes it easier to see that all rmaps are freed on failure.  My
> eyes skipped over that on the first read through.  E.g.
>
>                 if (!slot_arch.rmap[i])
>                         goto err;
>         }
>
>         return 0;
>
> err:
>         memslot_rmap_free(slot);
>         return -ENOMEM;
>

Lol, I had a goto in v3, but David Hildenbrand suggested removing it
and putting the free in the loop. I think I like it more this way too.

>
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static int kvm_alloc_memslot_metadata(struct kvm_memory_slot *slot,
> >                                     unsigned long npages)
> >  {
> >       int i;
> > +     int r;
>
> Personal preference, for short declarations like this I like putting 'em on a
> single line.
>
> >       /*
> >        * Clear out the previous array pointers for the KVM_MR_MOVE case.  The
> > @@ -10923,7 +10948,11 @@ static int kvm_alloc_memslot_metadata(struct kvm_memory_slot *slot,
> >        */
> >       memset(&slot->arch, 0, sizeof(slot->arch));
> >
> > -     for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
> > +     r = memslot_rmap_alloc(slot, npages);
> > +     if (r)
> > +             return r;
> > +
> > +     for (i = 1; i < KVM_NR_PAGE_SIZES; ++i) {
> >               struct kvm_lpage_info *linfo;
> >               unsigned long ugfn;
> >               int lpages;
> > @@ -10932,14 +10961,6 @@ static int kvm_alloc_memslot_metadata(struct kvm_memory_slot *slot,
> >               lpages = gfn_to_index(slot->base_gfn + npages - 1,
> >                                     slot->base_gfn, level) + 1;
> >
> > -             slot->arch.rmap[i] =
> > -                     kvcalloc(lpages, sizeof(*slot->arch.rmap[i]),
> > -                              GFP_KERNEL_ACCOUNT);
> > -             if (!slot->arch.rmap[i])
> > -                     goto out_free;
> > -             if (i == 0)
> > -                     continue;
> > -
> >               linfo = kvcalloc(lpages, sizeof(*linfo), GFP_KERNEL_ACCOUNT);
> >               if (!linfo)
> >                       goto out_free;
> > --
> > 2.31.1.607.g51e8a6a459-goog
> >