Hi,
v1 -> v2:
- Add a new patch 'virtio_balloon: separate vm events into a function'
to avoid any compiler warnings(unused stack variable on
CONFIG_VM_EVENT_COUNTERS=n)
- Suggested by David, use a loop 'for (zid = 0; zid < MAX_NR_ZONES; zid++)'
to obtain all the stall events.
RFC -> v1:
- several text changes: oom-kill -> oom-kills, SCAN_ASYNC -> ASYN_SCAN.
- move vm events codes into '#ifdef CONFIG_VM_EVENT_COUNTERS'
RFC version:
Link: https://lore.kernel.org/lkml/[email protected]/T/#m1898963b3c27a989b1123db475135c3ca687ca84
zhenwei pi (4):
virtio_balloon: separate vm events into a function
virtio_balloon: introduce oom-kill invocations
virtio_balloon: introduce memory allocation stall counter
virtio_balloon: introduce memory scan/reclaim info
drivers/virtio/virtio_balloon.c | 62 ++++++++++++++++++++++-------
include/uapi/linux/virtio_balloon.h | 16 +++++++-
2 files changed, 61 insertions(+), 17 deletions(-)
--
2.34.1
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 <[email protected]>
---
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
When the guest OS runs under critical memory pressure, the guest
starts to kill processes. A guest monitor agent may scan 'oom_kill'
from /proc/vmstat, and reports the OOM KILL event. However, the agent
may be killed and we will loss this critical event(and the later
events).
For now we can also grep for magic words in guest kernel log from host
side. Rather than this unstable way, virtio balloon reports OOM-KILL
invocations instead.
Acked-by: David Hildenbrand <[email protected]>
Signed-off-by: zhenwei pi <[email protected]>
---
drivers/virtio/virtio_balloon.c | 1 +
include/uapi/linux/virtio_balloon.h | 6 ++++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 59fe157e5722..87a1d6fa77fb 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -331,6 +331,7 @@ static inline unsigned int update_balloon_vm_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]);
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_OOM_KILL, events[OOM_KILL]);
#ifdef CONFIG_HUGETLB_PAGE
update_stat(vb, idx++, VIRTIO_BALLOON_S_HTLB_PGALLOC,
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index ddaa45e723c4..b17bbe033697 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -71,7 +71,8 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_CACHES 7 /* Disk caches */
#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
+#define VIRTIO_BALLOON_S_OOM_KILL 10 /* OOM killer invocations */
+#define VIRTIO_BALLOON_S_NR 11
#define VIRTIO_BALLOON_S_NAMES_WITH_PREFIX(VIRTIO_BALLOON_S_NAMES_prefix) { \
VIRTIO_BALLOON_S_NAMES_prefix "swap-in", \
@@ -83,7 +84,8 @@ struct virtio_balloon_config {
VIRTIO_BALLOON_S_NAMES_prefix "available-memory", \
VIRTIO_BALLOON_S_NAMES_prefix "disk-caches", \
VIRTIO_BALLOON_S_NAMES_prefix "hugetlb-allocations", \
- VIRTIO_BALLOON_S_NAMES_prefix "hugetlb-failures" \
+ VIRTIO_BALLOON_S_NAMES_prefix "hugetlb-failures", \
+ VIRTIO_BALLOON_S_NAMES_prefix "oom-kills" \
}
#define VIRTIO_BALLOON_S_NAMES VIRTIO_BALLOON_S_NAMES_WITH_PREFIX("")
--
2.34.1
Memory allocation stall counter represents the performance/latency of
memory allocation, expose this counter to the host side by virtio
balloon device via out-of-bound way.
Signed-off-by: zhenwei pi <[email protected]>
---
drivers/virtio/virtio_balloon.c | 8 ++++++++
include/uapi/linux/virtio_balloon.h | 6 ++++--
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 87a1d6fa77fb..ab039e83bc6f 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -323,6 +323,8 @@ static inline unsigned int update_balloon_vm_stats(struct virtio_balloon *vb,
#ifdef CONFIG_VM_EVENT_COUNTERS
unsigned long events[NR_VM_EVENT_ITEMS];
unsigned int idx = start;
+ unsigned int zid;
+ unsigned long stall = 0;
all_vm_events(events);
update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
@@ -333,6 +335,12 @@ static inline unsigned int update_balloon_vm_stats(struct virtio_balloon *vb,
update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
update_stat(vb, idx++, VIRTIO_BALLOON_S_OOM_KILL, events[OOM_KILL]);
+ /* sum all the stall events */
+ for (zid = 0; zid < MAX_NR_ZONES; zid++)
+ stall += events[ALLOCSTALL_NORMAL - ZONE_NORMAL + zid];
+
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_ALLOC_STALL, stall);
+
#ifdef CONFIG_HUGETLB_PAGE
update_stat(vb, idx++, VIRTIO_BALLOON_S_HTLB_PGALLOC,
events[HTLB_BUDDY_PGALLOC]);
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index b17bbe033697..487b893a160e 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -72,7 +72,8 @@ struct virtio_balloon_config {
#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_OOM_KILL 10 /* OOM killer invocations */
-#define VIRTIO_BALLOON_S_NR 11
+#define VIRTIO_BALLOON_S_ALLOC_STALL 11 /* Stall count of memory allocatoin */
+#define VIRTIO_BALLOON_S_NR 12
#define VIRTIO_BALLOON_S_NAMES_WITH_PREFIX(VIRTIO_BALLOON_S_NAMES_prefix) { \
VIRTIO_BALLOON_S_NAMES_prefix "swap-in", \
@@ -85,7 +86,8 @@ struct virtio_balloon_config {
VIRTIO_BALLOON_S_NAMES_prefix "disk-caches", \
VIRTIO_BALLOON_S_NAMES_prefix "hugetlb-allocations", \
VIRTIO_BALLOON_S_NAMES_prefix "hugetlb-failures", \
- VIRTIO_BALLOON_S_NAMES_prefix "oom-kills" \
+ VIRTIO_BALLOON_S_NAMES_prefix "oom-kills", \
+ VIRTIO_BALLOON_S_NAMES_prefix "alloc-stalls" \
}
#define VIRTIO_BALLOON_S_NAMES VIRTIO_BALLOON_S_NAMES_WITH_PREFIX("")
--
2.34.1
Expose memory scan/reclaim information to the host side via virtio
balloon device.
Now we have a metric to analyze the memory performance:
y: counter increases
n: counter does not changes
h: the rate of counter change is high
l: the rate of counter change is low
OOM: VIRTIO_BALLOON_S_OOM_KILL
STALL: VIRTIO_BALLOON_S_ALLOC_STALL
ASCAN: VIRTIO_BALLOON_S_SCAN_ASYNC
DSCAN: VIRTIO_BALLOON_S_SCAN_DIRECT
ARCLM: VIRTIO_BALLOON_S_RECLAIM_ASYNC
DRCLM: VIRTIO_BALLOON_S_RECLAIM_DIRECT
- OOM[y], STALL[*], ASCAN[*], DSCAN[*], ARCLM[*], DRCLM[*]:
the guest runs under really critial memory pressure
- OOM[n], STALL[h], ASCAN[*], DSCAN[l], ARCLM[*], DRCLM[l]:
the memory allocation stalls due to cgroup, not the global memory
pressure.
- OOM[n], STALL[h], ASCAN[*], DSCAN[h], ARCLM[*], DRCLM[h]:
the memory allocation stalls due to global memory pressure. The
performance gets hurt a lot. A high ratio between DRCLM/DSCAN shows
quite effective memory reclaiming.
- OOM[n], STALL[h], ASCAN[*], DSCAN[h], ARCLM[*], DRCLM[l]:
the memory allocation stalls due to global memory pressure.
the ratio between DRCLM/DSCAN gets low, the guest OS is thrashing
heavily, the serious case leads poor performance and difficult
trouble shooting. Ex, sshd may block on memory allocation when
accepting new connections, a user can't login a VM by ssh command.
- OOM[n], STALL[n], ASCAN[h], DSCAN[n], ARCLM[l], DRCLM[n]:
the low ratio between ARCLM/ASCAN shows that the guest tries to
reclaim more memory, but it can't. Once more memory is required in
future, it will struggle to reclaim memory.
Acked-by: David Hildenbrand <[email protected]>
Signed-off-by: zhenwei pi <[email protected]>
---
drivers/virtio/virtio_balloon.c | 9 +++++++++
include/uapi/linux/virtio_balloon.h | 12 ++++++++++--
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index ab039e83bc6f..e45146fde164 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -341,6 +341,15 @@ static inline unsigned int update_balloon_vm_stats(struct virtio_balloon *vb,
update_stat(vb, idx++, VIRTIO_BALLOON_S_ALLOC_STALL, stall);
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_ASYNC_SCAN,
+ pages_to_bytes(events[PGSCAN_KSWAPD]));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_DIRECT_SCAN,
+ pages_to_bytes(events[PGSCAN_DIRECT]));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_ASYNC_RECLAIM,
+ pages_to_bytes(events[PGSTEAL_KSWAPD]));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_DIRECT_RECLAIM,
+ pages_to_bytes(events[PGSTEAL_DIRECT]));
+
#ifdef CONFIG_HUGETLB_PAGE
update_stat(vb, idx++, VIRTIO_BALLOON_S_HTLB_PGALLOC,
events[HTLB_BUDDY_PGALLOC]);
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 487b893a160e..ee35a372805d 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -73,7 +73,11 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_HTLB_PGFAIL 9 /* Hugetlb page allocation failures */
#define VIRTIO_BALLOON_S_OOM_KILL 10 /* OOM killer invocations */
#define VIRTIO_BALLOON_S_ALLOC_STALL 11 /* Stall count of memory allocatoin */
-#define VIRTIO_BALLOON_S_NR 12
+#define VIRTIO_BALLOON_S_ASYNC_SCAN 12 /* Amount of memory scanned asynchronously */
+#define VIRTIO_BALLOON_S_DIRECT_SCAN 13 /* Amount of memory scanned directly */
+#define VIRTIO_BALLOON_S_ASYNC_RECLAIM 14 /* Amount of memory reclaimed asynchronously */
+#define VIRTIO_BALLOON_S_DIRECT_RECLAIM 15 /* Amount of memory reclaimed directly */
+#define VIRTIO_BALLOON_S_NR 16
#define VIRTIO_BALLOON_S_NAMES_WITH_PREFIX(VIRTIO_BALLOON_S_NAMES_prefix) { \
VIRTIO_BALLOON_S_NAMES_prefix "swap-in", \
@@ -87,7 +91,11 @@ struct virtio_balloon_config {
VIRTIO_BALLOON_S_NAMES_prefix "hugetlb-allocations", \
VIRTIO_BALLOON_S_NAMES_prefix "hugetlb-failures", \
VIRTIO_BALLOON_S_NAMES_prefix "oom-kills", \
- VIRTIO_BALLOON_S_NAMES_prefix "alloc-stalls" \
+ VIRTIO_BALLOON_S_NAMES_prefix "alloc-stalls", \
+ VIRTIO_BALLOON_S_NAMES_prefix "async-scans", \
+ VIRTIO_BALLOON_S_NAMES_prefix "direct-scans", \
+ VIRTIO_BALLOON_S_NAMES_prefix "async-reclaims", \
+ VIRTIO_BALLOON_S_NAMES_prefix "direct-reclaims" \
}
#define VIRTIO_BALLOON_S_NAMES VIRTIO_BALLOON_S_NAMES_WITH_PREFIX("")
--
2.34.1
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
On 22.04.24 09:42, zhenwei pi wrote:
> Memory allocation stall counter represents the performance/latency of
> memory allocation, expose this counter to the host side by virtio
> balloon device via out-of-bound way.
>
> Signed-off-by: zhenwei pi <[email protected]>
> ---
> drivers/virtio/virtio_balloon.c | 8 ++++++++
> include/uapi/linux/virtio_balloon.h | 6 ++++--
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 87a1d6fa77fb..ab039e83bc6f 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -323,6 +323,8 @@ static inline unsigned int update_balloon_vm_stats(struct virtio_balloon *vb,
> #ifdef CONFIG_VM_EVENT_COUNTERS
> unsigned long events[NR_VM_EVENT_ITEMS];
> unsigned int idx = start;
> + unsigned int zid;
> + unsigned long stall = 0;
>
> all_vm_events(events);
> update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> @@ -333,6 +335,12 @@ static inline unsigned int update_balloon_vm_stats(struct virtio_balloon *vb,
> update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> update_stat(vb, idx++, VIRTIO_BALLOON_S_OOM_KILL, events[OOM_KILL]);
>
> + /* sum all the stall events */
> + for (zid = 0; zid < MAX_NR_ZONES; zid++)
> + stall += events[ALLOCSTALL_NORMAL - ZONE_NORMAL + zid];
> +
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_ALLOC_STALL, stall);
> +
> #ifdef CONFIG_HUGETLB_PAGE
> update_stat(vb, idx++, VIRTIO_BALLOON_S_HTLB_PGALLOC,
> events[HTLB_BUDDY_PGALLOC]);
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index b17bbe033697..487b893a160e 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -72,7 +72,8 @@ struct virtio_balloon_config {
> #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_OOM_KILL 10 /* OOM killer invocations */
> -#define VIRTIO_BALLOON_S_NR 11
> +#define VIRTIO_BALLOON_S_ALLOC_STALL 11 /* Stall count of memory allocatoin */
> +#define VIRTIO_BALLOON_S_NR 12
>
> #define VIRTIO_BALLOON_S_NAMES_WITH_PREFIX(VIRTIO_BALLOON_S_NAMES_prefix) { \
> VIRTIO_BALLOON_S_NAMES_prefix "swap-in", \
> @@ -85,7 +86,8 @@ struct virtio_balloon_config {
> VIRTIO_BALLOON_S_NAMES_prefix "disk-caches", \
> VIRTIO_BALLOON_S_NAMES_prefix "hugetlb-allocations", \
> VIRTIO_BALLOON_S_NAMES_prefix "hugetlb-failures", \
> - VIRTIO_BALLOON_S_NAMES_prefix "oom-kills" \
> + VIRTIO_BALLOON_S_NAMES_prefix "oom-kills", \
> + VIRTIO_BALLOON_S_NAMES_prefix "alloc-stalls" \
> }
>
> #define VIRTIO_BALLOON_S_NAMES VIRTIO_BALLOON_S_NAMES_WITH_PREFIX("")
Acked-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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