Re: [PATCH v2 1/4] virtio_balloon: separate vm events into a function
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
---
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();
-
- 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();
+ 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
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
Re: [PATCH v2 1/4] virtio_balloon: separate vm events into a function
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
> ---
> 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();
> -
> - 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();
> + 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: Re: [PATCH v2 1/4] virtio_balloon: separate vm events into a function
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
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
