Re: [PATCH] Virtio-balloon: add user space API for sizing

Re: [PATCH] Virtio-balloon: add user space API for sizing

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4436 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20220214195908.4070138-1-kalutes@google.com>
References: <20220214195908.4070138-1-kalutes@google.com>
TO: Kameron Lutes <kalutes@google.com>
TO: "Michael S . Tsirkin" <mst@redhat.com>
TO: David Hildenbrand <david@redhat.com>
TO: Jason Wang <jasowang@redhat.com>
TO: virtualization(a)lists.linux-foundation.org
TO: linux-kernel(a)vger.kernel.org
TO: virtio-dev(a)lists.oasis-open.org
TO: kvm(a)vger.kernel.org
CC: Suleiman Souhlal <suleiman@chromium.org>
CC: Charles William Dick <cwd@google.com>
CC: Kameron Lutes <kalutes@google.com>

Hi Kameron,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.17-rc4 next-20220214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kameron-Lutes/Virtio-balloon-add-user-space-API-for-sizing/20220215-050844
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git d567f5db412ed52de0b3b3efca4a451263de6108
:::::: branch date: 5 hours ago
:::::: commit date: 5 hours ago
config: arm-randconfig-s031-20220214 (https://download.01.org/0day-ci/archive/20220215/202202151007.9aaQbvQc-lkp(a)intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/1ad8c1ce7ded6001b74fd4b7cc40957fb5bf05e6
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kameron-Lutes/Virtio-balloon-add-user-space-API-for-sizing/20220215-050844
        git checkout 1ad8c1ce7ded6001b74fd4b7cc40957fb5bf05e6
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm SHELL=/bin/bash drivers/virtio/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/virtio/virtio_balloon.c:892:9: sparse: sparse: no generic selection for 'restricted __le32 virtio_cread_v'
>> drivers/virtio/virtio_balloon.c:892:9: sparse: sparse: incompatible types in comparison expression (different base types):
>> drivers/virtio/virtio_balloon.c:892:9: sparse:    bad type *
   drivers/virtio/virtio_balloon.c:892:9: sparse:    unsigned int *
>> drivers/virtio/virtio_balloon.c:892:9: sparse: sparse: no generic selection for 'restricted __le32 [addressable] virtio_cread_v'

vim +892 drivers/virtio/virtio_balloon.c

71994620bb25a8 Wei Wang      2018-08-16  880  
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  881  static ssize_t balloon_size_show(struct device *dev,
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  882  				 struct device_attribute *attr, char *buf)
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  883  {
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  884  	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  885  	u32 num_pages;
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  886  
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  887  	/*
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  888  	 * Read the size directly from the balloon's configuration.
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  889  	 * The caller expects the balloon size enforced by the host,
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  890  	 * not the actual balloon size
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  891  	 */
1ad8c1ce7ded60 Kameron Lutes 2022-02-14 @892  	virtio_cread(vdev, struct virtio_balloon_config, num_pages,
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  893  		     &num_pages);
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  894  
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  895  	return sprintf(buf, "0x%x", num_pages);
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  896  }
1ad8c1ce7ded60 Kameron Lutes 2022-02-14  897  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH] Virtio-balloon: add user space API for sizing

[David Hildenbrand]

On 14.02.22 20:59, Kameron Lutes wrote:
> This new linux API will allow user space applications to directly
> control the size of the virtio-balloon. This is useful in
> situations where the guest must quickly respond to drastically
> increased memory pressure and cannot wait for the host to adjust
> the balloon's size.
> 
> Under the current wording of the Virtio spec, guest driven
> behavior such as this is permitted:
> 
> VIRTIO Version 1.1 Section 5.5.6
> "The device is driven either by the receipt of a configuration
> change notification, or by changing guest memory needs, such as
> performing memory compaction or responding to out of memory
> conditions."

Not quite. num_pages is determined by the hypervisor only and the guest
is not expected to change it, and if it does, it's ignored.

5.5.6 does not indicate at all that the guest may change it or that it
would have any effect. num_pages is examined only, actual is updated by
the driver.

5.5.6.1 documents what's allowed, e.g.,

  The driver SHOULD supply pages to the balloon when num_pages is
  greater than the actual number of pages in the balloon.

  The driver MAY use pages from the balloon when num_pages is less than
  the actual number of pages in the balloon.

and special handling for VIRTIO_BALLOON_F_DEFLATE_ON_OOM.

Especially, we have

  The driver MUST update actual after changing the number of pages in
  the balloon.

  The driver MAY update actual once after multiple inflate and deflate
  operations.

That's also why QEMU never syncs back the num_pages value from the guest
when writing the config.


Current spec does not allow for what you propose.


> 
> The intended use case for this API is one where the host
> communicates a deflation limit to the guest. The guest may then
> choose to respond to memory pressure by deflating its balloon down
> to the guest's allowable limit.

It would be good to have a full proposal and a proper spec update. I'd
assume you'd want separate values for soft vs. hard num_values -- if
that's what we really want.

BUT

There seems to be recent interest in handling memory pressure in a
better way (although how to really detect "serious memory pressure" vs
"ordinary reclaim" in Linux is still to be figured out). There is
already a discussion going on how that could happen. Adding random user
space toggles might not be the best idea. We might want a single
mechanism to achieve that.

https://lists.oasis-open.org/archives/virtio-comment/202201/msg00139.html

