Re: [PATCH v2 5/6] perf header: Support memory ranges

["Falcon, Thomas" <thomas.falcon@intel.com>]

On Tue, 2026-07-28 at 10:33 -0700, Namhyung Kim wrote:
> On Mon, Jul 13, 2026 at 07:43:58PM -0500, Thomas Falcon wrote:
> > Memory ranges were created to track different types of memory,
> > such as persistent, high bandwidth, or CXL-attached, that may
> > be present on a system for performance monitoring and resource
> > control purposes.
> > 
> > Memory range data are parsed from the ACPI MRRM table and exposed
> > to userspace tools via sysfs [1]. Memory range data is read from:
> > 
> > 	/sys/firmware/acpi/memory_ranges/rangeX
> > 
> > With the following attributes:
> > 
> > 	u64 base;
> > 	u64 length;
> > 	int node;
> > 	u8  local_region_id;
> > 	u8  remote_region_id;
> > 
> > Read memory range data from sysfs if present and save it in
> > the header of the perf data file under a new feature bit,
> > HEADER_MEMORY_RANGES (35). Memory range data can be viewed with the
> > --header or --header-only options of perf-report and perf-script.
> 
> Can you please add an example output here?

Hi, thanks for reviewing. To be honest, I have not been able to get
access to supported hardware yet so the only example I can provide
would look like this (on a non-capable system)

# memory ranges (nr 0):

I will look into getting access to one and, in the meantime, make the
other changes you requested.

Thanks,
Tom 

