[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function

>>> On 11.12.18 at 16:19, <roger.pau@xxxxxxxxxx> wrote:
> On Tue, Dec 11, 2018 at 08:08:51AM -0700, Jan Beulich wrote:
>> >>> On 05.12.18 at 15:54, <roger.pau@xxxxxxxxxx> wrote:
>> > To note it's calculating the approximate amount of memory required by
>> > shadow paging.
>> I don't understand this logic, and ...
>> > @@ -325,7 +325,7 @@ unsigned long __init dom0_compute_nr_pages(
>> >              break;
>> >  
>> >          /* Reserve memory for shadow or HAP. */
>> > -        avail -= dom0_paging_pages(d, nr_pages);
>> > +        avail -= dom0_shadow_pages(d, nr_pages);
>> >      }
>> ... the comment here (and lack of conditional restricting the
>> code to shadow mode) appear to support me: Have you
>> been mislead by the function having a comment referring
>> to libxl_get_required_shadow_memory()? I think if anything
>> that libxl function would want to be renamed (to replace
>> "shadow" by something more generic in its name).
> But the logic in dom0_shadow_pages to calculate the size of the paging
> memory pool is specifically for shadow AFAICT, I don't think HAP needs
> to take the number of vCPUs into account, since there's only a
> single p2m for the whole domain. OTOH shadow needs to take the number
> of vCPUs into account because each one will have a different shadow.

Yes, the vCPU count aspect is indeed shadow specific. However,
as said in reply to the other patch, the calculation here was at
least supposed to also take into account the P2M part of the
needed allocations. Yet the P2M part ought to be similar between
both modes.

> Note that patch 2 in this series adds a function to calculate the size
> of the paging memory pool for HAP, and a conditional is added to the
> expression above that takes into account whether shadow or HAP is in
> use when subtracting from the amount of available memory.

Well, assuming we can settle on what shape patch 2 should take
I can see the point in doing the rename here, but then with an
adjusted description: Especially in light of the code comment still
visible above you'll want to point out that the rename is in
preparation of splitting the calculations. Since I question the split,
though, the rename (in a separate patch) is questionable to me
too. If we used uniform P2M calculations and added just shadow's
per-vCPU extra on top, no rename in a separate patch would
seem warranted.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.