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 <talum...@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
