[v2] Improve memory statistics for virtio balloon

[PATCH v2 1/4] virtio_balloon: separate vm events into a function

Posted by zhenwei pi 1 year, 9 months ago

All the VM events related statistics have dependence on
'CONFIG_VM_EVENT_COUNTERS', once any stack variable is required by any
VM events in future, we would have codes like:
 #ifdef CONFIG_VM_EVENT_COUNTERS
      unsigned long foo;
 #endif
      ...
 #ifdef CONFIG_VM_EVENT_COUNTERS
      foo = events[XXX] + events[YYY];
      update_stat(vb, idx++, VIRTIO_BALLOON_S_XXX, foo);
 #endif

Separate vm events into a single function, also remove
'CONFIG_VM_EVENT_COUNTERS' from 'update_balloon_stats'.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/virtio/virtio_balloon.c | 44 ++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 1f5b3dd31fcf..59fe157e5722 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -316,34 +316,48 @@ static inline void update_stat(struct virtio_balloon *vb, int idx,
 
 #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
 
-static unsigned int update_balloon_stats(struct virtio_balloon *vb)
+/* Return the number of entries filled by vm events */
+static inline unsigned int update_balloon_vm_stats(struct virtio_balloon *vb,
+						   unsigned int start)
 {
+#ifdef CONFIG_VM_EVENT_COUNTERS
 	unsigned long events[NR_VM_EVENT_ITEMS];
-	struct sysinfo i;
-	unsigned int idx = 0;
-	long available;
-	unsigned long caches;
+	unsigned int idx = start;
 
 	all_vm_events(events);
-	si_meminfo(&i);
-
-	available = si_mem_available();
-	caches = global_node_page_state(NR_FILE_PAGES);
-
-#ifdef CONFIG_VM_EVENT_COUNTERS
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
-				pages_to_bytes(events[PSWPIN]));
+		    pages_to_bytes(events[PSWPIN]));
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
-				pages_to_bytes(events[PSWPOUT]));
+		    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
+#endif /* CONFIG_HUGETLB_PAGE */
+
+	return idx - start;
+#else /* CONFIG_VM_EVENT_COUNTERS */
+
+	return 0;
+#endif /* CONFIG_VM_EVENT_COUNTERS */
+}
+
+static unsigned int update_balloon_stats(struct virtio_balloon *vb)
+{
+	struct sysinfo i;
+	unsigned int idx = 0;
+	long available;
+	unsigned long caches;
+
+	idx += update_balloon_vm_stats(vb, idx);
+
+	si_meminfo(&i);
+	available = si_mem_available();
+	caches = global_node_page_state(NR_FILE_PAGES);
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
 				pages_to_bytes(i.freeram));
 	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
-- 
2.34.1

Re: [PATCH v2 1/4] virtio_balloon: separate vm events into a function

Posted by David Hildenbrand 1 year, 9 months ago

On 22.04.24 09:42, zhenwei pi wrote:
> All the VM events related statistics have dependence on
> 'CONFIG_VM_EVENT_COUNTERS', once any stack variable is required by any
> VM events in future, we would have codes like:
>   #ifdef CONFIG_VM_EVENT_COUNTERS
>        unsigned long foo;
>   #endif
>        ...
>   #ifdef CONFIG_VM_EVENT_COUNTERS
>        foo = events[XXX] + events[YYY];
>        update_stat(vb, idx++, VIRTIO_BALLOON_S_XXX, foo);
>   #endif
> 
> Separate vm events into a single function, also remove
> 'CONFIG_VM_EVENT_COUNTERS' from 'update_balloon_stats'.
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>   drivers/virtio/virtio_balloon.c | 44 ++++++++++++++++++++++-----------
>   1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 1f5b3dd31fcf..59fe157e5722 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -316,34 +316,48 @@ static inline void update_stat(struct virtio_balloon *vb, int idx,
>   
>   #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
>   
> -static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> +/* Return the number of entries filled by vm events */
> +static inline unsigned int update_balloon_vm_stats(struct virtio_balloon *vb,
> +						   unsigned int start)
>   {
> +#ifdef CONFIG_VM_EVENT_COUNTERS
>   	unsigned long events[NR_VM_EVENT_ITEMS];
> -	struct sysinfo i;
> -	unsigned int idx = 0;
> -	long available;
> -	unsigned long caches;
> +	unsigned int idx = start;
>   
>   	all_vm_events(events);
> -	si_meminfo(&i);
> -
> -	available = si_mem_available();
> -	caches = global_node_page_state(NR_FILE_PAGES);
> -
> -#ifdef CONFIG_VM_EVENT_COUNTERS
>   	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> -				pages_to_bytes(events[PSWPIN]));
> +		    pages_to_bytes(events[PSWPIN]));
>   	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
> -				pages_to_bytes(events[PSWPOUT]));
> +		    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
> +#endif /* CONFIG_HUGETLB_PAGE */
> +
> +	return idx - start;
> +#else /* CONFIG_VM_EVENT_COUNTERS */
> +
> +	return 0;
> +#endif /* CONFIG_VM_EVENT_COUNTERS */
> +}
> +
> +static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> +{
> +	struct sysinfo i;
> +	unsigned int idx = 0;
> +	long available;
> +	unsigned long caches;
> +
> +	idx += update_balloon_vm_stats(vb, idx);

No need to handle idx that complicated now. Just do

unsigned int idx;

idx = update_balloon_vm_stats(vb);

We can go down that path if we ever want to rearrange the code and not 
have the vm_stats first.

> +
> +	si_meminfo(&i);
> +	available = si_mem_available();
> +	caches = global_node_page_state(NR_FILE_PAGES);
>   	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>   				pages_to_bytes(i.freeram));
>   	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 1/4] virtio_balloon: separate vm events into a function

