OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [PATCH v11 0/6] mm / virtio: Provide support for unused page reporting

On 10/7/19 1:20 PM, Alexander Duyck wrote:
> On Mon, Oct 7, 2019 at 10:07 AM Nitesh Narayan Lal <nitesh@redhat.com> wrote:
>> On 10/7/19 12:27 PM, Alexander Duyck wrote:
>>> On Mon, 2019-10-07 at 12:19 -0400, Nitesh Narayan Lal wrote:
>>>> On 10/7/19 11:33 AM, Alexander Duyck wrote:
>>>>> On Mon, 2019-10-07 at 08:29 -0400, Nitesh Narayan Lal wrote:
>>>>>> On 10/2/19 10:25 AM, Alexander Duyck wrote:
> <snip>
>>>>>> page_reporting.c change:
>>>>>> @@ -101,8 +101,12 @@ static void scan_zone_bitmap(struct page_reporting_config
>>>>>> *phconf,
>>>>>>                 /* Process only if the page is still online */
>>>>>>                 page = pfn_to_online_page((setbit << PAGE_REPORTING_MIN_ORDER) +
>>>>>>                                           zone->base_pfn);
>>>>>> -               if (!page)
>>>>>> +               if (!page || !PageBuddy(page)) {
>>>>>> +                       clear_bit(setbit, zone->bitmap);
>>>>>> +                       atomic_dec(&zone->free_pages);
>>>>>>                         continue;
>>>>>> +               }
>>>>> I suspect the zone->free_pages is going to be expensive for you to deal
>>>>> with. It is a global atomic value and is going to have the cacheline
>>>>> bouncing that it is contained in. As a result thinks like setting the
>>>>> bitmap with be more expensive as every tome a CPU increments free_pages it
>>>>> will likely have to take the cache line containing the bitmap pointer as
>>>>> well.
>>>> I see I will have to explore this more. I am wondering if there is a way to
>>>> measure this If its effect is not visible in will-it-scale/page_fault1. If
>>>> there is a noticeable amount of degradation, I will have to address this.
>>> If nothing else you might look at seeing if you can split up the
>>> structures so that the bitmap and nr_bits is in a different region
>>> somewhere since those are read-mostly values.
>> ok, I will try to understand the issue and your suggestion.
>> Thank you for bringing this up.
>>> Also you are now updating the bitmap and free_pages both inside and
>>> outside of the zone lock so that will likely have some impact.
>> So as per your previous suggestion, I have made the bitmap structure
>> object as a rcu protected pointer. So we are safe from that side.
>> The other downside which I can think of is a race where one page
>> trying to increment free_pages and other trying to decrements it.
>> However, being an atomic variable that should not be a problem.
>> Did I miss anything?
> I'm not so much worried about a race as the cache line bouncing
> effect. Basically your notifier combined within this hinting thread
> will likely result in more time spent by the thread that holds the
> lock since it will be trying to access the bitmap to set the bit and
> the free_pages to report the bit, but at the same time you will have
> this thread clearing bits and decrementing the free_pages values.
> One thing you could consider in your worker thread would be to do
> reallocate and replace the bitmap every time you plan to walk it. By
> doing that you would avoid the cacheline bouncing on the bitmap since
> you would only have to read it, and you would no longer have another
> thread dirtying it. You could essentially reset the free_pages at the
> same time you replace the bitmap. It would need to all happen with the
> zone lock held though when you swap it out.

If I am not mistaken then from what you are suggesting, I will have to hold
the zone lock for the entire duration of swap & scan which would be costly if
the bitmap is large, isn't? Also, we might end up missing free pages that are
freed while we are scanning.

As far as free_pages count is concerned, I am thinking if I should
replace it with zone->free_area[REPORTING_ORDER].nr_free which is already there
(I still need to explore this in a bit more depth).

> - Alex

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]