Messages in this thread | | | Subject | Re: [virtio-dev] Re: [PATCH v2] virtio_balloon: export hugetlb page allocation counts | From | Jonathan Helman <> | Date | Fri, 13 Apr 2018 15:03:43 -0700 |
| |
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@redhat.com> 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@oracle.com> >>>>>>>>>>>> Reviewed-by: Jason Wang<jasowang@redhat.com> >>>>>>>>>>> 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@redhat.com> >>> Date: Sun Nov 12 13:05:38 2017 +0100 >>> >>> virtio_balloon: include disk/file caches memory statistics >>> >>> commit 5057dcd0f1aaad57e07e728ba20a99e205c6b9de >>> Author: Igor Redko <redkoi@virtuozzo.com> >>> 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.
| |