Re: [virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On Tue, Sep 10, 2019 at 06:22:37PM +0200, David Hildenbrand wrote: > On 10.09.19 18:18, Dr. David Alan Gilbert wrote: > > * Alexander Duyck (alexander.du...@gmail.com) wrote: > >> On Tue, Sep 10, 2019 at 7:47 AM Michal Hocko wrote: > >>> > >>> On Tue 10-09-19 07:42:43, Alexander Duyck wrote: > On Tue, Sep 10, 2019 at 5:42 AM Michal Hocko wrote: > > > > I wanted to review "mm: Introduce Reported pages" just realize that I > > have no clue on what is going on so returned to the cover and it didn't > > really help much. I am completely unfamiliar with virtio so please bear > > with me. > > > > On Sat 07-09-19 10:25:03, Alexander Duyck wrote: > > [...] > >> This series provides an asynchronous means of reporting to a hypervisor > >> that a guest page is no longer in use and can have the data associated > >> with it dropped. To do this I have implemented functionality that > >> allows > >> for what I am referring to as unused page reporting > >> > >> The functionality for this is fairly simple. When enabled it will > >> allocate > >> statistics to track the number of reported pages in a given free area. > >> When the number of free pages exceeds this value plus a high water > >> value, > >> currently 32, it will begin performing page reporting which consists of > >> pulling pages off of free list and placing them into a scatter list. > >> The > >> scatterlist is then given to the page reporting device and it will > >> perform > >> the required action to make the pages "reported", in the case of > >> virtio-balloon this results in the pages being madvised as > >> MADV_DONTNEED > >> and as such they are forced out of the guest. After this they are > >> placed > >> back on the free list, > > > > And here I am reallly lost because "forced out of the guest" makes me > > feel that those pages are no longer usable by the guest. So how come you > > can add them back to the free list. I suspect understanding this part > > will allow me to understand why we have to mark those pages and prevent > > merging. > > Basically as the paragraph above mentions "forced out of the guest" > really is just the hypervisor calling MADV_DONTNEED on the page in > question. So the behavior is the same as any userspace application > that calls MADV_DONTNEED where the contents are no longer accessible > from userspace and attempting to access them will result in a fault > and the page being populated with a zero fill on-demand page, or a > copy of the file contents if the memory is file backed. > >>> > >>> As I've said I have no idea about virt so this doesn't really tell me > >>> much. Does that mean that if somebody allocates such a page and tries to > >>> access it then virt will handle a fault and bring it back? > >> > >> Actually I am probably describing too much as the MADV_DONTNEED is the > >> hypervisor behavior in response to the virtio-balloon notification. A > >> more thorough explanation of it can be found by just running "man > >> madvise", probably best just to leave it at that since I am probably > >> confusing things by describing hypervisor behavior in a kernel patch > >> set. > >> > >> For the most part all the page reporting really does is provide a way > >> to incrementally identify unused regions of memory in the buddy > >> allocator. That in turn is used by virtio-balloon in a polling thread > >> to report to the hypervisor what pages are not in use so that it can > >> make a decision on what to do with the pages now that it knows they > >> are unused. > >> > >> All this is providing is just a report and it is optional if the > >> hypervisor will act on it or not. If the hypervisor takes some sort of > >> action on the page, then the expectation is that the hypervisor will > >> use some sort of mechanism such as a page fault to discover when the > >> page is used again. > > > > OK, that's interestingly different (but OK) from some other schemes that > > hav ebeen described which *require* the guest to somehow indicate the > > page is in use before starting to use the page again. > > > > virtio-balloon also has a mode where the guest would not have to > indicate to the host before re-using a page. Only > VIRTIO_BALLOON_F_MUST_TELL_HOST enforces this. So it's not completely new. VIRTIO_BALLOON_F_MUST_TELL_HOST is a bit different. When it's not set, guest still must tell host about pages in use, it just can batch these notifications sending them possibly after page has been used. So even with VIRTIO_BALLOON_F_MUST_TELL_HOST off you don't skip the notification. >From hypervisor point of view, this feature is very much like adding page to the balloon and immediately taking it out of the balloon again, just doing it in one operation. The main difference is the contents of the page, which matters with poisoning: in that
Re: [virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On 11.09.19 11:23, Michael S. Tsirkin wrote: > On Tue, Sep 10, 2019 at 06:22:37PM +0200, David Hildenbrand wrote: >> On 10.09.19 18:18, Dr. David Alan Gilbert wrote: >>> * Alexander Duyck (alexander.du...@gmail.com) wrote: On Tue, Sep 10, 2019 at 7:47 AM Michal Hocko wrote: > > On Tue 10-09-19 07:42:43, Alexander Duyck wrote: >> On Tue, Sep 10, 2019 at 5:42 AM Michal Hocko wrote: >>> >>> I wanted to review "mm: Introduce Reported pages" just realize that I >>> have no clue on what is going on so returned to the cover and it didn't >>> really help much. I am completely unfamiliar with virtio so please bear >>> with me. >>> >>> On Sat 07-09-19 10:25:03, Alexander Duyck wrote: >>> [...] This series provides an asynchronous means of reporting to a hypervisor that a guest page is no longer in use and can have the data associated with it dropped. To do this I have implemented functionality that allows for what I am referring to as unused page reporting The functionality for this is fairly simple. When enabled it will allocate statistics to track the number of reported pages in a given free area. When the number of free pages exceeds this value plus a high water value, currently 32, it will begin performing page reporting which consists of pulling pages off of free list and placing them into a scatter list. The scatterlist is then given to the page reporting device and it will perform the required action to make the pages "reported", in the case of virtio-balloon this results in the pages being madvised as MADV_DONTNEED and as such they are forced out of the guest. After this they are placed back on the free list, >>> >>> And here I am reallly lost because "forced out of the guest" makes me >>> feel that those pages are no longer usable by the guest. So how come you >>> can add them back to the free list. I suspect understanding this part >>> will allow me to understand why we have to mark those pages and prevent >>> merging. >> >> Basically as the paragraph above mentions "forced out of the guest" >> really is just the hypervisor calling MADV_DONTNEED on the page in >> question. So the behavior is the same as any userspace application >> that calls MADV_DONTNEED where the contents are no longer accessible >> from userspace and attempting to access them will result in a fault >> and the page being populated with a zero fill on-demand page, or a >> copy of the file contents if the memory is file backed. > > As I've said I have no idea about virt so this doesn't really tell me > much. Does that mean that if somebody allocates such a page and tries to > access it then virt will handle a fault and bring it back? Actually I am probably describing too much as the MADV_DONTNEED is the hypervisor behavior in response to the virtio-balloon notification. A more thorough explanation of it can be found by just running "man madvise", probably best just to leave it at that since I am probably confusing things by describing hypervisor behavior in a kernel patch set. For the most part all the page reporting really does is provide a way to incrementally identify unused regions of memory in the buddy allocator. That in turn is used by virtio-balloon in a polling thread to report to the hypervisor what pages are not in use so that it can make a decision on what to do with the pages now that it knows they are unused. All this is providing is just a report and it is optional if the hypervisor will act on it or not. If the hypervisor takes some sort of action on the page, then the expectation is that the hypervisor will use some sort of mechanism such as a page fault to discover when the page is used again. >>> >>> OK, that's interestingly different (but OK) from some other schemes that >>> hav ebeen described which *require* the guest to somehow indicate the >>> page is in use before starting to use the page again. >>> >> >> virtio-balloon also has a mode where the guest would not have to >> indicate to the host before re-using a page. Only >> VIRTIO_BALLOON_F_MUST_TELL_HOST enforces this. So it's not completely new. > > VIRTIO_BALLOON_F_MUST_TELL_HOST is a bit different. > When it's not set, guest still must tell host about > pages in use, it just can batch these notifications > sending them possibly after page has been used. > So even with VIRTIO_BALLOON_F_MUST_TELL_HOST off you don't > skip the notification. > I don't think so VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */ commit bf50e69f63d21091e525185c3ae761412be0ba72 Author: Dave Hansen Date: Thu Apr 7 10:43:25 2011 -0700
[virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On 11.09.19 14:25, Michal Hocko wrote: > On Wed 11-09-19 14:19:41, Michal Hocko wrote: >> On Wed 11-09-19 08:08:38, Michael S. Tsirkin wrote: >>> On Wed, Sep 11, 2019 at 01:36:19PM +0200, Michal Hocko wrote: On Tue 10-09-19 14:23:40, Alexander Duyck wrote: [...] > We don't put any limitations on the allocator other then that it needs to > clean up the metadata on allocation, and that it cannot allocate a page > that is in the process of being reported since we pulled it from the > free_list. If the page is a "Reported" page then it decrements the > reported_pages count for the free_area and makes sure the page doesn't > exist in the "Boundary" array pointer value, if it does it moves the > "Boundary" since it is pulling the page. This is still a non-trivial limitation on the page allocation from an external code IMHO. I cannot give any explicit reason why an ordering on the free list might matter (well except for page shuffling which uses it to make physical memory pattern allocation more random) but the architecture seems hacky and dubious to be honest. It shoulds like the whole interface has been developed around a very particular and single purpose optimization. I remember that there was an attempt to report free memory that provided a callback mechanism [1], which was much less intrusive to the internals of the allocator yet it should provide a similar functionality. Did you see that approach? How does this compares to it? Or am I completely off when comparing them? [1] mostly likely not the latest version of the patchset http://lkml.kernel.org/r/1502940416-42944-5-git-send-email-wei.w.w...@intel.com >>> >>> Linus nacked that one. He thinks invoking callbacks with lots of >>> internal mm locks is too fragile. >> >> I would be really curious how much he would be happy about injecting >> other restrictions on the allocator like this patch proposes. This is >> more intrusive as it has a higher maintenance cost longterm IMHO. > > Btw. I do agree that callbacks with internal mm locks are not great > either. We do have a model for that in mmu_notifiers and it is something > I do consider PITA, on the other hand it is mostly sleepable part of the > interface which makes it the real pain. The above callback mechanism was > explicitly documented with restrictions and that the context is > essentially atomic with no access to particular struct pages and no > expensive operations possible. So in the end I've considered it > acceptably painful. Not that I want to override Linus' nack but if > virtualization usecases really require some form of reporting and no > other way to do that push people to invent even more interesting > approaches then we should simply give them/you something reasonable > and least intrusive to our internals. > The issue with "[PATCH v14 4/5] mm: support reporting free page blocks" is that it cannot really handle the use case we have here if I am not wrong. While a page is getting processed by the hypervisor (e.g. MADV_DONTNEED), it must not get reused. "Some page blocks may leave the free list after zone->lock is released, so it is the caller's responsibility to either detect or prevent the use of such pages." If I'm not wrong, this only made sense to speed up migration in the hypervisor, where you can deal with false positives differently. -- Thanks, David / dhildenb - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On 9/11/19 8:42 AM, David Hildenbrand wrote: > On 11.09.19 14:25, Michal Hocko wrote: >> On Wed 11-09-19 14:19:41, Michal Hocko wrote: >>> On Wed 11-09-19 08:08:38, Michael S. Tsirkin wrote: On Wed, Sep 11, 2019 at 01:36:19PM +0200, Michal Hocko wrote: > On Tue 10-09-19 14:23:40, Alexander Duyck wrote: > [...] >> We don't put any limitations on the allocator other then that it needs to >> clean up the metadata on allocation, and that it cannot allocate a page >> that is in the process of being reported since we pulled it from the >> free_list. If the page is a "Reported" page then it decrements the >> reported_pages count for the free_area and makes sure the page doesn't >> exist in the "Boundary" array pointer value, if it does it moves the >> "Boundary" since it is pulling the page. > This is still a non-trivial limitation on the page allocation from an > external code IMHO. I cannot give any explicit reason why an ordering on > the free list might matter (well except for page shuffling which uses it > to make physical memory pattern allocation more random) but the > architecture seems hacky and dubious to be honest. It shoulds like the > whole interface has been developed around a very particular and single > purpose optimization. > > I remember that there was an attempt to report free memory that provided > a callback mechanism [1], which was much less intrusive to the internals > of the allocator yet it should provide a similar functionality. Did you > see that approach? How does this compares to it? Or am I completely off > when comparing them? > > [1] mostly likely not the latest version of the patchset > http://lkml.kernel.org/r/1502940416-42944-5-git-send-email-wei.w.w...@intel.com Linus nacked that one. He thinks invoking callbacks with lots of internal mm locks is too fragile. >>> I would be really curious how much he would be happy about injecting >>> other restrictions on the allocator like this patch proposes. This is >>> more intrusive as it has a higher maintenance cost longterm IMHO. >> Btw. I do agree that callbacks with internal mm locks are not great >> either. We do have a model for that in mmu_notifiers and it is something >> I do consider PITA, on the other hand it is mostly sleepable part of the >> interface which makes it the real pain. The above callback mechanism was >> explicitly documented with restrictions and that the context is >> essentially atomic with no access to particular struct pages and no >> expensive operations possible. So in the end I've considered it >> acceptably painful. Not that I want to override Linus' nack but if >> virtualization usecases really require some form of reporting and no >> other way to do that push people to invent even more interesting >> approaches then we should simply give them/you something reasonable >> and least intrusive to our internals. >> > The issue with "[PATCH v14 4/5] mm: support reporting free page blocks" > is that it cannot really handle the use case we have here if I am not > wrong. While a page is getting processed by the hypervisor (e.g. > MADV_DONTNEED), it must not get reused. > > "Some page blocks may > leave the free list after zone->lock is released, so it is the caller's > responsibility to either detect or prevent the use of such pages." > > If I'm not wrong, this only made sense to speed up migration in the > hypervisor, where you can deal with false positives differently. Another difference between the two approaches is the origin from where the reporting request is getting generated. (If I remember correctly) In Alexander's series or in my series [1], VM is able to report pages dynamically without any requirement of host intervention. [1] https://lkml.org/lkml/2019/8/12/593 -- Thanks Nitesh - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On 11.09.19 14:54, Michal Hocko wrote:
> On Wed 11-09-19 14:42:41, David Hildenbrand wrote:
>> On 11.09.19 14:25, Michal Hocko wrote:
>>> On Wed 11-09-19 14:19:41, Michal Hocko wrote:
On Wed 11-09-19 08:08:38, Michael S. Tsirkin wrote:
> On Wed, Sep 11, 2019 at 01:36:19PM +0200, Michal Hocko wrote:
>> On Tue 10-09-19 14:23:40, Alexander Duyck wrote:
>> [...]
>>> We don't put any limitations on the allocator other then that it needs
>>> to
>>> clean up the metadata on allocation, and that it cannot allocate a page
>>> that is in the process of being reported since we pulled it from the
>>> free_list. If the page is a "Reported" page then it decrements the
>>> reported_pages count for the free_area and makes sure the page doesn't
>>> exist in the "Boundary" array pointer value, if it does it moves the
>>> "Boundary" since it is pulling the page.
>>
>> This is still a non-trivial limitation on the page allocation from an
>> external code IMHO. I cannot give any explicit reason why an ordering on
>> the free list might matter (well except for page shuffling which uses it
>> to make physical memory pattern allocation more random) but the
>> architecture seems hacky and dubious to be honest. It shoulds like the
>> whole interface has been developed around a very particular and single
>> purpose optimization.
>>
>> I remember that there was an attempt to report free memory that provided
>> a callback mechanism [1], which was much less intrusive to the internals
>> of the allocator yet it should provide a similar functionality. Did you
>> see that approach? How does this compares to it? Or am I completely off
>> when comparing them?
>>
>> [1] mostly likely not the latest version of the patchset
>> http://lkml.kernel.org/r/1502940416-42944-5-git-send-email-wei.w.w...@intel.com
>
> Linus nacked that one. He thinks invoking callbacks with lots of
> internal mm locks is too fragile.
I would be really curious how much he would be happy about injecting
other restrictions on the allocator like this patch proposes. This is
more intrusive as it has a higher maintenance cost longterm IMHO.
>>>
>>> Btw. I do agree that callbacks with internal mm locks are not great
>>> either. We do have a model for that in mmu_notifiers and it is something
>>> I do consider PITA, on the other hand it is mostly sleepable part of the
>>> interface which makes it the real pain. The above callback mechanism was
>>> explicitly documented with restrictions and that the context is
>>> essentially atomic with no access to particular struct pages and no
>>> expensive operations possible. So in the end I've considered it
>>> acceptably painful. Not that I want to override Linus' nack but if
>>> virtualization usecases really require some form of reporting and no
>>> other way to do that push people to invent even more interesting
>>> approaches then we should simply give them/you something reasonable
>>> and least intrusive to our internals.
>>>
>>
>> The issue with "[PATCH v14 4/5] mm: support reporting free page blocks"
>> is that it cannot really handle the use case we have here if I am not
>> wrong. While a page is getting processed by the hypervisor (e.g.
>> MADV_DONTNEED), it must not get reused.
>
> What prevents to use the callback to get a list of pfn ranges to work on
> and then use something like start_isolate_page_range on the collected
> pfn ranges to make sure nobody steals pages from under your feet, do
> your thing and drop the isolated state afterwards.
>
> I am saying somethig like because you wouldn't really want a generic
> has_unmovable_pages but rather
> if (!page_ref_count(page)) {
> if (PageBuddy(page))
> iter += (1 << page_order(page)) - 1;
> continue;
> }
> subset of it.
>
Something slightly similar is being performed by Nitesh's patch set. On
every free of a certain granularity, he records it in the bitmap. These
bits are "hints of free pages".
A thread then walks over the bitmap and tries to allocate the "hints".
If the pages were already reused, the bit is silently cleared.
Instead of allocating/freeing, we could only try to isolate the
pageblock, then test if free. (One of the usual issues to work around is
MAX_ORDER-1 crossing pageblocks, that might need special care)
I think you should have a look at the rough idea of Nitesh's patch set
to see if something like that is going into a better direction. The
bitmap part is in place to do bulk reporting and avoid duplicate reports.
I think main points we want (and what I am missing from callback idea
being discussed) are
1. Do bulk reporting only when a certain threshold is reached
2. Report only bigger granularities (especially, avoid THP splits in the
hypervisor - >= 2MB proofed to be effective)
3. Avoid
[virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On 9/11/19 8:54 AM, Michal Hocko wrote:
> On Wed 11-09-19 14:42:41, David Hildenbrand wrote:
>> On 11.09.19 14:25, Michal Hocko wrote:
>>> On Wed 11-09-19 14:19:41, Michal Hocko wrote:
On Wed 11-09-19 08:08:38, Michael S. Tsirkin wrote:
> On Wed, Sep 11, 2019 at 01:36:19PM +0200, Michal Hocko wrote:
>> On Tue 10-09-19 14:23:40, Alexander Duyck wrote:
>> [...]
>>> We don't put any limitations on the allocator other then that it needs
>>> to
>>> clean up the metadata on allocation, and that it cannot allocate a page
>>> that is in the process of being reported since we pulled it from the
>>> free_list. If the page is a "Reported" page then it decrements the
>>> reported_pages count for the free_area and makes sure the page doesn't
>>> exist in the "Boundary" array pointer value, if it does it moves the
>>> "Boundary" since it is pulling the page.
>> This is still a non-trivial limitation on the page allocation from an
>> external code IMHO. I cannot give any explicit reason why an ordering on
>> the free list might matter (well except for page shuffling which uses it
>> to make physical memory pattern allocation more random) but the
>> architecture seems hacky and dubious to be honest. It shoulds like the
>> whole interface has been developed around a very particular and single
>> purpose optimization.
>>
>> I remember that there was an attempt to report free memory that provided
>> a callback mechanism [1], which was much less intrusive to the internals
>> of the allocator yet it should provide a similar functionality. Did you
>> see that approach? How does this compares to it? Or am I completely off
>> when comparing them?
>>
>> [1] mostly likely not the latest version of the patchset
>> http://lkml.kernel.org/r/1502940416-42944-5-git-send-email-wei.w.w...@intel.com
> Linus nacked that one. He thinks invoking callbacks with lots of
> internal mm locks is too fragile.
I would be really curious how much he would be happy about injecting
other restrictions on the allocator like this patch proposes. This is
more intrusive as it has a higher maintenance cost longterm IMHO.
>>> Btw. I do agree that callbacks with internal mm locks are not great
>>> either. We do have a model for that in mmu_notifiers and it is something
>>> I do consider PITA, on the other hand it is mostly sleepable part of the
>>> interface which makes it the real pain. The above callback mechanism was
>>> explicitly documented with restrictions and that the context is
>>> essentially atomic with no access to particular struct pages and no
>>> expensive operations possible. So in the end I've considered it
>>> acceptably painful. Not that I want to override Linus' nack but if
>>> virtualization usecases really require some form of reporting and no
>>> other way to do that push people to invent even more interesting
>>> approaches then we should simply give them/you something reasonable
>>> and least intrusive to our internals.
>>>
>> The issue with "[PATCH v14 4/5] mm: support reporting free page blocks"
>> is that it cannot really handle the use case we have here if I am not
>> wrong. While a page is getting processed by the hypervisor (e.g.
>> MADV_DONTNEED), it must not get reused.
> What prevents to use the callback to get a list of pfn ranges to work on
> and then use something like start_isolate_page_range on the collected
> pfn ranges to make sure nobody steals pages from under your feet, do
> your thing and drop the isolated state afterwards.
>
In my series, I am doing something similar.
- Track (MAX_ORDER - 2) free pages in bitmap maintained on a per-zone
basis.
- Use __isolate_free_page on the pages marked in the bitmap and are
still free.
- Report chunks of 16 isolated pages to the hypervisor.
- Return them back to the buddy once the request is processed.
> I am saying somethig like because you wouldn't really want a generic
> has_unmovable_pages but rather
> if (!page_ref_count(page)) {
> if (PageBuddy(page))
> iter += (1 << page_order(page)) - 1;
> continue;
> }
> subset of it.
--
Thanks
Nitesh
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On Wed, Sep 11, 2019 at 01:36:19PM +0200, Michal Hocko wrote: > On Tue 10-09-19 14:23:40, Alexander Duyck wrote: > [...] > > We don't put any limitations on the allocator other then that it needs to > > clean up the metadata on allocation, and that it cannot allocate a page > > that is in the process of being reported since we pulled it from the > > free_list. If the page is a "Reported" page then it decrements the > > reported_pages count for the free_area and makes sure the page doesn't > > exist in the "Boundary" array pointer value, if it does it moves the > > "Boundary" since it is pulling the page. > > This is still a non-trivial limitation on the page allocation from an > external code IMHO. I cannot give any explicit reason why an ordering on > the free list might matter (well except for page shuffling which uses it > to make physical memory pattern allocation more random) but the > architecture seems hacky and dubious to be honest. It shoulds like the > whole interface has been developed around a very particular and single > purpose optimization. > > I remember that there was an attempt to report free memory that provided > a callback mechanism [1], which was much less intrusive to the internals > of the allocator yet it should provide a similar functionality. Did you > see that approach? How does this compares to it? Or am I completely off > when comparing them? > > [1] mostly likely not the latest version of the patchset > http://lkml.kernel.org/r/1502940416-42944-5-git-send-email-wei.w.w...@intel.com > > -- > Michal Hocko > SUSE Labs Linus nacked that one. He thinks invoking callbacks with lots of internal mm locks is too fragile. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On 11.09.19 13:36, Michal Hocko wrote: > On Tue 10-09-19 14:23:40, Alexander Duyck wrote: > [...] >> We don't put any limitations on the allocator other then that it needs to >> clean up the metadata on allocation, and that it cannot allocate a page >> that is in the process of being reported since we pulled it from the >> free_list. If the page is a "Reported" page then it decrements the >> reported_pages count for the free_area and makes sure the page doesn't >> exist in the "Boundary" array pointer value, if it does it moves the >> "Boundary" since it is pulling the page. > > This is still a non-trivial limitation on the page allocation from an > external code IMHO. I cannot give any explicit reason why an ordering on > the free list might matter (well except for page shuffling which uses it > to make physical memory pattern allocation more random) but the > architecture seems hacky and dubious to be honest. It shoulds like the > whole interface has been developed around a very particular and single > purpose optimization. > > I remember that there was an attempt to report free memory that provided > a callback mechanism [1], which was much less intrusive to the internals > of the allocator yet it should provide a similar functionality. Did you > see that approach? How does this compares to it? Or am I completely off > when comparing them? > > [1] mostly likely not the latest version of the patchset > http://lkml.kernel.org/r/1502940416-42944-5-git-send-email-wei.w.w...@intel.com > FWIW, Nitesh was looking into another approach [1], whereby the metadata is stored outside of the buddy (unreported pages are tracked in a bitmap). There are some limitations to this approach (esp., sparse zones might waste memory (1bit per 2MB), memory hot(un)plug not supported yet completely, scanning of the bitmap necessary). OTOH, the core buddy modifications are minimized. [1] https://lkml.org/lkml/2019/8/12/593 -- Thanks, David / dhildenb - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [RFC][PATCH v12 0/2] mm: Support for page reporting
On 12.08.19 15:12, Nitesh Narayan Lal wrote:
> This patch series proposes an efficient mechanism for reporting free memory
> from a guest to its hypervisor. It especially enables guests with no page
> cache
> (e.g., nvdimm, virtio-pmem) or with small page caches (e.g., ram > disk) to
> rapidly hand back free memory to the hypervisor.
> This approach has a minimal impact on the existing core-mm infrastructure.
>
> This approach tracks all freed pages of the order MAX_ORDER - 2 in bitmaps.
> A new hook after buddy merging is used to set the bits in the bitmap for a
> freed
> page. Each set bit is cleared after they are processed/checked for
> re-allocation.
> Bitmaps are stored on a per-zone basis and are protected by the zone lock. A
> workqueue asynchronously processes the bitmaps as soon as a pre-defined memory
> threshold is met, trying to isolate and report pages that are still free.
> The isolated pages are stored in a scatterlist and are reported via
> virtio-balloon, which is responsible for sending batched pages to the
> hypervisor. Once the hypervisor processed the reporting request, the isolated
> pages are returned back to the buddy.
> The thershold which defines the number of pages which will be isolated and
> reported to the hypervisor at a time is currently hardcoded to 16 in the
> guest.
>
> Benefit analysis:
> Number of 5 GB guests (each touching 4 to 5 GB memory) that can be launched
> on a
> 15 GB single NUMA system without using swap space in the host.
>
> Guest kernel--> Unmodified with v12 page reporting
> Number of guests--> 2 7
>
> Conclusion: In a page-reporting enabled kernel, the guest is able to report
> most of its unused memory back to the host. Due to this on the same host, I
> was
> able to launch 7 guests without touching any swap compared to 2 which were
> launched with an unmodified kernel.
>
> Performance Analysis:
> In order to measure the performance impact of this patch-series over an
> unmodified kernel, I am using will-it-scale/page_fault1 on a 30 GB, 24 vcpus
> single NUMA guest which is affined to a single node in the host. Over several
> runs, I observed that with this patch-series there is a degradation of around
> 1-3% for certain cases. This degradation could be a result of page-zeroing
> overhead which comes with every page-fault in the guest.
> I also tried this test on a 2 NUMA node host running page reporting
> enabled 60GB guest also having 2 NUMA nodes and 24 vcpus. I observed a similar
> degradation of around 1-3% in most of the cases.
> For certain cases, the variability even with an unmodified kernel was around
> 4-6% with every fresh boot. I will continue to investigate this further to
> find
> the reason behind it.
>
> Ongoing work-items:
> * I have a working prototype for supporting memory hotplug/hotremove with page
> reporting. However, it still requires more testing and fixes specifically on
> the hotremove side.
> Right now, for any memory hotplug or hotremove request bitmap or its
> respective fields are not changed. Hence, memory added via hotplug is not
> tracked in the bitmap. Similarly, removed memory is not reported to the
> hypervisor by using an online memory check.
> * I will also have to look into the details about how to handle page poisoning
> scenarios and test with directly assigned devices.
>
>
> Changes from v11:
> https://lkml.org/lkml/2019/7/10/742
> * Moved the fields required to manage bitmap of free pages to 'struct zone'.
> * Replaced the list which was used to hold and report the free pages with
> scatterlist.
> * Tried to fix the anti-kernel patterns and improve overall code quality.
> * Fixed a few bugs in the code which were reported in the last posting.
> * Moved to use MADV_DONTNEED from MADV_FREE.
> * Replaced page hinting in favor of page reporting.
> * Addressed other comments which I received in the last posting.
>
>
> Changes from v10:
> https://lkml.org/lkml/2019/6/3/943
> * Added logic to take care of multiple NUMA nodes scenarios.
> * Simplified the logic for reporting isolated pages to the host. (Eg. replaced
> dynamically allocated arrays with static ones, introduced wait event instead
> of the loop in order to wait for a response from the host)
> * Added a mutex to prevent race condition when page reporting is enabled by
> multiple drivers.
> * Simplified the logic responsible for decrementing free page counter for each
> zone.
> * Simplified code structuring/naming.
>
Some current limitations of this patchset seem to be
1. Sparse zones eventually wasting memory (1bit per 2MB).
As I already set, I consider this in most virtual environments a special
case (especially a lot of sparsity). You can simply compare the spanned
vs. present pages and don't allocate a bitmap in case it's too sparse
("currently unsupported environment"). These pieces won't be considered
for free page reporting,
[virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On Wed, Sep 11, 2019 at 4:36 AM Michal Hocko wrote: > > On Tue 10-09-19 14:23:40, Alexander Duyck wrote: > [...] > > We don't put any limitations on the allocator other then that it needs to > > clean up the metadata on allocation, and that it cannot allocate a page > > that is in the process of being reported since we pulled it from the > > free_list. If the page is a "Reported" page then it decrements the > > reported_pages count for the free_area and makes sure the page doesn't > > exist in the "Boundary" array pointer value, if it does it moves the > > "Boundary" since it is pulling the page. > > This is still a non-trivial limitation on the page allocation from an > external code IMHO. I cannot give any explicit reason why an ordering on > the free list might matter (well except for page shuffling which uses it > to make physical memory pattern allocation more random) but the > architecture seems hacky and dubious to be honest. It shoulds like the > whole interface has been developed around a very particular and single > purpose optimization. How is this any different then the code that moves a page that will likely be merged to the tail though? In our case the "Reported" page is likely going to be much more expensive to allocate and use then a standard page because it will be faulted back in. In such a case wouldn't it make sense for us to want to keep the pages that don't require faults ahead of those pages in the free_list so that they are more likely to be allocated? All we are doing with the boundary list is preventing still resident pages from being deferred behind pages that would require a page fault to get access to. > I remember that there was an attempt to report free memory that provided > a callback mechanism [1], which was much less intrusive to the internals > of the allocator yet it should provide a similar functionality. Did you > see that approach? How does this compares to it? Or am I completely off > when comparing them? > > [1] mostly likely not the latest version of the patchset > http://lkml.kernel.org/r/1502940416-42944-5-git-send-email-wei.w.w...@intel.com There have been a few comparisons between this patch set and the ones from Wei Wang. In regards to the one you are pointing to the main difference is that I am not permanently locking memory. Basically what happens is that the iterator will take the lock, pull a few pages, release the lock while reporting them, and then take the lock to return those pages, grab some more, and repeat. I was actually influenced somewhat by the suggestions that patchset received, specifically I believe it resembles something like what was suggested by Linus in response to v35 of that patch set: https://lore.kernel.org/linux-mm/ca+55afzqj8wxxnhadutioomipgfonvbqkmjl_tfk7e5ar1f...@mail.gmail.com/ Basically where the feature Wei Wang was working on differs from this patch set is that I need this to run continually, his only needed to run periodically as he was just trying to identify free pages at a fixed point in time. My goal is to identify pages that have been freed since the last time I reported them. To do that I need a flag in the page to identify those pages, and an iterator in the form of a boundary pointer so that I can incrementally walk through the list without losing track of freed pages. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On 9/11/19 9:20 AM, Michal Hocko wrote:
> On Wed 11-09-19 15:03:39, David Hildenbrand wrote:
>> On 11.09.19 14:54, Michal Hocko wrote:
>>> On Wed 11-09-19 14:42:41, David Hildenbrand wrote:
On 11.09.19 14:25, Michal Hocko wrote:
> On Wed 11-09-19 14:19:41, Michal Hocko wrote:
>> On Wed 11-09-19 08:08:38, Michael S. Tsirkin wrote:
>>> On Wed, Sep 11, 2019 at 01:36:19PM +0200, Michal Hocko wrote:
On Tue 10-09-19 14:23:40, Alexander Duyck wrote:
[...]
> We don't put any limitations on the allocator other then that it
> needs to
> clean up the metadata on allocation, and that it cannot allocate a
> page
> that is in the process of being reported since we pulled it from the
> free_list. If the page is a "Reported" page then it decrements the
> reported_pages count for the free_area and makes sure the page doesn't
> exist in the "Boundary" array pointer value, if it does it moves the
> "Boundary" since it is pulling the page.
This is still a non-trivial limitation on the page allocation from an
external code IMHO. I cannot give any explicit reason why an ordering
on
the free list might matter (well except for page shuffling which uses
it
to make physical memory pattern allocation more random) but the
architecture seems hacky and dubious to be honest. It shoulds like the
whole interface has been developed around a very particular and single
purpose optimization.
I remember that there was an attempt to report free memory that
provided
a callback mechanism [1], which was much less intrusive to the
internals
of the allocator yet it should provide a similar functionality. Did you
see that approach? How does this compares to it? Or am I completely off
when comparing them?
[1] mostly likely not the latest version of the patchset
http://lkml.kernel.org/r/1502940416-42944-5-git-send-email-wei.w.w...@intel.com
>>> Linus nacked that one. He thinks invoking callbacks with lots of
>>> internal mm locks is too fragile.
>> I would be really curious how much he would be happy about injecting
>> other restrictions on the allocator like this patch proposes. This is
>> more intrusive as it has a higher maintenance cost longterm IMHO.
> Btw. I do agree that callbacks with internal mm locks are not great
> either. We do have a model for that in mmu_notifiers and it is something
> I do consider PITA, on the other hand it is mostly sleepable part of the
> interface which makes it the real pain. The above callback mechanism was
> explicitly documented with restrictions and that the context is
> essentially atomic with no access to particular struct pages and no
> expensive operations possible. So in the end I've considered it
> acceptably painful. Not that I want to override Linus' nack but if
> virtualization usecases really require some form of reporting and no
> other way to do that push people to invent even more interesting
> approaches then we should simply give them/you something reasonable
> and least intrusive to our internals.
>
The issue with "[PATCH v14 4/5] mm: support reporting free page blocks"
is that it cannot really handle the use case we have here if I am not
wrong. While a page is getting processed by the hypervisor (e.g.
MADV_DONTNEED), it must not get reused.
>>> What prevents to use the callback to get a list of pfn ranges to work on
>>> and then use something like start_isolate_page_range on the collected
>>> pfn ranges to make sure nobody steals pages from under your feet, do
>>> your thing and drop the isolated state afterwards.
>>>
>>> I am saying somethig like because you wouldn't really want a generic
>>> has_unmovable_pages but rather
>>> if (!page_ref_count(page)) {
>>> if (PageBuddy(page))
>>> iter += (1 << page_order(page)) - 1;
>>> continue;
>>> }
>>> subset of it.
>>>
>> Something slightly similar is being performed by Nitesh's patch set. On
>> every free of a certain granularity, he records it in the bitmap. These
>> bits are "hints of free pages".
>>
>> A thread then walks over the bitmap and tries to allocate the "hints".
>> If the pages were already reused, the bit is silently cleared.
>>
>> Instead of allocating/freeing, we could only try to isolate the
>> pageblock, then test if free. (One of the usual issues to work around is
>> MAX_ORDER-1 crossing pageblocks, that might need special care)
> OK, cool that I have reinvented the wheel ;). Allocation is indeed not
> necessary as long as pages are isolated because nobody will allocate
> them.
>
>> I think you should have a look at the rough idea of
[virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
On 11.09.19 15:51, Michal Hocko wrote: > On Wed 11-09-19 15:20:02, Michal Hocko wrote: > [...] >>> 4. Continuously report, not the "one time report everything" approach. >> >> So you mean the allocator reporting this rather than an external code to >> poll right? I do not know, how much this is nice to have than must have? > > Another idea that I haven't really thought through so it might turned > out to be completely bogus but let's try anyway. Your "report everything" > just made me look and realize that free_pages_prepare already performs > stuff that actually does something similar yet unrelated. > > We do report to special page poisoning, zeroying or > CONFIG_DEBUG_PAGEALLOC to unmap the address from the kernel address > space. This sounds like something fitting your model no? > AFAIKS, the poisoning/unmapping is done whenever a page is freed. I don't quite see yet how that would help to remember if a page was already reported. After reporting the page we would have to switch some state (Nitesh: bitmap bit, Alexander: page flag) to identify that. Of course, we could map the page and treat that as "the state" when we reported it, but I am not sure that's such a good idea :) As always, I might be very wrong ... -- Thanks, David / dhildenb - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9 0/8] stg mail -e --version=v9 \
>> Something slightly similar is being performed by Nitesh's patch set. On >> every free of a certain granularity, he records it in the bitmap. These >> bits are "hints of free pages". >> >> A thread then walks over the bitmap and tries to allocate the "hints". >> If the pages were already reused, the bit is silently cleared. >> >> Instead of allocating/freeing, we could only try to isolate the >> pageblock, then test if free. (One of the usual issues to work around is >> MAX_ORDER-1 crossing pageblocks, that might need special care) > > OK, cool that I have reinvented the wheel ;). Allocation is indeed not > necessary as long as pages are isolated because nobody will allocate > them. It's always good if you come to the same conclusion ;) > >> I think you should have a look at the rough idea of Nitesh's patch set >> to see if something like that is going into a better direction. The >> bitmap part is in place to do bulk reporting and avoid duplicate reports. > > Let's see how much time I can find for that in my endless inbox whack a mole. Can totally understand - it's only a single patch. > >> I think main points we want (and what I am missing from callback idea >> being discussed) are >> 1. Do bulk reporting only when a certain threshold is reached > > Is a time based approach too coarse? Usually that's then another parameter to fine tune, something to avoid when just reporting continuously. I wouldn't say it's a no go, but at least I would prefer right now to do it continuously. > >> 2. Report only bigger granularities (especially, avoid THP splits in the >> hypervisor - >= 2MB proofed to be effective) > > the callback has supported order based scan in some of its iteration. Missed that. But yeah, the other points are more important :) > >> 3. Avoid reporting what has just been reported. > > Is the overhead of checking a pfn range in a bitmask that much of an > overhead to really care? It's all about remembering what has already been reported. Nitesh solved that via the bitmap. So he does exactly that. If we can optimize the bitmap out - perfect - but I don't see an easy way to do that :) > >> 4. Continuously report, not the "one time report everything" approach. > > So you mean the allocator reporting this rather than an external code to > poll right? I do not know, how much this is nice to have than must have? I guess it is debatable - but I don't consider this one of the fundamental issues. How to identify what to report and remember what has already been reported is the main issue. Polling vs. notification is just the cherry on top - IMHO. -- Thanks, David / dhildenb - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
