Re: [Xen-devel] [PATCH v6 2/5] mm: keep PGC_extra pages on a separate list

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.03.2020 19:11, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 16 March 2020 16:53
>>
>> On 10.03.2020 18:49, paul@xxxxxxx wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2314,7 +2314,7 @@ int assign_pages(
>>>          smp_wmb(); /* Domain pointer must be visible before updating 
>>> refcnt. */
>>>          pg[i].count_info =
>>>              (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
>>> -        page_list_add_tail(&pg[i], &d->page_list);
>>> +        page_list_add_tail(&pg[i], page_to_list(d, &pg[i]));
>>>      }
>>
>> This moves the one extra page we currently have (VMX'es APIC access
>> page) to a different list. Without adjustment to domain cleanup,
>> how is this page now going to get freed? (This of course then should
>> be extended to Arm, even if right now there's no "extra" page there.)
>>
> 
> I don't think there is any need to adjust as the current code in will
> drop the allocation ref in vmx_free_vlapic_mapping(), so it doesn't
> matter that it is missed by relinquish_memory().

Hmm, yes. It feels like thin ice, but I think you're right. Nevertheless
the freeing of extra pages should imo ultimately move to the central
place, i.e. it would seem more clean to me if the put_page_alloc_ref()
call was dropped from vmx_free_vlapic_mapping().

>>> --- a/xen/include/asm-x86/mm.h
>>> +++ b/xen/include/asm-x86/mm.h
>>> @@ -629,10 +629,8 @@ typedef struct mm_rwlock {
>>>      const char        *locker_function; /* func that took it */
>>>  } mm_rwlock_t;
>>>
>>> -#define arch_free_heap_page(d, pg)                                      \
>>> -    page_list_del2(pg, is_xen_heap_page(pg) ?                           \
>>> -                       &(d)->xenpage_list : &(d)->page_list,            \
>>> -                   &(d)->arch.relmem_list)
>>> +#define arch_free_heap_page(d, pg) \
>>> +    page_list_del2(pg, page_to_list((d), (pg)), &(d)->arch.relmem_list)
>>
>> Arguments passed on as is (i.e. not as part of an expression) don't
>> need parentheses.
>>
> 
> Are you saying it should be:
> 
> #define arch_free_heap_page(d, pg) \
>     page_list_del2(pg, page_to_list(d, pg), &(d)->arch.relmem_list)

Yes. But if this and the other cosmetic changes are the only changes to
make, I'd be fine to do so while committing (if no other reason for a
v7 arises):
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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