Balloon header includes:
 - feature bit for Working Set Reporting
 - number of Working Set bins member in balloon config
 - types for communicating Working Set information

Signed-off-by: T.J. Alumbaugh <talumbau@google.com>
---
 .../standard-headers/linux/virtio_balloon.h   | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h
index f343bfefd8..df61eaceee 100644
--- a/include/standard-headers/linux/virtio_balloon.h
+++ b/include/standard-headers/linux/virtio_balloon.h
@@ -37,6 +37,7 @@
 #define VIRTIO_BALLOON_F_FREE_PAGE_HINT	3 /* VQ to report free pages */
 #define VIRTIO_BALLOON_F_PAGE_POISON	4 /* Guest is using page poisoning */
 #define VIRTIO_BALLOON_F_REPORTING	5 /* Page reporting virtqueue */
+#define VIRTIO_BALLOON_F_WS_REPORTING	6 /* Working set report virtqueues */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12
@@ -59,6 +60,9 @@ struct virtio_balloon_config {
 	};
 	/* Stores PAGE_POISON if page poisoning is in use */
 	uint32_t poison_val;
+	/* Stores the number of histogram bins if WS reporting in use */
+	uint8_t working_set_num_bins;
+	uint8_t padding[3];
 };
 
 #define VIRTIO_BALLOON_S_SWAP_IN  0   /* Amount of memory swapped in */
@@ -116,4 +120,20 @@ struct virtio_balloon_stat {
 	__virtio64 val;
 } QEMU_PACKED;
 
+enum virtio_balloon_working_set_op {
+    VIRTIO_BALLOON_WS_REQUEST = 1, /* a Working Set request from the host */
+    VIRTIO_BALLOON_WS_CONFIG = 2,  /* a Working Set config from the host */
+};
+
+struct virtio_balloon_working_set {
+	/* A tag for additional metadata */
+	__virtio16 tag;
+	/* The NUMA node for this report. */
+	__virtio16 node_id;
+	uint8_t reserved[4];
+	__virtio64 idle_age_ms;
+	/* A bin each for anonymous and file-backed memory. */
+	__virtio64 memory_size_bytes[2];
+} QEMU_PACKED;
+
 #endif /* _LINUX_VIRTIO_BALLOON_H */
-- 
2.41.0.rc0.172.g3f132b7071-goog

On 26.05.23 00:20, T.J. Alumbaugh wrote:

Hi,

please try writing a comprehensive patch description: the goal should be 
that one can understand what's happening in the single patch without all 
of the following patches at hand. [ that's how I am reading them, and 
ahve to ask many stupid questions :P ]

> Balloon header includes:
>   - feature bit for Working Set Reporting
>   - number of Working Set bins member in balloon config
>   - types for communicating Working Set information
> 

Can you briefly summarize how all the bits here interact?

I assume, once VIRTIO_BALLOON_F_WS_REPORTING has been negotiated

(1) There is a new virtqueue for sending WS-related requests from the
     device (host) to the driver (guest).

-> How does a request look like?
-> How does a response look like?
-> Error cases?

(2) There is a new config space option.

-> Who's supposed to read this, who's supposed to write it?
-> Can it be changed dynamically?
-> What's the meaning / implication of that value.

> Signed-off-by: T.J. Alumbaugh <talumbau@google.com>
> ---
>   .../standard-headers/linux/virtio_balloon.h   | 20 +++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h
> index f343bfefd8..df61eaceee 100644
> --- a/include/standard-headers/linux/virtio_balloon.h
> +++ b/include/standard-headers/linux/virtio_balloon.h
> @@ -37,6 +37,7 @@
>   #define VIRTIO_BALLOON_F_FREE_PAGE_HINT	3 /* VQ to report free pages */
>   #define VIRTIO_BALLOON_F_PAGE_POISON	4 /* Guest is using page poisoning */
>   #define VIRTIO_BALLOON_F_REPORTING	5 /* Page reporting virtqueue */
> +#define VIRTIO_BALLOON_F_WS_REPORTING	6 /* Working set report virtqueues */

... are there multiple virtqueues? How many?

>   
>   /* Size of a PFN in the balloon interface. */
>   #define VIRTIO_BALLOON_PFN_SHIFT 12
> @@ -59,6 +60,9 @@ struct virtio_balloon_config {
>   	};
>   	/* Stores PAGE_POISON if page poisoning is in use */
>   	uint32_t poison_val;
> +	/* Stores the number of histogram bins if WS reporting in use */
> +	uint8_t working_set_num_bins;
> +	uint8_t padding[3];
>   };
>   
>   #define VIRTIO_BALLOON_S_SWAP_IN  0   /* Amount of memory swapped in */
> @@ -116,4 +120,20 @@ struct virtio_balloon_stat {
>   	__virtio64 val;
>   } QEMU_PACKED;
>   
> +enum virtio_balloon_working_set_op {
> +    VIRTIO_BALLOON_WS_REQUEST = 1, /* a Working Set request from the host */
> +    VIRTIO_BALLOON_WS_CONFIG = 2,  /* a Working Set config from the host */
> +};
> +
> +struct virtio_balloon_working_set {
> +	/* A tag for additional metadata */
> +	__virtio16 tag;
> +	/* The NUMA node for this report. */
> +	__virtio16 node_id;

How will we handle the case when the guest decides to use a different 
NUMA layout (e.g., numa disabled, fake numa, ...).

Is the guest supposed to detect that and *not* indicate a NUMA ID then?


Also, I wonder

> +	uint8_t reserved[4];
> +	__virtio64 idle_age_ms;
> +	/* A bin each for anonymous and file-backed memory. */

Why not have them separately, and properly named?

I'm not sure if it's a good idea to distinguish them based on anon vs. 
file-backed.

What would you do with shmem? It can be swapped like anon memory, ... if 
swap is enabled.

What's the main motivation for splitting this up? Is the "file-backed" 
part supposed to give some idea about the pagecache size? But what about 
mlock or page pinning?


Now I should take a step back and read the cover letter :)

