Re: [PATCH v20 4/7] virtio-balloon: VIRTIO_BALLOON_F_SG

From: Wei Wang
Date: Sun Dec 24 2017 - 02:40:08 EST

On 12/24/2017 12:45 PM, Tetsuo Handa wrote:
Matthew Wilcox wrote:
+ unsigned long pfn = page_to_pfn(page);
+ int ret;
+ *pfn_min = min(pfn, *pfn_min);
+ *pfn_max = max(pfn, *pfn_max);
+ do {
+ if (xb_preload(GFP_NOWAIT | __GFP_NOWARN) < 0)
+ return -ENOMEM;
+ ret = xb_set_bit(&vb->page_xb, pfn);
+ xb_preload_end();
+ } while (unlikely(ret == -EAGAIN));
OK, so you don't need a spinlock because you're under a mutex? But you
can't allocate memory because you're in the balloon driver, and so a
GFP_KERNEL allocation might recurse into your driver?
Right. We can't (directly or indirectly) depend on __GFP_DIRECT_RECLAIM && !__GFP_NORETRY
allocations because the balloon driver needs to handle OOM notifier callback.

do the job? I'm a little hazy on exactly how the balloon driver works.
GFP_NOIO implies __GFP_DIRECT_RECLAIM. In the worst case, it can lockup due to
the too small to fail memory allocation rule. GFP_NOIO | __GFP_NORETRY would work
if there is really a guarantee that GFP_NOIO | __GFP_NORETRY never depend on
__GFP_DIRECT_RECLAIM && !__GFP_NORETRY allocations, which is too subtle for me to
validate. The direct reclaim dependency is too complicated to validate.
I consider that !__GFP_DIRECT_RECLAIM is the future-safe choice.

What's the problem with (or how is it better than) the "GFP_NOWAIT | __GFP_NOWARN" we are using here?

If you can't preload with anything better than that, I think that
xb_set_bit() should attempt an allocation with GFP_NOWAIT | __GFP_NOWARN,
and then you can skip the preload; it has no value for you.
Yes, that's why I suggest directly using kzalloc() at xb_set_bit().

It has some possibilities to remove that preload if we also do the bitmap allocation in the xb_set_bit():

bitmap = rcu_dereference_raw(*slot);
if (!bitmap) {
bitmap = this_cpu_xchg(ida_bitmap, NULL);
if (!bitmap) {
bitmap = kmalloc(sizeof(*bitmap), gfp);
if (!bitmap)
return -ENOMEM;

But why not just follow the radix tree implementation style that puts the allocation in preload, which would be invoked with a more relaxed gfp in other use cases?
Its usage in virtio_balloon is just a little special that we need to put the allocation within the balloon_lock, which doesn't give us the benefit of using a relaxed gfp in preload, but it doesn't prevent us from living with the current APIs (i.e. the preload + xb_set pattern).
On the other side, if we do it as above, we have more things that need to consider. For example, what if the a use case just want the radix tree implementation style, which means it doesn't want allocation within xb_set(), then would we be troubled with how to avoid the allocation path in that case?

So, I think it is better to stick with the convention by putting the allocation in preload. Breaking the convention should show obvious advantages, IMHO.

@@ -173,8 +292,15 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
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);
+ }
Is this the right behaviour?
I don't think so. In the worst case, we can set no bit using xb_set_page().

If we can't record the page in the xb,
wouldn't we rather send it across as a single page?

I think that we need to be able to fallback to !use_sg path when OOM.

I also have different thoughts:

1) For OOM, we have leak_balloon_sg_oom (oom has nothing to do with fill_balloon), which does not use xbitmap to record pages, thus no memory allocation.

2) If the memory is already under pressure, it is pointless to continue inflating memory to the host. We need to give thanks to the memory allocation failure reported by xbitmap, which gets us a chance to release the inflated pages that have been demonstrated to cause the memory pressure of the guest.