Posted by Michael S. Tsirkin 1 year, 9 months ago

On Mon, Apr 22, 2024 at 03:42:51PM +0800, zhenwei pi wrote:
> All the VM events related statistics have dependence on
> 'CONFIG_VM_EVENT_COUNTERS', once any stack variable is required by any
> VM events in future, we would have codes like:
>  #ifdef CONFIG_VM_EVENT_COUNTERS
>       unsigned long foo;
>  #endif
>       ...
>  #ifdef CONFIG_VM_EVENT_COUNTERS
>       foo = events[XXX] + events[YYY];
>       update_stat(vb, idx++, VIRTIO_BALLOON_S_XXX, foo);
>  #endif
> 
> Separate vm events into a single function, also remove
> 'CONFIG_VM_EVENT_COUNTERS' from 'update_balloon_stats'.
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  drivers/virtio/virtio_balloon.c | 44 ++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 1f5b3dd31fcf..59fe157e5722 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -316,34 +316,48 @@ static inline void update_stat(struct virtio_balloon *vb, int idx,
>  
>  #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
>  
> -static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> +/* Return the number of entries filled by vm events */
> +static inline unsigned int update_balloon_vm_stats(struct virtio_balloon *vb,
> +						   unsigned int start)
>  {
> +#ifdef CONFIG_VM_EVENT_COUNTERS
>  	unsigned long events[NR_VM_EVENT_ITEMS];
> -	struct sysinfo i;
> -	unsigned int idx = 0;
> -	long available;
> -	unsigned long caches;
> +	unsigned int idx = start;
>  
>  	all_vm_events(events);
> -	si_meminfo(&i);
> -
> -	available = si_mem_available();
> -	caches = global_node_page_state(NR_FILE_PAGES);
> -
> -#ifdef CONFIG_VM_EVENT_COUNTERS
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> -				pages_to_bytes(events[PSWPIN]));
> +		    pages_to_bytes(events[PSWPIN]));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
> -				pages_to_bytes(events[PSWPOUT]));
> +		    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
> +#endif /* CONFIG_HUGETLB_PAGE */
> +
> +	return idx - start;
> +#else /* CONFIG_VM_EVENT_COUNTERS */
> +
> +	return 0;
> +#endif /* CONFIG_VM_EVENT_COUNTERS */
> +}
> +

Generally the preferred style is this:

#ifdef .....

static inline unsigned int update_balloon_vm_stats(struct virtio_balloon *vb,
						   unsigned int start)
{
	....
}

#else /* CONFIG_VM_EVENT_COUNTERS */

static inline unsigned int update_balloon_vm_stats(struct virtio_balloon *vb,
						   unsigned int start)
{
	return 0;
}

#endif

however given it was a spaghetti of ifdefs even before that,
the patch's ok I think.


> +static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> +{
> +	struct sysinfo i;
> +	unsigned int idx = 0;
> +	long available;
> +	unsigned long caches;
> +
> +	idx += update_balloon_vm_stats(vb, idx);
> +
> +	si_meminfo(&i);
> +	available = si_mem_available();
> +	caches = global_node_page_state(NR_FILE_PAGES);
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>  				pages_to_bytes(i.freeram));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
> -- 
> 2.34.1

Re: [PATCH v2 1/4] virtio_balloon: separate vm events into a function

