Re: [Xen-devel] [PATCH v6 4/5] mm: add 'is_special_page' inline function...

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.03.2020 15:47, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 17 March 2020 13:07
>>
>> On 10.03.2020 18:49, paul@xxxxxxx wrote:
>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>> @@ -749,8 +749,9 @@ p2m_pod_zero_check_superpage(struct p2m_domain *p2m, 
>>> gfn_t gfn)
>>>
>>>          n = 1UL << min(cur_order, SUPERPAGE_ORDER + 0U);
>>>          for ( k = 0, page = mfn_to_page(mfn); k < n; ++k, ++page )
>>> -            if ( !(page->count_info & PGC_allocated) ||
>>> -                 (page->count_info & (PGC_page_table | PGC_xen_heap)) ||
>>> +            if ( is_special_page(page) ||
>>> +                 !(page->count_info & PGC_allocated) ||
>>> +                 (page->count_info & PGC_page_table) ||
>>>                   (page->count_info & PGC_count_mask) > max_ref )
>>>                  goto out;
>>>      }
>>> @@ -886,8 +887,9 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t 
>>> *gfns, unsigned int count
>>>           * If this is ram, and not a pagetable or from the xen heap, and
>>>           * probably not mapped elsewhere, map it; otherwise, skip.
>>>           */
>>> -        if ( p2m_is_ram(types[i]) && (pg->count_info & PGC_allocated) &&
>>> -             !(pg->count_info & (PGC_page_table | PGC_xen_heap)) &&
>>> +        if ( p2m_is_ram(types[i]) && !is_special_page(pg) &&
>>> +             (pg->count_info & PGC_allocated) &&
>>> +             !(pg->count_info & PGC_page_table) &&
>>>               ((pg->count_info & PGC_count_mask) <= max_ref) )
>>>              map[i] = map_domain_page(mfns[i]);
>>>          else
>>
>> I appreciate your desire to use the inline function you add, and
>> I also appreciate that you likely prefer to not make the other
>> suggested change (at least not right here), but then I think the
>> commit message would better gain a remark regarding the
>> suspicious uses of PGC_page_table here.
> 
> What's suspicious about it?

Hmm, looks like I was wrongly remembering shadow code to be setting
this on the shadows of guest page tables, rather than on the guest
page tables themselves. I'm sorry for the noise. (Keeping the bit-
is-set tests together may still be a good idea though code generation
wise, unless you know that all halfway recent compiler versions can
fold the code fine in its current shape.)

> I now realise the above comment also needs fixing.

Oh, indeed.

>> I continue to think that
>> they should be dropped as being pointless. In any event I fear
>> the resulting code will be less efficient, as I'm unconvinced
>> that the compiler will fold the now split bit checks.
> 
> I could go back to defining is_special_page() as a macro.

I don't think this would make a difference.

Jan

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