-- 
Thanks,

David / dhildenb

On Wed, May 31, 2023 at 10:12:26AM +0200, David Hildenbrand wrote:
> On 26.05.23 00:20, T.J. Alumbaugh wrote:
> 
> Hi,
> 
> please try writing a comprehensive patch description: the goal should be
> that one can understand what's happening in the single patch without all of
> the following patches at hand. [ that's how I am reading them, and ahve to
> ask many stupid questions :P ]
> 
> > Balloon header includes:
> >   - feature bit for Working Set Reporting
> >   - number of Working Set bins member in balloon config
> >   - types for communicating Working Set information
> > 
> 
> Can you briefly summarize how all the bits here interact?
> 
> I assume, once VIRTIO_BALLOON_F_WS_REPORTING has been negotiated
> 
> (1) There is a new virtqueue for sending WS-related requests from the
>     device (host) to the driver (guest).

this belongs in driver description, indeed.

> -> How does a request look like?
> -> How does a response look like?
> -> Error cases?

These last 3 are best in the spec, not in driver.

> (2) There is a new config space option.
> 
> -> Who's supposed to read this, who's supposed to write it?
> -> Can it be changed dynamically?
> -> What's the meaning / implication of that value.

same. best in spec.

> > Signed-off-by: T.J. Alumbaugh <talumbau@google.com>
> > ---
> >   .../standard-headers/linux/virtio_balloon.h   | 20 +++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> > 
> > diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h
> > index f343bfefd8..df61eaceee 100644
> > --- a/include/standard-headers/linux/virtio_balloon.h
> > +++ b/include/standard-headers/linux/virtio_balloon.h
> > @@ -37,6 +37,7 @@
> >   #define VIRTIO_BALLOON_F_FREE_PAGE_HINT	3 /* VQ to report free pages */
> >   #define VIRTIO_BALLOON_F_PAGE_POISON	4 /* Guest is using page poisoning */
> >   #define VIRTIO_BALLOON_F_REPORTING	5 /* Page reporting virtqueue */
> > +#define VIRTIO_BALLOON_F_WS_REPORTING	6 /* Working set report virtqueues */
> 
> ... are there multiple virtqueues? How many?
> 
> >   /* Size of a PFN in the balloon interface. */
> >   #define VIRTIO_BALLOON_PFN_SHIFT 12
> > @@ -59,6 +60,9 @@ struct virtio_balloon_config {
> >   	};
> >   	/* Stores PAGE_POISON if page poisoning is in use */
> >   	uint32_t poison_val;
> > +	/* Stores the number of histogram bins if WS reporting in use */
> > +	uint8_t working_set_num_bins;
> > +	uint8_t padding[3];
> >   };
> >   #define VIRTIO_BALLOON_S_SWAP_IN  0   /* Amount of memory swapped in */
> > @@ -116,4 +120,20 @@ struct virtio_balloon_stat {
> >   	__virtio64 val;
> >   } QEMU_PACKED;
> > +enum virtio_balloon_working_set_op {
> > +    VIRTIO_BALLOON_WS_REQUEST = 1, /* a Working Set request from the host */
> > +    VIRTIO_BALLOON_WS_CONFIG = 2,  /* a Working Set config from the host */
> > +};
> > +
> > +struct virtio_balloon_working_set {
> > +	/* A tag for additional metadata */
> > +	__virtio16 tag;
> > +	/* The NUMA node for this report. */
> > +	__virtio16 node_id;
> 
> How will we handle the case when the guest decides to use a different NUMA
> layout (e.g., numa disabled, fake numa, ...).
> 
> Is the guest supposed to detect that and *not* indicate a NUMA ID then?
> 
> 
> Also, I wonder
> 
> > +	uint8_t reserved[4];
> > +	__virtio64 idle_age_ms;
> > +	/* A bin each for anonymous and file-backed memory. */
> 
> Why not have them separately, and properly named?
> 
> I'm not sure if it's a good idea to distinguish them based on anon vs.
> file-backed.
> 
> What would you do with shmem? It can be swapped like anon memory, ... if
> swap is enabled.
> 
> What's the main motivation for splitting this up? Is the "file-backed" part
> supposed to give some idea about the pagecache size? But what about mlock or
> page pinning?
> 
> 
> Now I should take a step back and read the cover letter :)
> 
> -- 
> Thanks,
> 
> David / dhildenb