Message-ID: <20191105145651.GD30717@redhat.com>
Date:   Tue, 5 Nov 2019 09:56:51 -0500
From:   Andrea Arcangeli <aarcange@...hat.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Jessica Yu <jeyu@...nel.org>,
        Matthias Maennich <maennich@...gle.com>
Subject: Re: [PATCH 03/13] kvm: monolithic: fixup x86-32 build

On Tue, Nov 05, 2019 at 03:09:37PM +0100, Paolo Bonzini wrote:
> You can reorder patches so that kvm_x86_ops assignments never happen.
> That way, 4/13 for example would be moved to the very beginning.

Ok 3/13 and 4/14 can both go before 2/13, I reordered them fine, the
end result at least is the same and the intermediate result should
improve. Hopefully this is the best solution for those two outliers.

Once this is committed I expect Sean to take over 4/14 with a more
optimal version to get rid of that branch like he proposed initially.

> >> - look into how to remove the modpost warnings.  A simple (though
> >> somewhat ugly) way is to keep a kvm.ko module that includes common
> >> virt/kvm/ code as well as, for x86 only, page_track.o.  A few functions,
> >> such as kvm_mmu_gfn_disallow_lpage and kvm_mmu_gfn_allow_lpage, would
> >> have to be moved into mmu.h, but that's not a big deal.
> > 
> > I think we should:
> > 
> > 1) whitelist to shut off the warnings on demand
> 
> Do you mean adding a whitelist to modpost?  That would work, though I am
> not sure if the module maintainer (Jessica Yu) would accept that.

Yes that's exactly what I meant.

> > 2) verify that if two modules are registering the same export symbol
> >    the second one fails to load and the module code is robust about
> >    that, this hopefully should already be the case
> > 
> > Provided verification of 2), the whitelist is more efficient than
> > losing 4k of ram in all KVM hypervisors out there.
> 
> I agree.

Ok, so I enlarged the CC list accordingly to check how the whitelist
can be done in modpost.

> >> - provide at least some examples of replacing the NULL kvm_x86_ops
> >> checks with error codes in the function (or just early "return"s).  I
> >> can help with the others, but remember that for the patch to be merged,
> >> kvm_x86_ops must be removed completely.
> > 
> > Even if kvm_x86_ops wouldn't be guaranteed to go away, this would
> > already provide all the performance benefit to the KVM users, so I
> > wouldn't see a reason not to apply it even if kvm_x86_ops cannot go
> > away.
> 
> The answer is maintainability.  My suggestion is that we start looking
> into removing all assignments and tests of kvm_x86_ops, one step at a
> time.  Until this is done, unfortunately we won't be able to reap the
> performance benefit.  But the advantage is that this can be done in many

There's not much performance benefit left from the removal
kvm_x86_ops. It'll only remove a few branches at best (and only if we
don't have to replace the branches on the pointer check with other
branches on a static variable to disambiguate the different cases).

> separate submissions; it doesn't have to be one huge patch.
> 
> Once this is done, removing kvm_x86_ops is trivial in the end.  It's
> okay if the intermediate step has minimal performance regressions, we
> know what it will look like.  I have to order patches with maintenance
> first and performance second, if possible.

The removal of kvm_x86_ops is just a badly needed code cleanup and of
course I agree it must happen sooner than later. I'm just trying to
avoid running into rejects on those further commit cleanups too.

> By the way, we are already planning to make some module parameters
> per-VM instead of global, so this refactoring would also help that effort.
>
> > Said that it will go away and there's no concern about it. It's
> > just that the patchset seems large enough already and it rejects
> > heavily already at every port. I simply stopped at the first self
> > contained step that provides all performance benefits.
> 
> That is good enough to prove the feasibility of the idea, so I agree
> that was a good plan.

All right, so I'm not exactly sure what's the plan and if it's ok to
do it over time or if I should go ahead doing all logic changes while
the big patch remains out of tree.

If you apply it and reorder 4/13 and 3/13 before 2/13 in a rebase like
I did locally, it should already be good starting point in my view and
the modpost also can be fixed over time too, the warnings appears
harmless so far.

Thanks,
Andrea

Powered by blists - more mailing lists