Re: [Xen-devel] [PATCH 4/4] x86/PoD: shorten certain operations on higher order ranges

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.09.15 at 19:10, <george.dunlap@xxxxxxxxxx> wrote:
> On 09/15/2015 08:37 AM, Jan Beulich wrote:
>> @@ -574,41 +576,46 @@ recount:
>>       * + There are PoD entries to handle, or
>>       * + There is ram left, and we want to steal it
>>       */
>> -    for ( i=0;
>> -          i<(1<<order) && (pod>0 || (steal_for_cache && ram > 0));
>> -          i++)
>> +    for ( i = 0;
>> +          i < (1UL << order) && (pod > 0 || (steal_for_cache && ram > 0));
>> +          i += n )
>>      {
>>          mfn_t mfn;
>>          p2m_type_t t;
>>          p2m_access_t a;
>> +        unsigned int cur_order;
>>  
>> -        mfn = p2m->get_entry(p2m, gpfn + i, &t, &a, 0, NULL, NULL);
>> +        mfn = p2m->get_entry(p2m, gpfn + i, &t, &a, 0, &cur_order, NULL);
>> +        if ( order < cur_order )
>> +            cur_order = order;
>> +        n = 1UL << cur_order;
>>          if ( t == p2m_populate_on_demand )
>>          {
>> -            p2m_set_entry(p2m, gpfn + i, _mfn(INVALID_MFN), 0, p2m_invalid,
>> -                          p2m->default_access);
>> -            p2m->pod.entry_count--;
>> +            p2m_set_entry(p2m, gpfn + i, _mfn(INVALID_MFN), cur_order,
>> +                          p2m_invalid, p2m->default_access);
>> +            p2m->pod.entry_count -= n;
>>              BUG_ON(p2m->pod.entry_count < 0);
>> -            pod--;
>> +            pod -= n;
>>          }
>>          else if ( steal_for_cache && p2m_is_ram(t) )
>>          {
>>              struct page_info *page;
>> +            unsigned int j;
>>  
>>              ASSERT(mfn_valid(mfn));
>>  
>>              page = mfn_to_page(mfn);
>>  
>> -            p2m_set_entry(p2m, gpfn + i, _mfn(INVALID_MFN), 0, p2m_invalid,
>> -                          p2m->default_access);
>> -            set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY);
>> -
>> -            p2m_pod_cache_add(p2m, page, 0);
>> +            p2m_set_entry(p2m, gpfn + i, _mfn(INVALID_MFN), cur_order,
>> +                          p2m_invalid, p2m->default_access);
>> +            for ( j = 0; j < n; ++j )
>> +                set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY);
>> +            p2m_pod_cache_add(p2m, page, cur_order);
>>  
>>              steal_for_cache =  ( p2m->pod.entry_count > p2m->pod.count );
> 
> This code will now steal an entire 2MiB page even if we only need a few
> individual pages, then free it below (calling p2m_pod_set_cache_target()).
> 
> Upon reflection, this is actually a feature -- if we have singleton
> pages in the cache, we can free those first, hopefully keeping the 2MiB
> page together.
> 
> It would be good to have a comment here to that effect, though; perhaps:
> 
> "If we need less than 1<<order, may end up stealing more memory here
> than we actually need.  This will be rectified below, however; and
> stealing too much and then freeing what we need may allow us to free
> smaller pages from the cache, and avoid breaking up superpages."

Good point - I didn't really notice this change in behavior. Comment
added.

> It occurs to me that the steal_for_cache calculations are also wrong
> here --it should be (p2m->pod.entry_count - pod > p2m->pod.count) --
> i.e., we should steal if liabilities would be greater than assets
> *after* this function completed, not before.

In another patch (by you?) perhaps?

> Otherwise, everything else looks good, thanks.
> 
> I assume you'll resubmit this when the patch it depends on leaves RFC?
> In which case I'll wait until I see that version to Ack it.

The dependencies were patches 1 and 2 in this series, which
already went in. Patch 3 (the RFC one) was independent; in the
one here we just leverage what was needed as a prereq for that
other one.

Jan


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