Linux Virtualization - Oct 2017 - [PATCH v1 2/3] virtio-balloon: deflate up to oom_pages on OOM

On Fri, Oct 20, 2017 at 07:54:25PM +0800, Wei Wang
wrote:
> The current implementation only deflates 256 pages even when a user
> specifies more than that via the oom_pages module param. This patch
> enables the deflating of up to oom_pages pages if there are enough
> inflated pages.
> 
> Signed-off-by: Wei Wang <wei.w.wang at intel.com>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: Michal Hocko <mhocko at kernel.org>
> Cc: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
This seems reasonable. Does this by itself help?

> ---
>  drivers/virtio/virtio_balloon.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
> index 1ecd15a..ab55cf8 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -43,8 +43,8 @@
>  #define OOM_VBALLOON_DEFAULT_PAGES 256
>  #define VIRTBALLOON_OOM_NOTIFY_PRIORITY 80
>  
> -static int oom_pages = OOM_VBALLOON_DEFAULT_PAGES;
> -module_param(oom_pages, int, S_IRUSR | S_IWUSR);
> +static unsigned int oom_pages = OOM_VBALLOON_DEFAULT_PAGES;
> +module_param(oom_pages, uint, 0600);
>  MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
>  
>  #ifdef CONFIG_BALLOON_COMPACTION
> @@ -359,16 +359,20 @@ static int virtballoon_oom_notify(struct
notifier_block *self,
>  {
>  	struct virtio_balloon *vb;
>  	unsigned long *freed;
> -	unsigned num_freed_pages;
> +	unsigned int npages = oom_pages;
>  
>  	vb = container_of(self, struct virtio_balloon, nb);
>  	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
>  		return NOTIFY_OK;
>  
>  	freed = parm;
> -	num_freed_pages = leak_balloon(vb, oom_pages);
> +
> +	/* Don't deflate more than the number of inflated pages */
> +	while (npages && atomic64_read(&vb->num_pages))
> +		npages -= leak_balloon(vb, npages);
> +
>  	update_balloon_size(vb);
> -	*freed += num_freed_pages;
> +	*freed += oom_pages - npages;
>  
>  	return NOTIFY_OK;
>  }
> -- 
> 2.7.4

Michael S. Tsirkin wrote:
> On Fri, Oct 20, 2017 at 07:54:25PM +0800, Wei Wang wrote:
> > The current implementation only deflates 256 pages even when a user
> > specifies more than that via the oom_pages module param. This patch
> > enables the deflating of up to oom_pages pages if there are enough
> > inflated pages.
> 
> This seems reasonable. Does this by itself help?
At least
> > -	num_freed_pages = leak_balloon(vb, oom_pages);
> > +
> > +	/* Don't deflate more than the number of inflated pages */
> > +	while (npages && atomic64_read(&vb->num_pages))
> > +		npages -= leak_balloon(vb, npages);
don't we need to abort if leak_balloon() returned 0 for some reason?

On 10/22/2017 12:11 PM, Tetsuo Handa wrote:
> Michael S. Tsirkin wrote:
>>> -	num_freed_pages = leak_balloon(vb, oom_pages);
>>> +
>>> +	/* Don't deflate more than the number of inflated pages */
>>> +	while (npages && atomic64_read(&vb->num_pages))
>>> +		npages -= leak_balloon(vb, npages);
> don't we need to abort if leak_balloon() returned 0 for some reason?
I don't think so. Returning 0 should be a normal case when the host tries
to give back some pages to the guest, but there is no pages that have ever
been inflated. For example, right after booting the guest, the host sends a
deflating request to give the guest 1G memory, leak_balloon should return 0,
and guest wouldn't get 1 more G memory.


Best,
Wei