Re: [Xen-devel] [PATCH 2/4] x86: relax a few get_gfn() invocations

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.04.19 at 12:30, <george.dunlap@xxxxxxxxxx> wrote:
> On 3/13/19 12:38 PM, Jan Beulich wrote:
>> In a few cases only a query is intended, i.e. without populating a
>> possible PoD or paged out entry, when the intention is to replace the
>> current entry anyway. Use get_gfn_query() there instead.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> The first one should be fine, but pretty sure the second two will
> mis-fire for PoD: p2m->pod.entry_count won't get updated.  As it is,
> it's sort-of misfiring already, since the "freed" memory won't be
> reclaimed for the PoD cache.

Well, in the 2nd instance (xenmem_add_to_physmap_one()) we
call guest_physmap_add_page(), which is a thin wrapper around
guest_physmap_add_entry(). That function already does _some_
updating of PoD statistics, but quite not enough, I agree.

Thanks for noticing these issues, I withdraw the latter two parts,
and I'll commit just the first hunk (with a slightly adjusted
description).

> Which probably means it's time for some serious refactoring to prevent
> this sort of error from creeping in.  p2m->pod.entry_count accounting
> should probably be moved into p2m_entry_modify(), and

Right.

> p2m_pod_decrease_reservation() should probably be moved somewhere else
> -- perhaps guest_remove_page() or something?

This one I'm less sure about - it's meant to avoid having to go
through guest_remove_page() for every one of the pages
after all. Since it layers on top of ->set_entry(), perhaps its
call site could be left alone, but its statistics adjustments may
need dropping (or modifying) when the former change gets
done.

Otoh when things are properly layered, perhaps it's better not
to have any such "bypasses", and the function would then best
go away altogether?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel