Re: [virtio-dev] Re: [PATCH v2] virtio_balloon: export hugetlb page allocation counts

From: Jonathan Helman
Date: Fri Apr 13 2018 - 18:04:04 EST



On 04/13/2018 02:51 PM, Michael S. Tsirkin wrote:
On Fri, Apr 13, 2018 at 10:10:57AM -0700, Jonathan Helman wrote:


On 04/13/2018 06:44 AM, Michael S. Tsirkin wrote:
On Fri, Apr 13, 2018 at 03:01:11PM +0800, Jason Wang wrote:


On 2018å04æ12æ 08:24, Jonathan Helman wrote:


On 04/10/2018 08:12 PM, Jason Wang wrote:


On 2018å04æ10æ 05:11, Jonathan Helman wrote:


On 03/22/2018 07:38 PM, Jason Wang wrote:


On 2018å03æ22æ 11:10, Michael S. Tsirkin wrote:
On Thu, Mar 22, 2018 at 09:52:18AM +0800, Jason Wang wrote:
On 2018å03æ20æ 12:26, Jonathan Helman wrote:
On Mar 19, 2018, at 7:31 PM, Jason
Wang<jasowang@xxxxxxxxxx> wrote:



On 2018å03æ20æ 06:14, Jonathan Helman wrote:
Export the number of successful and failed hugetlb page
allocations via the virtio balloon driver. These 2 counts
come directly from the vm_events HTLB_BUDDY_PGALLOC and
HTLB_BUDDY_PGALLOC_FAIL.

Signed-off-by: Jonathan Helman<jonathan.helman@xxxxxxxxxx>
Reviewed-by: Jason Wang<jasowang@xxxxxxxxxx>
Thanks.

---
ÂÂ drivers/virtio/virtio_balloon.cÂÂÂÂ | 6 ++++++
ÂÂ include/uapi/linux/virtio_balloon.h | 4 +++-
ÂÂ 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git
a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
index dfe5684..6b237e3 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -272,6 +272,12 @@ static unsigned int
update_balloon_stats(struct
virtio_balloon *vb)
pages_to_bytes(events[PSWPOUT]));
ÂÂÂÂÂÂ update_stat(vb, idx++,
VIRTIO_BALLOON_S_MAJFLT,
events[PGMAJFAULT]);
ÂÂÂÂÂÂ update_stat(vb, idx++,
VIRTIO_BALLOON_S_MINFLT,
events[PGFAULT]);
+#ifdef CONFIG_HUGETLB_PAGE
+ÂÂÂ update_stat(vb, idx++, VIRTIO_BALLOON_S_HTLB_PGALLOC,
+ÂÂÂÂÂÂÂÂÂÂÂ events[HTLB_BUDDY_PGALLOC]);
+ÂÂÂ update_stat(vb, idx++, VIRTIO_BALLOON_S_HTLB_PGFAIL,
+ÂÂÂÂÂÂÂÂÂÂÂ events[HTLB_BUDDY_PGALLOC_FAIL]);
+#endif
ÂÂ #endif
ÂÂÂÂÂÂ update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pages_to_bytes(i.freeram));
diff --git
a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
index 4e8b830..40297a3 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -53,7 +53,9 @@ struct virtio_balloon_config {
ÂÂ #define VIRTIO_BALLOON_S_MEMTOTÂÂ 5
/* Total amount of memory */
ÂÂ #define VIRTIO_BALLOON_S_AVAILÂÂÂ 6
/* Available memory as in /proc */
ÂÂ #define VIRTIO_BALLOON_S_CACHESÂÂ 7ÂÂ /* Disk caches */
-#define VIRTIO_BALLOON_S_NRÂÂÂÂÂÂ 8
+#define VIRTIO_BALLOON_S_HTLB_PGALLOC
8Â /* Hugetlb page allocations */
+#define VIRTIO_BALLOON_S_HTLB_PGFAIL
9Â /* Hugetlb page allocation failures
*/
+#define VIRTIO_BALLOON_S_NRÂÂÂÂÂÂ 10
ÂÂÂÂ /*
ÂÂÂ * Memory statistics structure.
Not for this patch, but it looks to me that
exporting such nr through uapi is fragile.
Sorry, can you explain what you mean here?

Jon
Spec said "Within an output buffer submitted to the
statsq, the device MUST
ignore entries with tag values that it does not
recognize". So exporting
VIRTIO_BALLOON_S_NR seems useless and device
implementation can not depend
on such number in uapi.

Thanks
Suggestions? I don't like to break build for people ...


Didn't have a good idea. But maybe we should keep
VIRTIO_BALLOON_S_NR unchanged, and add a comment here.

Thanks

I think Jason's comment is for a future patch. Didn't see this
patch get applied, so wondering if it could be.

Thanks,
Jon

Hi Jon:

Have you tested new driver with old qemu?

Yes, this testing scenario looks good. Thanks.

Jon

Hi Jon:

I mean e.g compiling qemu with new linux headers. E.g current qemu has:

static const char *balloon_stat_names[] = {
ÂÂ [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in",
ÂÂ [VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out",
ÂÂ [VIRTIO_BALLOON_S_MAJFLT] = "stat-major-faults",
ÂÂ [VIRTIO_BALLOON_S_MINFLT] = "stat-minor-faults",
ÂÂ [VIRTIO_BALLOON_S_MEMFREE] = "stat-free-memory",
ÂÂ [VIRTIO_BALLOON_S_MEMTOT] = "stat-total-memory",
ÂÂ [VIRTIO_BALLOON_S_AVAIL] = "stat-available-memory",
ÂÂ [VIRTIO_BALLOON_S_CACHES] = "stat-disk-caches",
ÂÂ [VIRTIO_BALLOON_S_NR] = NULL
};

I'm afraid it will be broken if VIRTIO_BALLOON_S_NR is 10.

Thanks

Well it is handy for sizing arrays and this isn't the first time we did
this:

commit 4d32029b8ddb7be4d1699c6d8e1675ff5476d149
Author: TomÃÅ Golembiovskà <tgolembi@xxxxxxxxxx>
Date: Sun Nov 12 13:05:38 2017 +0100

virtio_balloon: include disk/file caches memory statistics

commit 5057dcd0f1aaad57e07e728ba20a99e205c6b9de
Author: Igor Redko <redkoi@xxxxxxxxxxxxx>
Date: Thu Mar 17 14:19:08 2016 -0700

virtio_balloon: export 'available' memory to balloon statistics

how about we give QEMU a hand and just put the list of names in
the header?

I posted a patch like that, pls review.


Sorry, maybe I'm missing something. We have an internal copy of the header
file in qemu under include/standard-headers/linux/virtio_balloon.h, which
hw/virtio/virtio-balloon.c includes (through including
hw/virtio/virtio-balloon.h). I thought qemu would use internal header files
so that it could be compiled on any Linux kernel version. So when would we
ever have the issue Jason is discussing?

Thanks,
Jon

Whenever we update the headers.


Ah got it. Updating the headers without updating that string array breaks things. Thanks.