Michael S. Tsirkin
2020-Apr-02 12:37 UTC
[RFC for Linux] virtio_balloon: Add VIRTIO_BALLOON_F_THP_ORDER to handle THP spilt issue
On Thu, Apr 02, 2020 at 04:00:05PM +0800, teawater wrote:> > > > 2020?3?31? 22:07?Michael S. Tsirkin <mst at redhat.com> ??? > > > > On Tue, Mar 31, 2020 at 04:03:18PM +0200, David Hildenbrand wrote: > >> On 31.03.20 15:37, Michael S. Tsirkin wrote: > >>> On Tue, Mar 31, 2020 at 03:32:05PM +0200, David Hildenbrand wrote: > >>>> On 31.03.20 15:24, Michael S. Tsirkin wrote: > >>>>> On Tue, Mar 31, 2020 at 12:35:24PM +0200, David Hildenbrand wrote: > >>>>>> On 26.03.20 10:49, Michael S. Tsirkin wrote: > >>>>>>> On Thu, Mar 26, 2020 at 08:54:04AM +0100, David Hildenbrand wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>>> Am 26.03.2020 um 08:21 schrieb Michael S. Tsirkin <mst at redhat.com>: > >>>>>>>>> > >>>>>>>>> ?On Thu, Mar 12, 2020 at 09:51:25AM +0100, David Hildenbrand wrote: > >>>>>>>>>>> On 12.03.20 09:47, Michael S. Tsirkin wrote: > >>>>>>>>>>> On Thu, Mar 12, 2020 at 09:37:32AM +0100, David Hildenbrand wrote: > >>>>>>>>>>>> 2. You are essentially stealing THPs in the guest. So the fastest > >>>>>>>>>>>> mapping (THP in guest and host) is gone. The guest won't be able to make > >>>>>>>>>>>> use of THP where it previously was able to. I can imagine this implies a > >>>>>>>>>>>> performance degradation for some workloads. This needs a proper > >>>>>>>>>>>> performance evaluation. > >>>>>>>>>>> > >>>>>>>>>>> I think the problem is more with the alloc_pages API. > >>>>>>>>>>> That gives you exactly the given order, and if there's > >>>>>>>>>>> a larger chunk available, it will split it up. > >>>>>>>>>>> > >>>>>>>>>>> But for balloon - I suspect lots of other users, > >>>>>>>>>>> we do not want to stress the system but if a large > >>>>>>>>>>> chunk is available anyway, then we could handle > >>>>>>>>>>> that more optimally by getting it all in one go. > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> So if we want to address this, IMHO this calls for a new API. > >>>>>>>>>>> Along the lines of > >>>>>>>>>>> > >>>>>>>>>>> struct page *alloc_page_range(gfp_t gfp, unsigned int min_order, > >>>>>>>>>>> unsigned int max_order, unsigned int *order) > >>>>>>>>>>> > >>>>>>>>>>> the idea would then be to return at a number of pages in the given > >>>>>>>>>>> range. > >>>>>>>>>>> > >>>>>>>>>>> What do you think? Want to try implementing that? > >>>>>>>>>> > >>>>>>>>>> You can just start with the highest order and decrement the order until > >>>>>>>>>> your allocation succeeds using alloc_pages(), which would be enough for > >>>>>>>>>> a first version. At least I don't see the immediate need for a new > >>>>>>>>>> kernel API. > >>>>>>>>> > >>>>>>>>> OK I remember now. The problem is with reclaim. Unless reclaim is > >>>>>>>>> completely disabled, any of these calls can sleep. After it wakes up, > >>>>>>>>> we would like to get the larger order that has become available > >>>>>>>>> meanwhile. > >>>>>>>>> > >>>>>>>> > >>>>>>>> Yes, but that?s a pure optimization IMHO. > >>>>>>>> So I think we should do a trivial implementation first and then see what we gain from a new allocator API. Then we might also be able to justify it using real numbers. > >>>>>>>> > >>>>>>> > >>>>>>> Well how do you propose implement the necessary semantics? > >>>>>>> I think we are both agreed that alloc_page_range is more or > >>>>>>> less what's necessary anyway - so how would you approximate it > >>>>>>> on top of existing APIs? > >>>>>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h > >>> > >>> ..... > >>> > >>> > >>>>>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c > >>>>>> index 26de020aae7b..067810b32813 100644 > >>>>>> --- a/mm/balloon_compaction.c > >>>>>> +++ b/mm/balloon_compaction.c > >>>>>> @@ -112,23 +112,35 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, > >>>>>> EXPORT_SYMBOL_GPL(balloon_page_list_dequeue); > >>>>>> > >>>>>> /* > >>>>>> - * balloon_page_alloc - allocates a new page for insertion into the balloon > >>>>>> - * page list. > >>>>>> + * balloon_pages_alloc - allocates a new page (of at most the given order) > >>>>>> + * for insertion into the balloon page list. > >>>>>> * > >>>>>> * Driver must call this function to properly allocate a new balloon page. > >>>>>> * Driver must call balloon_page_enqueue before definitively removing the page > >>>>>> * from the guest system. > >>>>>> * > >>>>>> + * Will fall back to smaller orders if allocation fails. The order of the > >>>>>> + * allocated page is stored in page->private. > >>>>>> + * > >>>>>> * Return: struct page for the allocated page or NULL on allocation failure. > >>>>>> */ > >>>>>> -struct page *balloon_page_alloc(void) > >>>>>> +struct page *balloon_pages_alloc(int order) > >>>>>> { > >>>>>> - struct page *page = alloc_page(balloon_mapping_gfp_mask() | > >>>>>> - __GFP_NOMEMALLOC | __GFP_NORETRY | > >>>>>> - __GFP_NOWARN); > >>>>>> - return page; > >>>>>> + struct page *page; > >>>>>> + > >>>>>> + while (order >= 0) { > >>>>>> + page = alloc_pages(balloon_mapping_gfp_mask() | > >>>>>> + __GFP_NOMEMALLOC | __GFP_NORETRY | > >>>>>> + __GFP_NOWARN, order); > >>>>>> + if (page) { > >>>>>> + set_page_private(page, order); > >>>>>> + return page; > >>>>>> + } > >>>>>> + order--; > >>>>>> + } > >>>>>> + return NULL; > >>>>>> } > >>>>>> -EXPORT_SYMBOL_GPL(balloon_page_alloc); > >>>>>> +EXPORT_SYMBOL_GPL(balloon_pages_alloc); > >>>>>> > >>>>>> /* > >>>>>> * balloon_page_enqueue - inserts a new page into the balloon page list. > >>>>> > >>>>> > >>>>> I think this will try to invoke direct reclaim from the first iteration > >>>>> to free up the max order. > >>>> > >>>> %__GFP_NORETRY: The VM implementation will try only very lightweight > >>>> memory direct reclaim to get some memory under memory pressure (thus it > >>>> can sleep). It will avoid disruptive actions like OOM killer. > >>>> > >>>> Certainly good enough for a first version I would say, no? > >>> > >>> Frankly how well that behaves would depend a lot on the workload. > >>> Can regress just as well. > >>> > >>> For the 1st version I'd prefer something that is the least disruptive, > >>> and that IMHO means we only trigger reclaim at all in the same configuration > >>> as now - when we can't satisfy the lowest order allocation. > >> > >> Agreed. > >> > >>> > >>> Anything else would be a huge amount of testing with all kind of > >>> workloads. > >>> > >> > >> So doing a "& ~__GFP_RECLAIM" in case order > 0? (as done in > >> GFP_TRANSHUGE_LIGHT) > > > > That will improve the situation when reclaim is not needed, but leave > > the problem in place for when it's needed: if reclaim does trigger, we > > can get a huge free page and immediately break it up. > > > > So it's ok as a first step but it will make the second step harder as > > we'll need to test with reclaim :). > > > I worry that will increases the allocation failure rate for large pages. > > I tried alloc 2M memory without __GFP_RECLAIM when I wrote the VIRTIO_BALLOON_F_THP_ORDER first version. > It will fail when I use usemem punch-holes function generates 400m fragmentation pages in the guest kernel. > > What about add another option to balloon to control with __GFP_RECLAIM or without it? > > Best, > HuiThat is why I suggested a new API so we do not fragment memory.> > > > > >> -- > >> Thanks, > >> > >> David / dhildenb