Re: [RFC PATCH] SYSCTL_numainfo.memsize: Switch spanned to present memory

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.12.2024 12:12, Bernhard Kaindl wrote:
> This the 2nd part of my submission to fix the NUMA node memsize
> retured in xen_sysctl_meminfo[].memsize by the XEN_SYSCTL_numainfo
> hypercall to not count MMIO memory holes etc but only memory pages.
> 
> For this, we introduced NODE_DATA->node_present_pages as a prereq.
> With the prereq merged in master, I send this 2nd part for review:
> 
> This RFC is for changing the value of xen_sysctl_meminfo[]->memsize
> from NODE_DATA->node_spanned_pages << PAGE_SHIFT
>   to NODE_DATA->node_present_pages << PAGE_SHIFT
> for returing total present NUMA node memory instead of spanned range.
> 
> Sample of struct xen_sysctl_meminfo[].* as presented by in xl info -n:
> 
> xl info -n:
> [...]
> node:    memsize    memfree    distances
>    0:  -> 67584 <-   60672      10,21
>    1:     65536      60958      21,10
> 
> The -> memsize <- marked here is the value that we'd like to fix:
> The current value based node_spanned_pages is often 2TB too large.
> 
> We're currently not using these often false memsize values in XenServer
> according to my code review and and Andrew seemed to confirm this as well.
> 
> I think that the same is likely true for other Xen toolstacks, but of course
> to review this change or propose an alternaive is the purpose of this RFC.
> 
> Thanks,
> Bernhard

All of the above reads like a cover letter. What's missing is a patch
description, part of which would be to clarify whether the field is
indeed unused except for display purposes, or why respective users would
at least not regress from this change. What's also unclear is what
comments you're actually after (i.e. what question(s) you want to have
answered), seeing this is tagged RFC.

The code change is in principle fine, just that ...

> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxx>
> ---
>  xen/common/sysctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index d02f44fe3a..cba6d3cfea 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -316,7 +316,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>                  {
>                      if ( node_online(i) )
>                      {
> -                        meminfo.memsize = node_spanned_pages(i) << 
> PAGE_SHIFT;
> +                        meminfo.memsize = node_present_pages(i) << 
> PAGE_SHIFT;
>                          meminfo.memfree = avail_node_heap_pages(i) << 
> PAGE_SHIFT;
>                      }
>                      else
> --
> 2.43.0
> 
> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxx>
> ---
>  xen/common/sysctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index d02f44fe3a..cba6d3cfea 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -316,7 +316,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>                  {
>                      if ( node_online(i) )
>                      {
> -                        meminfo.memsize = node_spanned_pages(i) << 
> PAGE_SHIFT;
> +                        meminfo.memsize = node_present_pages(i) << 
> PAGE_SHIFT;
>                          meminfo.memfree = avail_node_heap_pages(i) << 
> PAGE_SHIFT;
>                      }
>                      else

... everything appears twice in your email.

Jan