On Tue, Sep 28, 2010 at 10:03 PM, Anthony Liguori <anthony@xxxxxxxxxxxxx> wrote:
> On 09/28/2010 08:49 AM, Dave Young wrote:
>>
>> On Tue, Sep 28, 2010 at 9:34 PM, Anthony Liguori<anthony@xxxxxxxxxxxxx>
>> Âwrote:
>>
>>>
>>> On 09/28/2010 08:19 AM, Dave Young wrote:
>>>
>>>>
>>>> Balloon could cause guest memory oom killing and panic. If we disable
>>>> the
>>>> oom killer it will be better at least avoid guest panic.
>>>>
>>>> If alloc failed we can just adjust the balloon target to be equal to
>>>> current number by call vdev->config->set
>>>>
>>>> But during test I found the config->set num_pages does not change the
>>>> config actually, Should I do hacks in userspace as well? If so where
>>>> should
>>>> I start to hack?
>>>>
>>>>
>>>
>>>
>>
>> Hi,
>>
>> Thanks your comments.
>>
>>
>>>
>>> The guest is not supposed to set the target field in it's config. ÂThis
>>> is a
>>> host read/write, guest read-only field.
>>>
>>
>> Could you tell where to set it? If so, IMHO set config api should
>> fail, isn't it?
>>
>>
>>>
>>> The problem with your approach generally speaking is that it's unclear
>>> whether this is the right policy. ÂFor instance, what if another
>>> application
>>> held a very large allocation which caused the fill request to stop but
>>> then
>>> shortly afterwards, the aforementioned application released that
>>> allocation.
>>> ÂIf instead of just stopping, we paused and tried again later, we could
>>> potentially succeed.
>>>
>>
>> Yes, it is possible. But maybe better to do balloon from qemu monitor
>> later?
>>
>
> It's part of the specification, not something that's enforced or even
> visible within the APIs.
>
>>> I think a better approach might be a graceful back off. ÂFor instance,
>>> when
>>> you hit this condition, deflate the balloon by 10% based on the
>>> assumption
>>> that you probably already gone too far. ÂBefore you attempt to allocate
>>> to
>>> the target again, set a timeout that increases in duration exponentially
>>> until you reach some maximum (say 10s).
>>>
>>
>> I'm afraid most times it will keep doing inflate/deflate circularly.
>>
>
> With sufficiently large timeouts, does it matter?
>
> The other side of the argument is that the host should be more careful about
> doing balloon requests to the guest. ÂAlternatively, you can argue that the
> guest should be able to balloon itself and that's where the logic should be.
>
> But I think split policy across the guest and host would prove to be
> prohibitively complex to deal with.
Thanks.
What do you think about add an option like:
-balloon virtio,nofail
With nofail option we stop balloon if oom
The default behavior without nofail will be as your said, ie. retry
every 5 minutes
>
> Regards,
>
> Anthony Liguori
>
>>
>>>
>>> This isn't the only approach, but hopefully it conveys the idea of
>>> gracefully backing off without really giving up.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>
>>>>
>>>> Thanks
>>>>
>>>> --- linux-2.6.orig/drivers/virtio/virtio_balloon.c   Â2010-09-25
>>>> 20:58:14.190000001 +0800
>>>> +++ linux-2.6/drivers/virtio/virtio_balloon.c  2010-09-28
>>>> 21:05:42.203333675 +0800
>>>> @@ -25,6 +25,7 @@
>>>> Â#include<linux/freezer.h>
>>>> Â#include<linux/delay.h>
>>>> Â#include<linux/slab.h>
>>>> +#include<linux/oom.h>
>>>>
>>>> Âstruct virtio_balloon
>>>> Â{
>>>> @@ -97,8 +98,22 @@ static void tell_host(struct virtio_ball
>>>> Â Â Â Âwait_for_completion(&vb->acked);
>>>> Â}
>>>>
>>>> -static void fill_balloon(struct virtio_balloon *vb, size_t num)
>>>> +static int cblimit(int times)
>>>> Â{
>>>> + Â Â Â static int t;
>>>> +
>>>> + Â Â Â if (t< Â Âtimes)
>>>> + Â Â Â Â Â Â Â t++;
>>>> + Â Â Â else
>>>> + Â Â Â Â Â Â Â t = 0;
>>>> +
>>>> + Â Â Â return !t;
>>>> +}
>>>> +
>>>> +static int fill_balloon(struct virtio_balloon *vb, size_t num)
>>>> +{
>>>> + Â Â Â int ret = 0;
>>>> +
>>>> Â Â Â Â/* We can only do one array worth at a time. */
>>>> Â Â Â Ânum = min(num, ARRAY_SIZE(vb->pfns));
>>>>
>>>> @@ -106,10 +121,13 @@ static void fill_balloon(struct virtio_b
>>>> Â Â Â Â Â Â Â Âstruct page *page = alloc_page(GFP_HIGHUSER |
>>>> __GFP_NORETRY
>>>> |
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â__GFP_NOMEMALLOC | __GFP_NOWARN);
>>>> Â Â Â Â Â Â Â Âif (!page) {
>>>> - Â Â Â Â Â Â Â Â Â Â Â if (printk_ratelimit())
>>>> + Â Â Â Â Â Â Â Â Â Â Â if (cblimit(5)) {
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âdev_printk(KERN_INFO,&vb->vdev->dev,
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "Out of puff! Can't get %zu
>>>> pages\n",
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â num);
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ret = -ENOMEM;
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â goto out;
>>>> + Â Â Â Â Â Â Â Â Â Â Â }
>>>> Â Â Â Â Â Â Â Â Â Â Â Â/* Sleep for at least 1/5 of a second before
>>>> retry.
>>>> */
>>>> Â Â Â Â Â Â Â Â Â Â Â Âmsleep(200);
>>>> Â Â Â Â Â Â Â Â Â Â Â Âbreak;
>>>> @@ -120,11 +138,11 @@ static void fill_balloon(struct virtio_b
>>>> Â Â Â Â Â Â Â Âlist_add(&page->lru,&vb->pages);
>>>> Â Â Â Â}
>>>>
>>>> - Â Â Â /* Didn't get any? ÂOh well. */
>>>> - Â Â Â if (vb->num_pfns == 0)
>>>> - Â Â Â Â Â Â Â return;
>>>> +out:
>>>> + Â Â Â if (vb->num_pfns)
>>>> + Â Â Â Â Â Â Â tell_host(vb, vb->inflate_vq);
>>>>
>>>> - Â Â Â tell_host(vb, vb->inflate_vq);
>>>> + Â Â Â return ret;
>>>> Â}
>>>>
>>>> Âstatic void release_pages_by_pfn(const u32 pfns[], unsigned int num)
>>>> @@ -251,6 +269,14 @@ static void update_balloon_size(struct v
>>>> Â Â Â Â Â Â Â Â Â Â Â Â&actual, sizeof(actual));
>>>> Â}
>>>>
>>>> +static void update_balloon_target(struct virtio_balloon *vb)
>>>> +{
>>>> + Â Â Â __le32 num_pages = cpu_to_le32(vb->num_pages);
>>>> + Â Â Â vb->vdev->config->set(vb->vdev,
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â offsetof(struct virtio_balloon_config,
>>>> num_pages),
>>>> +&num_pages, sizeof(num_pages));
>>>> +}
>>>> +
>>>> Âstatic int balloon(void *_vballoon)
>>>> Â{
>>>> Â Â Â Âstruct virtio_balloon *vb = _vballoon;
>>>> @@ -267,9 +293,14 @@ static int balloon(void *_vballoon)
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â || freezing(current));
>>>> Â Â Â Â Â Â Â Âif (vb->need_stats_update)
>>>> Â Â Â Â Â Â Â Â Â Â Â Âstats_handle_request(vb);
>>>> - Â Â Â Â Â Â Â if (diff> Â Â0)
>>>> - Â Â Â Â Â Â Â Â Â Â Â fill_balloon(vb, diff);
>>>> - Â Â Â Â Â Â Â else if (diff< Â Â0)
>>>> + Â Â Â Â Â Â Â if (diff> Â Â0) {
>>>> + Â Â Â Â Â Â Â Â Â Â Â int oom;
>>>> + Â Â Â Â Â Â Â Â Â Â Â oom_killer_disable();
>>>> + Â Â Â Â Â Â Â Â Â Â Â oom = fill_balloon(vb, diff);
>>>> + Â Â Â Â Â Â Â Â Â Â Â oom_killer_enable();
>>>> + Â Â Â Â Â Â Â Â Â Â Â if (oom)
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â update_balloon_target(vb);
>>>> + Â Â Â Â Â Â Â } else if (diff< Â Â0)
>>>> Â Â Â Â Â Â Â Â Â Â Â Âleak_balloon(vb, -diff);
>>>> Â Â Â Â Â Â Â Âupdate_balloon_size(vb);
>>>> Â Â Â Â}
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
--
Regards
dave
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
