On 22.01.2020 17:51, Tamas K Lengyel wrote:
> On Wed, Jan 22, 2020 at 8:23 AM Jan Beulich <jbeul...@suse.com> wrote:
>>
>> On 21.01.2020 18:49, Tamas K Lengyel wrote:
>>> The owner domain of shared pages is dom_cow, use that for get_page
>>> otherwise the function fails to return the correct page.
>>
>> I think this description needs improvement: The function does the
>> special shared page dance in one place (on the "fast path")
>> already. This wants mentioning, either because it was a mistake
>> to have it just there, or because a new need has appeared to also
>> have it on the "slow path".
>
> It was a pre-existing error not to get the page from dom_cow for a
> shared entry in the slow path. I only ran into it now because the
> erroneous type_count check move in the previous version of the series
> was resulting in all pages being fully deduplicated instead of mostly
> being shared. Now that the pages are properly shared running LibVMI on
> the fork resulted in failures do to this bug.
>
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -594,7 +594,10 @@ struct page_info *p2m_get_page_from_gfn(
>>> if ( p2m_is_ram(*t) && mfn_valid(mfn) )
>>> {
>>> page = mfn_to_page(mfn);
>>> - if ( !get_page(page, p2m->domain) )
>>> + if ( !get_page(page, p2m->domain) &&
>>> + /* Page could be shared */
>>> + (!dom_cow || !p2m_is_shared(*t) ||
>>> + !get_page(page, dom_cow)) )
>>
>> While there may be a reason why on the fast path two get_page()
>> invocations are be necessary, couldn't you get away with just
>> one
>>
>> if ( !get_page(page, !dom_cow || !p2m_is_shared(*t) ? p2m->domain
>> : dom_cow) )
>>
>> at least here? It's also not really clear to me why here and
>> there we need "!dom_cow || !p2m_is_shared(*t)" - wouldn't
>> p2m_is_shared() return consistently "false" when !dom_cow ?
>
> I simply copied the existing code from the slow_path as-is. IMHO it
> would suffice to do a single get_page(page, !p2m_is_shared(*t) ?
> p2m->domain : dom_cow); since we can't have any entries that are
> shared when dom_cow is NULL so this is safe, no need for the extra
> !dom_cow check. If you prefer I can make the change for both
> locations.
If the change is correct to make also in the other place, I'd
definitely prefer you doing so.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
