Re: [Xen-devel] [PATCH v4 1/8] mm: Place unscrubbed pages at the end of pagelist

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 06/09/2017 10:50 AM, Jan Beulich wrote:
>>>> On 19.05.17 at 17:50, <boris.ostrovsky@xxxxxxxxxx> wrote:
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -383,6 +383,8 @@ typedef struct page_list_head 
>> heap_by_zone_and_order_t[NR_ZONES][MAX_ORDER+1];
>>  static heap_by_zone_and_order_t *_heap[MAX_NUMNODES];
>>  #define heap(node, zone, order) ((*_heap[node])[zone][order])
>>  
>> +static unsigned long node_need_scrub[MAX_NUMNODES];
> Just as a remark - I think it is inefficient to store per-node data
> this way (equally applies to _heap[]); this would better be put
> into per-node memory. Since we don't have (and likely also don't
> want to have) this_node() / per_node(), perhaps we could
> (ab)use per-CPU data for this.

I did think about doing this but then decided against it since I wasn't
sure it's worth the extra space.


>
>> @@ -798,11 +814,18 @@ static struct page_info *alloc_heap_pages(
>>      return NULL;
>>  
>>   found: 
>> +    need_scrub = (pg->u.free.first_dirty != INVALID_DIRTY_IDX);
>> +
>>      /* We may have to halve the chunk a number of times. */
>>      while ( j != order )
>>      {
>> -        PFN_ORDER(pg) = --j;
>> -        page_list_add_tail(pg, &heap(node, zone, j));
>> +        /*
>> +         * Some of the sub-chunks may be clean but we will mark them
>> +         * as dirty (if need_scrub is set) to avoid traversing the
>> +         * list here.
>> +         */
>> +        page_list_add_scrub(pg, node, zone, --j,
>> +                            need_scrub ? 0 : INVALID_DIRTY_IDX);
> I suppose this is going to be improved in subsequent patches,
> and hence the comment will be gone by the end of the series?

No, it stays the same throughout he series. I suppose I could improve
this by tracking the original buddy's first_dirty and call
page_list_add_scrub(.., false) until we reach that value.


>
>> @@ -919,9 +965,52 @@ static int reserve_offlined_page(struct page_info *head)
>>      return count;
>>  }
>>  
>> +static void scrub_free_pages(unsigned int node)
>> +{
>> +    struct page_info *pg;
>> +    unsigned int zone;
>> +
>> +    ASSERT(spin_is_locked(&heap_lock));
>> +
>> +    if ( !node_need_scrub[node] )
>> +        return;
>> +
>> +    for ( zone = 0; zone < NR_ZONES; zone++ )
>> +    {
>> +        unsigned int order = MAX_ORDER;
>> +        do {
> Blank line between declaration(s) and statement(s) please.
>
> Also, considering that this entire function runs with the heap lock
> held, I think we really need to split that one into per-node ones.
> But of course, as with the NUMA locality related remark, this isn't
> a request to necessarily do this in the series here.

Keep in mind that last patch drops the lock when doing actual scrubbing
so it will get better.

>> +            unsigned int first_dirty;
> On x86 this change is fine at present, albeit not optimal. Its ARM
> equivalent, however, grows struct page_info in the 32-bit case,

It does? I am looking at include/asm-arm/mm.h and I don't see this.

> which I don't think is wanted or needed. You really only need
> MAX_ORDER+1 bits here, so I'd suggest making this a bit field

Just to make sure --- when you say "bit field" you mean masking various
bits in a word, not C language bit fields?

> (also on x86, to at once make obvious how many bits remain
> unused), and perhaps making need_tlbflush a single bit at once
> (unless its address is being taken somewhere).


accumulate_tlbflush() flush wants the address but I think it can be
modified to handle a single bit.


-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel