Re: [RFC PATCH v2 1/5] virtio-balloon: Add Working Set Reporting feature

[David Hildenbrand <david@redhat.com>]

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