From: Alexander Atanasov <alexander.atanasov@xxxxxxxxxxxxx> Sent: Tuesday, July 5, 2022 2:44 AM
>
> Allow the guest to know how much it is ballooned by the host.
> It is useful when debugging out of memory conditions.
>
> When host gets back memory from the guest it is accounted
> as used memory in the guest but the guest have no way to know
> how much it is actually ballooned.
>
> Expose current state, flags and max possible memory to the guest.
Thanks for the contribution! I can see it being useful. But I'd note
that the existing code has a trace function that outputs pretty much all
the same information when it is reported to the Hyper-V host in
post_status() every 1 second. However, the debugfs interface might be
a bit easier to use for ongoing sampling. Related, I see that the VMware
balloon driver use the debugfs interface, but no tracing. The virtio
balloon driver has neither. I'm not against adding this debugfs interface,
but I wanted to make sure there's real value over the existing tracing.
See also some minor comments below.
Michael
> While at it - fix a 10+ years old typo.
>
> Signed-off-by: Alexander Atanasov <alexander.atanasov@xxxxxxxxxxxxx>
> ---
> drivers/hv/hv_balloon.c | 127 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 126 insertions(+), 1 deletion(-)
>
>
> Note - no attempt to handle guest vs host page size difference
> is made - see ballooning_enabled.
> Basicly if balloon page size != guest page size balloon is off.
>
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 91e8a72eee14..b7b87d168d46 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -11,6 +11,7 @@
> #include <linux/kernel.h>
> #include <linux/jiffies.h>
> #include <linux/mman.h>
> +#include <linux/debugfs.h>
> #include <linux/delay.h>
> #include <linux/init.h>
> #include <linux/module.h>
> @@ -248,7 +249,7 @@ struct dm_capabilities_resp_msg {
> * num_committed: Committed memory in pages.
> * page_file_size: The accumulated size of all page files
> * in the system in pages.
> - * zero_free: The nunber of zero and free pages.
> + * zero_free: The number of zero and free pages.
> * page_file_writes: The writes to the page file in pages.
> * io_diff: An indicator of file cache efficiency or page file activity,
> * calculated as File Cache Page Fault Count - Page Read Count.
> @@ -567,6 +568,14 @@ struct hv_dynmem_device {
> __u32 version;
>
> struct page_reporting_dev_info pr_dev_info;
> +
> +#ifdef CONFIG_DEBUG_FS
> + /*
> + * Maximum number of pages that can be hot_add-ed
> + */
> + __u64 max_dynamic_page_count;
> +#endif
> +
> };
>
> static struct hv_dynmem_device dm_device;
> @@ -1078,6 +1087,9 @@ static void process_info(struct hv_dynmem_device *dm,
> struct dm_info_msg *msg)
>
> pr_info("Max. dynamic memory size: %llu MB\n",
> (*max_page_count) >> (20 - HV_HYP_PAGE_SHIFT));
> +#ifdef CONFIG_DEBUG_FS
> + dm->max_dynamic_page_count = *max_page_count;
> +#endif
Arguably, you could drop the #ifdef's in the above two places, to reduce the code
clutter. The extra memory and CPU overhead of always saving max_dynamic_page_count
is negligible. What I don't know for sure is if the compiler or other static checking
tools will complain about a field being set but not used.
> }
>
> break;
> @@ -1807,6 +1819,115 @@ static int balloon_connect_vsp(struct hv_device *dev)
> return ret;
> }
>
> +/*
> + * DEBUGFS Interface
> + */
> +#ifdef CONFIG_DEBUG_FS
> +
> +/**
> + * virtio_balloon_debug_show - shows statistics of balloon operations.
> + * @f: pointer to the &struct seq_file.
> + * @offset: ignored.
> + *
> + * Provides the statistics that can be accessed in virtio-balloon in the debugfs.
> + *
> + * Return: zero on success or an error code.
> + */
> +static int hv_balloon_debug_show(struct seq_file *f, void *offset)
> +{
> + struct hv_dynmem_device *dm = f->private;
> + unsigned long num_pages_committed;
> + char *sname;
> +
> + seq_printf(f, "%-22s: %u.%u\n", "host_version",
> + DYNMEM_MAJOR_VERSION(dm->version),
> + DYNMEM_MINOR_VERSION(dm->version));
> +
> + seq_printf(f, "%-22s:", "capabilities");
> + if (ballooning_enabled())
> + seq_puts(f, " enabled");
> +
> + if (hot_add_enabled())
> + seq_puts(f, " hot_add");
> +
> + seq_puts(f, "\n");
> +
> + seq_printf(f, "%-22s: %u", "state", dm->state);
> + switch (dm->state) {
> + case DM_INITIALIZING:
> + sname = "Initializing";
> + break;
> + case DM_INITIALIZED:
> + sname = "Initialized";
> + break;
> + case DM_BALLOON_UP:
> + sname = "Balloon Up";
> + break;
> + case DM_BALLOON_DOWN:
> + sname = "Balloon Down";
> + break;
> + case DM_HOT_ADD:
> + sname = "Hot Add";
> + break;
> + case DM_INIT_ERROR:
> + sname = "Error";
> + break;
> + default:
> + sname = "Unknown";
> + }
> + seq_printf(f, " (%s)\n", sname);
> +
> + /* HV Page Size */
> + seq_printf(f, "%-22s: %ld\n", "page_size", HV_HYP_PAGE_SIZE);
> +
> + /* Pages added with hot_add */
> + seq_printf(f, "%-22s: %u\n", "pages_added", dm->num_pages_added);
> +
> + /* pages that are "onlined"/used from pages_added */
> + seq_printf(f, "%-22s: %u\n", "pages_onlined", dm->num_pages_onlined);
> +
> + /* pages we have given back to host */
> + seq_printf(f, "%-22s: %u\n", "pages_ballooned", dm->num_pages_ballooned);
> +
> + num_pages_committed = vm_memory_committed();
> + num_pages_committed += dm->num_pages_ballooned +
> + (dm->num_pages_added > dm->num_pages_onlined ?
> + dm->num_pages_added - dm->num_pages_onlined : 0) +
> + compute_balloon_floor();
Duplicating this calculation that also appears in post_status() is not ideal. Maybe
post_status() should store the value in a field in the struct hv_dynmem_device, and
this function would just output the field. Again, there's the potential for a code
checker to complain about a field being written but not read. Alternatively,
the calculation could go into a helper function that is called here and in
post_status(). I'm not sure if it is more useful to report the last value that
was reported by the Hyper-V host, or the currently calculated value. There is a
trace point that records the values reported to the host, so maybe the latter
is more useful here.
> + seq_printf(f, "%-22s: %lu\n", "total_pages_commited",
> + num_pages_committed);
> +
> + seq_printf(f, "%-22s: %llu\n", "max_dynamic_page_count",
> + dm->max_dynamic_page_count);
> +
> + return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(hv_balloon_debug);
> +
> +static void hv_balloon_debugfs_init(struct hv_dynmem_device *b)
> +{
> + debugfs_create_file("hv-balloon", 0444, NULL, b,
> + &hv_balloon_debug_fops);
> +}
> +
> +static void hv_balloon_debugfs_exit(struct hv_dynmem_device *b)
> +{
> + debugfs_remove(debugfs_lookup("hv-balloon", NULL));
> +}
> +
> +#else
> +
> +static inline void hv_balloon_debugfs_init(struct hv_dynmem_device *b)
> +{
> +}
> +
> +static inline void hv_balloon_debugfs_exit(struct hv_dynmem_device *b)
> +{
> +}
> +
> +#endif /* CONFIG_DEBUG_FS */
> +
> static int balloon_probe(struct hv_device *dev,
> const struct hv_vmbus_device_id *dev_id)
> {
> @@ -1854,6 +1975,8 @@ static int balloon_probe(struct hv_device *dev,
> goto probe_error;
> }
>
> + hv_balloon_debugfs_init(&dm_device);
> +
> return 0;
>
> probe_error:
> @@ -1879,6 +2002,8 @@ static int balloon_remove(struct hv_device *dev)
> if (dm->num_pages_ballooned != 0)
> pr_warn("Ballooned pages: %d\n", dm->num_pages_ballooned);
>
> + hv_balloon_debugfs_exit(dm);
> +
> cancel_work_sync(&dm->balloon_wrk.wrk);
> cancel_work_sync(&dm->ha_wrk.wrk);
>
> --
> 2.25.1
