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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a9606b92-4be4-df2e-2cf9-64d4b2a54628@oracle.com>
Date:   Fri, 13 Apr 2018 15:03:43 -0700
From:   Jonathan Helman <jonathan.helman@...cle.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Jason Wang <jasowang@...hat.com>,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, virtio-dev@...ts.oasis-open.org,
        Mihai Carabas <mihai.carabas@...cle.com>
Subject: Re: [virtio-dev] Re: [PATCH v2] virtio_balloon: export hugetlb page
 allocation counts



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@...hat.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@...cle.com>
>>>>>>>>>>>> Reviewed-by: Jason Wang<jasowang@...hat.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@...hat.com>
>>> Date:   Sun Nov 12 13:05:38 2017 +0100
>>>
>>>       virtio_balloon: include disk/file caches memory statistics
>>>
>>> commit 5057dcd0f1aaad57e07e728ba20a99e205c6b9de
>>> Author: Igor Redko <redkoi@...tuozzo.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.