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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.07.14 at 14:25, <bob.liu@xxxxxxxxxx> wrote:
> On 07/01/2014 05:12 PM, Jan Beulich wrote:
>>>>> On 30.06.14 at 15:39, <lliubbo@xxxxxxxxx> wrote:
>>> @@ -948,6 +954,7 @@ static void free_heap_pages(
>>>      {
>>>          if ( !tainted )
>>>          {
>>> +            node_need_scrub[node] = 1;
>>>              for ( i = 0; i < (1 << order); i++ )
>>>                  pg[i].count_info |= PGC_need_scrub;
>>>          }
>> 
>> Iirc it was more than this single place where you set
>> PGC_need_scrub, and hence where you'd now need to set the
>> other flag too.
>> 
> 
> I'm afraid this is the only place where PGC_need_scrub was set.

Ah, indeed - I misremembered others, they are all tests for the flag.

> I'm sorry for all of the coding style problems.
> 
> By the way is there any script which can be used to check the code
> before submitting? Something like ./scripts/checkpatch.pl under linux.

No, there isn't. But avoiding (or spotting) hard tabs should be easy
enough, and other things you ought to simply inspect your patch for
- after all that's no different from what reviewers do.

>>> +    }
>>> +
>>> +    /* free percpu free list */
>>> +    if ( !page_list_empty(local_free_list) )
>>> +    {
>>> +        spin_lock(&heap_lock);
>>> +        page_list_for_each_safe( pg, tmp, local_free_list )
>>> +        {
>>> +            order = PFN_ORDER(pg);
>>> +            page_list_del(pg, local_free_list);
>>> +            for ( i = 0; i < (1 << order); i++ )
>>> +       {
>>> +                pg[i].count_info |= PGC_state_free;
>>> +                pg[i].count_info &= ~PGC_need_scrub;
>> 
>> This needs to happen earlier - the scrub flag should be cleared right
>> after scrubbing, and the free flag should imo be set when the page
>> gets freed. That's for two reasons:
>> 1) Hypervisor allocations don't need scrubbed pages, i.e. they can
>> allocate memory regardless of the scrub flag's state.
> 
> AFAIR, the reason I set those flags here is to avoid a panic happen.

That's pretty vague a statement.

>> 2) You still detain the memory on the local lists from allocation. On a
>> many-node system, the 16Mb per node can certainly sum up (which
>> is not to say that I don't view the 16Mb on a single node as already
>> problematic).
> 
> Right, but we can adjust SCRUB_BATCH_ORDER.
> Anyway I'll take a retry as you suggested.

You should really drop the idea of removing pages temporarily.
All you need to do is make sure a page being allocated and getting
simultaneously scrubbed by another CPU won't get passed to the
caller until the scrubbing finished. In particular it's no problem if
the allocating CPU occasionally ends up scrubbing a page already
being scrubbed elsewhere.

>>> +                            }
>>> +                            PFN_ORDER(pg) = SCRUB_BATCH_ORDER;
>>> +                        }
>>> +
>>> +                        for ( i = 0; i < (1 << PFN_ORDER(pg)); i++ )
>> 
>> Can't you just use "order" here (and a few lines down)?
>> 
> 
> Then I have to use another temporary variable. Anyway, I'll make a update.

Why? You don't modify the value? Oh, did you misread this as a
request to replace "i"? That wasn't my point, I was asking to replace
PFN_ORDER(pg).

>>> +                        {
>>> +                            ASSERT( test_bit(_PGC_need_scrub, 
>>> &pg[i].count_info) );
>>> +                            ASSERT( !test_bit(_PGC_broken, 
>>> &pg[i].count_info) );
>>> +                            mark_page_offline(&pg[i], 0);
>> 
>> mark_page_offline() ???
>>
> 
> Yes, it's a bit tricky here and I don't have a good idea right now.
> The problem is when free a page frame we have to avoid merging with
> pages on percpu scrub/free list, so I marked pages offline temporarily
> while adding to percpu lists.

Which will be solved implicitly by no longer removing pages from
what the allocator can see.

> Another thing I'm still not clear about is how to handle the situation
> if #mc happened for pages on percpu scrub/free list.

Dito.

Jan

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