linux-kernel - Re: [PATCH v6 2/2] Unify how inflated memory is accounted in virtio balloon driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220809063624-mutt-send-email-mst@kernel.org>
Date:   Tue, 9 Aug 2022 06:42:34 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Alexander Atanasov <alexander.atanasov@...tuozzo.com>
Cc:     David Hildenbrand <david@...hat.com>,
        Jason Wang <jasowang@...hat.com>, kernel@...nvz.org,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/2] Unify how inflated memory is accounted in virtio
 balloon driver

On Tue, Jul 26, 2022 at 02:10:47PM +0000, Alexander Atanasov wrote:
> Always account inflated memory as used for both cases - with and
> without deflate on oom. Do not change total ram which can confuse
> userspace and users.
> 
> Signed-off-by: Alexander Atanasov <alexander.atanasov@...tuozzo.com>

I only noticed this patch accidentally since it looked like
part of discussion on an older patch.
Please do not do this, each version should be its own thread,
if you want to link to previous discussion provide a
cover letter and do it there.

> ---
>  drivers/virtio/virtio_balloon.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 97d3b29cb9f1..fa6ddec45fc4 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -244,9 +244,6 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
>  
>  		set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
>  		vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> -		if (!virtio_has_feature(vb->vdev,
> -					VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> -			adjust_managed_page_count(page, -1);
>  		vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
>  	}
>  
> @@ -265,9 +262,6 @@ static void release_pages_balloon(struct virtio_balloon *vb,
>  	struct page *page, *next;
>  
>  	list_for_each_entry_safe(page, next, pages, lru) {
> -		if (!virtio_has_feature(vb->vdev,
> -					VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> -			adjust_managed_page_count(page, 1);
>  		list_del(&page->lru);
>  		put_page(page); /* balloon reference */
>  	}

We adjusted total ram with balloon usage for many years.  I don't think
we can just drop this accounting using "can confuse userspace" as a
justification - any userspace that's confused by this has been confused
since approximately forever.

> @@ -750,12 +744,9 @@ static void report_free_page_func(struct work_struct *work)
>  static int virtio_balloon_debug_show(struct seq_file *f, void *offset)
>  {
>  	struct virtio_balloon *vb = f->private;
> -	s64 num_pages = vb->num_pages << (VIRTIO_BALLOON_PFN_SHIFT - 10);
> +	u64 num_pages = vb->num_pages << (VIRTIO_BALLOON_PFN_SHIFT - 10);
>  
> -	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> -		num_pages = -num_pages;
> -
> -	seq_printf(f, "inflated: %lld kB\n", num_pages);
> +	seq_printf(f, "inflated: %llu kB\n", num_pages);
>  
>  	return 0;

I don't much like it when patch 1 adds code only for patch 2 to drop it.

>  }
> -- 
> 2.25.1