> 
> > 
> > [1]:
> > https://lore.kernel.org/lkml/20250505173819.419271-1-tony.luck@intel.com/
> > 
> > Assisted-by: Sashiko:gemini-3.1-pro-preview
> > Signed-off-by: Thomas Falcon <thomas.falcon@intel.com>
> > ---
> > Changes in v2: 
> > -- Added check for NULL return of sysfs__mountpoint() when parsing
> >    memory ranges in sysfs
> > -- increased MAX_MEMORY_RANGES sanity check from 64 to 256 based on
> >    ACPI MRRM table implementation in Linux kernel
> > -- added comment for MAX_MEMORY_RANGES to clarify that it is a
> > sanity check
> >    for malformed perf.data files
> > -- removed a line of code was removed from util/env.h but was added
> > back in
> >    v1 due to bad rebase
> > ---
> [SNIP]
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index e90e541f546b..0669898362d6 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -92,6 +92,7 @@
> >  #define MAX_PMU_CAPS		512
> >  #define MAX_PMU_MAPPINGS	4096
> >  #define MAX_SCHED_DOMAINS	64
> > +#define MAX_MEMORY_RANGES	256
> >  
> >  /*
> >   * magic2 = "PERFILE2"
> > @@ -1891,6 +1892,130 @@ static int write_cpu_domain_info(struct
> > feat_fd *ff,
> >  	return ret;
> >  }
> >  
> > +static int memory_range__read(struct memory_range *range, int idx)
> > +{
> > +	char path[PATH_MAX], file[PATH_MAX];
> 
> I hope we can reduce the number of buffers for the pathnames as it
> already has another in the caller.  Ideally it'd be great if it can
> use
> openat() with an FD for the directory.
> 
> 
> > +	struct stat st;
> > +	int tmp;
> > +
> > +	scnprintf(path, PATH_MAX,
> > "firmware/acpi/memory_ranges/range%d", idx);
> > +	scnprintf(file, PATH_MAX, "%s/%s", sysfs__mountpoint(),
> > path);
> > +	if (stat(file, &st))
> > +		return -1;
> 
> It seems this check is redundant.
> 
> > +
> > +	scnprintf(file, PATH_MAX, "%s/base", path);
> > +	if (sysfs__read_xll(file, (unsigned long long *) &range-
> > >base))
> > +		return -1;
> > +
> > +	scnprintf(file, PATH_MAX, "%s/length", path);
> > +	if (sysfs__read_xll(file, (unsigned long long *) &range-
> > >length))
> > +		return -1;
> > +
> > +	scnprintf(file, PATH_MAX, "%s/node", path);
> > +	if (sysfs__read_int(file, (int *) &range->node))
> > +		return -1;
> > +
> > +	scnprintf(file, PATH_MAX, "%s/local_region_id", path);
> > +	if (sysfs__read_int(file, &tmp))
> > +		return -1;
> > +
> > +	if (tmp < 0 || tmp > 255)
> 
> Probably better to compare with MAX_MEMORY_REGIONS.
> 
> 
> > +		return -1;
> > +	range->local_region_id = tmp;
> > +
> > +	scnprintf(file, PATH_MAX, "%s/remote_region_id", path);
> > +	if (sysfs__read_int(file, &tmp))
> > +		return -1;
> > +
> > +	if (tmp < 0 || tmp > 255)
> 
> Ditto.
> 
> 
> > +		return -1;
> > +	range->remote_region_id = tmp;
> > +
> > +	return 0;
> > +}
> > +
> > +static int memory_range__parse(struct memory_range **ranges)
> > +{
> > +	const char *sysfs = sysfs__mountpoint();
> > +	int i, err, nr_memory_ranges = 0;
> > +	char path[PATH_MAX];
> > +	struct stat st;
> > +
> > +	if (!sysfs)
> > +		return 0;
> > +
> > +	scnprintf(path, PATH_MAX,
> > "%s/firmware/acpi/memory_ranges", sysfs);
> > +	if (stat(path, &st))
> > +		return 0;
> > +
> > +	while (1) {
> > +		scnprintf(path, PATH_MAX,
> > "%s/firmware/acpi/memory_ranges/range%d", sysfs, nr_memory_ranges);
> 
> This line looks too long.
> 
> 
> > +		if (stat(path, &st))
> > +			break;
> > +
> > +		nr_memory_ranges++;
> > +	}
> > +
> > +	if (nr_memory_ranges == 0)
> > +		return 0;
> > +
> > +	*ranges = zalloc(nr_memory_ranges * sizeof(struct
> > memory_range));
> 
> I think calloc() is preferred when you multiply the size and the
> count.
> 
> 
> > +	if (!(*ranges))
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < nr_memory_ranges; i++) {
> > +		struct memory_range range;
> > +
> > +		err = memory_range__read(&range, i);
> > +		if (err < 0)
> > +			goto out_error;
> > +
> > +		(*ranges)[i] = range;
> > +	}
> > +
> > +	return nr_memory_ranges;
> > +
> > +out_error:
> > +	zfree(ranges);
> > +	return -1;
> > +}
> > +
> [SNIP] 
> > +static int process_memory_ranges(struct feat_fd *ff, void *data
> > __maybe_unused)
> > +{
> > +	struct perf_env *env = &ff->ph->env;
> > +	struct memory_range *ranges, *r;
> > +	u32 nr_memory_ranges, i;
> > +
> > +	if (do_read_u32(ff, &nr_memory_ranges))
> > +		return -1;
> > +
> > +	if (!nr_memory_ranges) {
> > +		pr_debug("memory ranges not available\n");
> > +		return 0;
> > +	}
> > +
> > +	/* According to version 1.1 of the ACPI MRRM table, the
> > maximum
> > +	 * number of memory regions can be at most 255. Do a
> > sanity check
> > +	 * here to guard against a malformed perf.data file.
> > +	 */
> > +	if (nr_memory_ranges >= MAX_MEMORY_RANGES) {
> > +		pr_err("Invalid memory_ranges: nr_memory_ranges
> > (%u) > %u\n",
> > +		       nr_memory_ranges, MAX_MEMORY_RANGES);
> > +		return -1;
> > +	}
> > +
> > +	ranges = zalloc(nr_memory_ranges * sizeof(*ranges));
> 
> Please use calloc().
> 
> Thanks,
> Namhyung
> 
> 
> > +	if (!ranges)
> > +		return -1;
> > +
> > +	for (i = 0; i < nr_memory_ranges; i++) {
> > +		r = &ranges[i];
> > +
> > +		if (do_read_u64(ff, &r->base))
> > +			goto error;
> > +		if (do_read_u64(ff, &r->length))
> > +			goto error;
> > +		if (do_read_u32(ff, (u32 *) &r->node))
> > +			goto error;
> > +		if (__do_read(ff, &r->local_region_id,
> > sizeof(u8)))
> > +			goto error;
> > +		if (__do_read(ff, &r->remote_region_id,
> > sizeof(u8)))
> > +			goto error;
> > +	}
> > +
> > +	env->memory_ranges = ranges;
> > +	env->nr_memory_ranges = nr_memory_ranges;
> > +
> > +	return 0;
> > +error:
> > +	zfree(&ranges);
> > +	return -1;
> > +}
> > +
> >  #define FEAT_OPR(n, func, __full_only) \
> >  	[HEADER_##n] = {					\
> >  		.name	    = __stringify(n),			\
> > @@ -4265,6 +4459,7 @@ const struct perf_header_feature_ops
> > feat_ops[HEADER_LAST_FEATURE] = {
> >  	FEAT_OPR(CPU_DOMAIN_INFO,	cpu_domain_info,	tr
> > ue),
> >  	FEAT_OPR(E_MACHINE,	e_machine,	false),
> >  	FEAT_OPR(CLN_SIZE,	cln_size,	false),
> > +	FEAT_OPR(MEMORY_RANGES,	memory_ranges,	false),
> >  };
> >  
> >  struct header_print_data {
> > diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> > index 5e03f884b7cc..765037762758 100644
> > --- a/tools/perf/util/header.h
> > +++ b/tools/perf/util/header.h
> > @@ -56,6 +56,7 @@ enum {
> >  	HEADER_CPU_DOMAIN_INFO,
> >  	HEADER_E_MACHINE,
> >  	HEADER_CLN_SIZE,
> > +	HEADER_MEMORY_RANGES,
> >  	HEADER_LAST_FEATURE,
> >  	HEADER_FEAT_BITS	= 256,
> >  };
> > -- 
> > 2.43.0
> >