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

[Michael S. Tsirkin]

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