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

[Jason Wang <jasowang@redhat.com>]

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