On 12/26/2017 06:38 PM, Tetsuo Handa wrote:
Wei Wang wrote:
On 12/25/2017 10:51 PM, Tetsuo Handa wrote:
Wei Wang wrote:
What we are doing here is to free the pages that were just allocated in
this round of inflating. Next round will be sometime later when the
balloon work item gets its turn to run. Yes, it will then continue to
inflate.
Here are the two cases that will happen then:
1) the guest is still under memory pressure, the inflate will fail at
memory allocation, which results in a msleep(200), and then it exists
for another time to run.
2) the guest isn't under memory pressure any more (e.g. the task which
consumes the huge amount of memory is gone), it will continue to inflate
as normal till the requested size.
How likely does 2) occur? It is not so likely. msleep(200) is enough to spam
the guest with puff messages. Next round is starting too quickly.
I meant one of the two cases, 1) or 2), would happen, rather than 2)
happens after 1).
If 2) doesn't happen, then 1) happens. It will continue to try to
inflate round by round. But the memory allocation won't succeed, so
there will be no pages to inflate to the host. That is, the inflating is
simply a code path to the msleep(200) as long as the guest is under
memory pressure.
Back to our code change, it doesn't result in incorrect behavior as
explained above.
I think what we are doing is a quite sensible behavior, except a small
change I plan to make:
while ((page = balloon_page_pop(&pages))) {
- balloon_page_enqueue(&vb->vb_dev_info, page);
if (use_sg) {
if (xb_set_page(vb, page, &pfn_min, &pfn_max) <
0) {
__free_page(page);
continue;
}
} else {
set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
}
+ balloon_page_enqueue(&vb->vb_dev_info, page);
Also, as of Linux 4.15, only up to VIRTIO_BALLOON_ARRAY_PFNS_MAX pages (i.e.
1MB) are invisible from deflate request. That amount would be an acceptable
error. But your patch makes more pages being invisible, for pages allocated
by balloon_page_alloc() without holding balloon_lock are stored into a local
variable "LIST_HEAD(pages)" (which means that balloon_page_dequeue() with
balloon_lock held won't be able to find pages not yet queued by
balloon_page_enqueue()), doesn't it? What if all memory pages were held in
"LIST_HEAD(pages)" and balloon_page_dequeue() was called before
balloon_page_enqueue() is called?
If we think of the balloon driver just as a regular driver or
application, that will be a pretty nature thing. A regular driver can
eat a huge amount of memory for its own usages, would this amount of
memory be treated as an error as they are invisible to the
balloon_page_enqueue?
No. Memory used by applications which consumed a lot of memory in their
mm_struct is reclaimed by the OOM killer/reaper. Drivers try to avoid
allocating more memory than they need. If drivers allocate more memory
than they need, they have a hook for releasing unused memory (i.e.
register_shrinker() or OOM notifier). What I'm saying here is that
the hook for releasing unused memory does not work unless memory held in
LIST_HEAD(pages) becomes visible to balloon_page_dequeue().
If a system has 128GB of memory, and 127GB of memory was stored into
LIST_HEAD(pages) upon first fill_balloon() request, and somebody held
balloon_lock from OOM notifier path from out_of_memory() before
fill_balloon() holds balloon_lock, leak_balloon_sg_oom() finds that
no memory can be freed because balloon_page_enqueue() was never called,
and allows the caller of out_of_memory() to invoke the OOM killer despite
there is 127GB of memory which can be freed if fill_balloon() was able
to hold balloon_lock before leak_balloon_sg_oom() holds balloon_lock.
I don't think that that amount is an acceptable error.
I understand you are worried that OOM couldn't get balloon pages while
there are some in the local list. This is a debatable issue, and it may
lead to a long discussion. If this is considered to be a big issue, we
can make the local list to be global in vb, and accessed by oom
notifier, this won't affect this patch, and can be achieved with an
add-on patch. How about leaving this discussion as a second step outside
this series? Balloon has something more that can be improved, and this
patch series is already big.
Best,
Wei
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
