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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.07.14 at 02:42, <bob.liu@xxxxxxxxxx> wrote:

> On 07/24/2014 02:24 PM, Jan Beulich wrote:
>>>>> 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
> 
> The page contents are a problem if the race condition I mentioned in
> previous email happen.
> 
> Because there is a time window between checking the PGC_need_scrub flag
> and doing the real scrub in idle thread, the idle thread will still
> scrub a page after that page have been allocated by allocation path and
> been used(and have been written some useful data).

Did you really read all of my previous reply?

>> 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
> 
> Right, but I don't have better ideas beside using page_list_for_each()
> or delist pages temporarily from heap list.
> 
>> 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).
> 
> It sounds like another list have to be introduced, but I don't think
> this can help to get rid of lock contention or the similar race condition.

Yeah, you need to get a little creative here...

Jan


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