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(&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
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(&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: 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