Posted by David Hildenbrand 1 year, 9 months ago

On 22.04.24 09:42, zhenwei pi wrote:
> All the VM events related statistics have dependence on
> 'CONFIG_VM_EVENT_COUNTERS', once any stack variable is required by any
> VM events in future, we would have codes like:
>   #ifdef CONFIG_VM_EVENT_COUNTERS
>        unsigned long foo;
>   #endif
>        ...
>   #ifdef CONFIG_VM_EVENT_COUNTERS
>        foo = events[XXX] + events[YYY];
>        update_stat(vb, idx++, VIRTIO_BALLOON_S_XXX, foo);
>   #endif
> 
> Separate vm events into a single function, also remove

Why not simply use __maybe_unused for that variable?

-- 
Cheers,

David / dhildenb

Re: Re: [PATCH v2 1/4] virtio_balloon: separate vm events into a function

Posted by zhenwei pi 1 year, 9 months ago


On 4/22/24 15:47, David Hildenbrand wrote:
> On 22.04.24 09:42, zhenwei pi wrote:
>> All the VM events related statistics have dependence on
>> 'CONFIG_VM_EVENT_COUNTERS', once any stack variable is required by any
>> VM events in future, we would have codes like:
>>   #ifdef CONFIG_VM_EVENT_COUNTERS
>>        unsigned long foo;
>>   #endif
>>        ...
>>   #ifdef CONFIG_VM_EVENT_COUNTERS
>>        foo = events[XXX] + events[YYY];
>>        update_stat(vb, idx++, VIRTIO_BALLOON_S_XXX, foo);
>>   #endif
>>
>> Separate vm events into a single function, also remove
> 
> Why not simply use __maybe_unused for that variable?
> 

1>
static unsigned int update_balloon_stats()
{
     unsigned __maybe_unused long foo;

     ...
#ifdef CONFIG_VM_EVENT_COUNTERS
     foo = events[XXX] + events[YYY];
     update_stat(vb, idx++, VIRTIO_BALLOON_S_XXX, foo);
#endif
}

2>
static inline unsigned int update_balloon_vm_stats()
{
#ifdef CONFIG_VM_EVENT_COUNTERS
     unsigned long foo;

     foo = events[XXX] + events[YYY];
     update_stat(vb, idx++, VIRTIO_BALLOON_S_XXX, foo);
#endif
}

 From the point of my view, I don't need to compile code in my brain 
when reading codes for case 2. :)

-- 
zhenwei pi

Re: [PATCH v2 1/4] virtio_balloon: separate vm events into a function

Posted by David Hildenbrand 1 year, 9 months ago

On 22.04.24 10:04, zhenwei pi wrote:
> 
> 
> On 4/22/24 15:47, David Hildenbrand wrote:
>> On 22.04.24 09:42, zhenwei pi wrote:
>>> All the VM events related statistics have dependence on
>>> 'CONFIG_VM_EVENT_COUNTERS', once any stack variable is required by any
>>> VM events in future, we would have codes like:
>>>    #ifdef CONFIG_VM_EVENT_COUNTERS
>>>         unsigned long foo;
>>>    #endif
>>>         ...
>>>    #ifdef CONFIG_VM_EVENT_COUNTERS
>>>         foo = events[XXX] + events[YYY];
>>>         update_stat(vb, idx++, VIRTIO_BALLOON_S_XXX, foo);
>>>    #endif
>>>
>>> Separate vm events into a single function, also remove
>>
>> Why not simply use __maybe_unused for that variable?
>>
> 
> 1>
> static unsigned int update_balloon_stats()
> {
>       unsigned __maybe_unused long foo;
> 
>       ...
> #ifdef CONFIG_VM_EVENT_COUNTERS
>       foo = events[XXX] + events[YYY];
>       update_stat(vb, idx++, VIRTIO_BALLOON_S_XXX, foo);
> #endif
> }
> 
> 2>
> static inline unsigned int update_balloon_vm_stats()
> {
> #ifdef CONFIG_VM_EVENT_COUNTERS
>       unsigned long foo;
> 
>       foo = events[XXX] + events[YYY];
>       update_stat(vb, idx++, VIRTIO_BALLOON_S_XXX, foo);
> #endif
> }
> 
>   From the point of my view, I don't need to compile code in my brain
> when reading codes for case 2. :)

But for #1? :)

I mean, you didn't compile the code in your brain when you sent out v1 :P

But I agree that moving that to a separate function ins cleaner, staring 
at resulting update_balloon_stats().

Let me comment on some nits as a fresh reply.

-- 
Cheers,

David / dhildenb