Re: [Xen-devel] [PATCH v2 3/3] xen: use idle vcpus to scrub pages

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.07.14 at 04:08, <bob.liu@xxxxxxxxxx> wrote:

> On 07/23/2014 03:28 PM, Jan Beulich wrote:
>>>>> On 15.07.14 at 11:16, <bob.liu@xxxxxxxxxx> wrote:
>>> After so many days I haven't make a workable solution if don't remove
>>> pages temporarily. The hardest part is iterating the heap free list
>>> without holding heap_lock because if holding the lock it might be heavy
>>> lock contention.
>>> So do you think it's acceptable if fixed all other concerns about this
>>> patch?
>> 
>> No, I don't think so. Instead I'm of the opinion that you may have
>> worked in the wrong direction: Rather than not taking the heap lock
>> at all, it may also be sufficient to shrink the lock holding time (i.e.
>> avoid long loops with the lock held).
>> 
> 
> But I still think have to drop pages from heap list temporarily else
> heap lock must be taken for a long time to get rid of E.g. below race
> condition.
> 
> A: alloc path            B: idle loop
> 
>                         spin_lock(&heap_lock)
>                       page_list_for_each( pg, &heap(node, zone, order) )
>                         if _PGC_need_scrub is set, break;
>                         spin_unlock(&heap_lock)
> 
>                         if ( test_bit(_PGC_need_scrub, pg)
> 
> ^^^^
> spin_lock(&heap_lock)
> delist page
> spin_unlock(&heap_lock)
> 
> write data to this page
> 
>                          scrub_one_page(pg)
>                          ^^^ will clean useful data

No (and I'm sure I said so before): The only problem is with the
linked list itself; the page contents are not a problem - the
allocation path can simply wait for the already suggested
_PGC_scrubbing flag to clear before returning. And as already
said (see above), by avoiding page_list_for_each() within the
locked region you already significantly reduce lock contention.
I.e. you need another means to find pages awaiting to be
scrubbed. You may want to leverage that the allocation path
does the scrubbing if needed (e.g. by not scrubbing the first
of any set of contiguous free pages on the idle path, linking up
all other ones recognizing that their link fields are unused while
on an order-greater-than-zero free list).

Jan


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