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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240422045442-mutt-send-email-mst@kernel.org>
Date: Mon, 22 Apr 2024 04:58:20 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: zhenwei pi <pizhenwei@...edance.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	virtualization@...ts.linux.dev, david@...hat.com,
	jasowang@...hat.com, xuanzhuo@...ux.alibaba.com,
	akpm@...ux-foundation.org
Subject: 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 <pizhenwei@...edance.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