-- 
Thanks,

David / dhildenb

Re: [PATCH] Virtio-balloon: add user space API for sizing

[David Hildenbrand]

On 14.02.22 20:59, Kameron Lutes wrote:
> This new linux API will allow user space applications to directly
> control the size of the virtio-balloon. This is useful in
> situations where the guest must quickly respond to drastically
> increased memory pressure and cannot wait for the host to adjust
> the balloon's size.
> 
> Under the current wording of the Virtio spec, guest driven
> behavior such as this is permitted:
> 
> VIRTIO Version 1.1 Section 5.5.6
> "The device is driven either by the receipt of a configuration
> change notification, or by changing guest memory needs, such as
> performing memory compaction or responding to out of memory
> conditions."

Not quite. num_pages is determined by the hypervisor only and the guest
is not expected to change it, and if it does, it's ignored.

5.5.6 does not indicate at all that the guest may change it or that it
would have any effect. num_pages is examined only, actual is updated by
the driver.

5.5.6.1 documents what's allowed, e.g.,

  The driver SHOULD supply pages to the balloon when num_pages is
  greater than the actual number of pages in the balloon.

  The driver MAY use pages from the balloon when num_pages is less than
  the actual number of pages in the balloon.

and special handling for VIRTIO_BALLOON_F_DEFLATE_ON_OOM.

Especially, we have

  The driver MUST update actual after changing the number of pages in
  the balloon.

  The driver MAY update actual once after multiple inflate and deflate
  operations.

That's also why QEMU never syncs back the num_pages value from the guest
when writing the config.


Current spec does not allow for what you propose.


> 
> The intended use case for this API is one where the host
> communicates a deflation limit to the guest. The guest may then
> choose to respond to memory pressure by deflating its balloon down
> to the guest's allowable limit.

It would be good to have a full proposal and a proper spec update. I'd
assume you'd want separate values for soft vs. hard num_values -- if
that's what we really want.

BUT

There seems to be recent interest in handling memory pressure in a
better way (although how to really detect "serious memory pressure" vs
"ordinary reclaim" in Linux is still to be figured out). There is
already a discussion going on how that could happen. Adding random user
space toggles might not be the best idea. We might want a single
mechanism to achieve that.

https://lists.oasis-open.org/archives/virtio-comment/202201/msg00139.html

-- 
Thanks,

David / dhildenb

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] Virtio-balloon: add user space API for sizing

[Kameron Lutes]

This new linux API will allow user space applications to directly
control the size of the virtio-balloon. This is useful in
situations where the guest must quickly respond to drastically
increased memory pressure and cannot wait for the host to adjust
the balloon's size.

Under the current wording of the Virtio spec, guest driven
behavior such as this is permitted:

VIRTIO Version 1.1 Section 5.5.6
"The device is driven either by the receipt of a configuration
change notification, or by changing guest memory needs, such as
performing memory compaction or responding to out of memory
conditions."

The intended use case for this API is one where the host
communicates a deflation limit to the guest. The guest may then
choose to respond to memory pressure by deflating its balloon down
to the guest's allowable limit.

Signed-off-by: Kameron Lutes <kalutes@google.com>
---
 drivers/virtio/virtio_balloon.c | 55 +++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f4c34a2a6b8e..aa06305a3137 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -878,6 +878,54 @@ static int virtio_balloon_register_shrinker(struct virtio_balloon *vb)
 	return register_shrinker(&vb->shrinker);
 }
 
+static ssize_t balloon_size_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
+	u32 num_pages;
+
+	/*
+	 * Read the size directly from the balloon's configuration.
+	 * The caller expects the balloon size enforced by the host,
+	 * not the actual balloon size
+	 */
+	virtio_cread(vdev, struct virtio_balloon_config, num_pages,
+		     &num_pages);
+
+	return sprintf(buf, "0x%x", num_pages);
+}
+
+static ssize_t balloon_size_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
+	u32 num_pages;
+	int err;
+
+	err = kstrtou32(buf, 0, &num_pages);
+
+	if (err < 0) {
+		dev_err(dev, "Failed to read balloon size from file\n");
+		return err;
+	}
+
+	/*
+	 * Write num_pages back to the balloon's config section
+	 */
+	virtio_cwrite_le(vdev, struct virtio_balloon_config, num_pages,
+		      &num_pages);
+
+	/*
+	 * Signal to the balloon that the configuration has changed.
+	 * This triggers any necessary resizing actions
+	 */
+	virtballoon_changed(vdev);
+
+	return count;
+}
+static DEVICE_ATTR_RW(balloon_size);
+
 static int virtballoon_probe(struct virtio_device *vdev)
 {
 	struct virtio_balloon *vb;
@@ -1015,12 +1063,19 @@ static int virtballoon_probe(struct virtio_device *vdev)
 			goto out_unregister_oom;
 	}
 
+	err = device_create_file(&vb->vdev->dev, &dev_attr_balloon_size);
+	if (err)
+		goto out_unregister_page_reporting;
+
 	virtio_device_ready(vdev);
 
 	if (towards_target(vb))
 		virtballoon_changed(vdev);
 	return 0;
 
+out_unregister_page_reporting:
+	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING))
+		page_reporting_unregister(&vb->pr_dev_info);
 out_unregister_oom:
 	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
 		unregister_oom_notifier(&vb->oom_nb);
-- 
2.35.1.265.g69c8d7142f-goog