Re: [PATCH v10 00/17] Allow AET to use PMT as loadable module

[Luck, Tony]

Sashiko review: https://sashiko.dev/#/patchset/20260729172752.11561-1-tony.luck%40intel.com

My responses to issues raised below. In some cases I've clipped just
part of Sashiko response. Use the link above to see the full details
and context for each.

--- Patch [1]: [PATCH v10 01/17] x86/resctrl: Fix enumeration of number of supported RMIDs ---

  "Does this early return introduce a potential division-by-zero during boot ..."

Yes. I need to add this code to the start of get_rdt_mon_resources():

	if (boot_cpu_data.x86_cache_max_rmid < 0)
		return false;

--- Patch 7: [PATCH v10 07/17] arm,x86,fs/resctrl: Handle change in number of RMIDs on each mount ---

  "Does this logic ignore Application Energy Telemetry (AET) if it relies on
   RDT_RESOURCE_PERF_PKG instead of RDT_RESOURCE_L3? 
   If the system supports AET but lacks L3 cache monitoring, x86_cache_max_rmid
   could be less than or equal to 0, causing this function to return 0. 
   Additionally, if the fallback to x86_cache_max_rmid is taken, x86_cache_max_rmid
   represents a maximum index rather than a count. Could this cause an off-by-one 
   under-allocation, since a maximum index of N means there are N + 1 RMIDs?"

First part of this complaint is incorrect. If any monitoring is
supported (including AET) then boot_cpu_data.x86_cache_max_rmid will be
some number greater than zero from cpuid_ebx(0xf).

Second part is correct. I've muddled up the "max index" and the "number
of RMIDs" and need to add one to the max index when allocating data
structures.

  "If resctrl_arch_system_max_rmid_idx() returns 0 for a system with AET but no
   L3 monitoring, won't max_idx_limit be 0, leading to a zero-sized rmid_ptrs
   array?"

Can't happen. See above.

--- Patch 8: [PATCH v10 08/17] x86/resctrl: Enforce system RMID limit on AET event groups ---

  "Does bypassing rdt_get_l3_mon_config() when L3 events are disabled break
   Sub-NUMA Cluster (SNC) detection?"

Yes. But that's the right thing to do. Sub-NUMA Cluster detection is
only needed for the L3 monitor events. If those don't exist (or have
been disabled with clearcpuid) then the full range of RMIDs are
available for use by AET.

  "Will this result in an off-by-one truncation of the AET RMID limit if L3
   monitoring is disabled?"

Yes. One of the places where fix is needed: num_rmids = max_idx + 1

  "This is a pre-existing issue, but could this code encounter a divide-by-zero
   during boot if the maximum RMID index evaluates to 0?"

As above. Maximum RMID index can't be zero.

--- Patch 9: [PATCH v10 09/17] x86/resctrl: Add PMT registration API for AET enumeration callbacks ---

  "Is this description accurate given that CONFIG_X86_CPU_RESCTRL_INTEL_AET in
   Kconfig strictly enforces INTEL_PMT_TELEMETRY=y, preventing it from being
   built as a module?
   Additionally, if that Kconfig dependency were relaxed to allow it to be a
   module, wouldn't it immediately result in a link-time error? It appears
   intel_aet_get_events() still directly calls the exported function
   intel_pmt_get_regions_by_feature() rather than using the new callbacks."

Sashiko doesn't understand that I've broken some parts of this series
into "add some new infrastructure" and then subsequent patches "use that
new infrastructure". At this point in the series Sashiko correctly notes
that the telemetry driver is forced to be built-in. The issue raised
here is fixed by the point in the series that removes the Kconfig
restriction.

  "Do these static variables cause a -Wunused-but-set-variable compiler
   warning that breaks builds when CONFIG_WERROR=y is enabled?
   It appears intel_aet_get_events() was not updated in this patch to use the
   new get_feature callback, leaving these variables unread."

Ditto. Variables used in later patch.

  "Since these variables are assigned here but never read, and the registration
   function intel_aet_register_enumeration() doesn't seem to be called
   anywhere in the kernel, does this leave the variables and function as dead
   code?"

Yup. Dead code now. Gets used in later patch.

--- Patch 10: [PATCH v10 10/17] platform/x86/intel/pmt: Register enumeration functions with resctrl ---

  "The commit message indicates the goal is to allow INTEL_PMT_TELEMETRY to be a
   loadable module while resctrl remains built-in. However, are the callbacks
   registered here actually used by the resctrl subsystem?
   Looking at intel_aet_get_events() and intel_aet_exit() in 
   arch/x86/kernel/cpu/resctrl/intel_aet.c, it appears they still make direct
   calls to the exported PMT functions:"

Another "defined now, used in later patch" issue.

--- Patch 11: [PATCH v10 11/17] arm,x86/resctrl: Resolve INTEL_PMT_TELEMETRY symbols at runtime ---

  "Will the module reference acquired during mount permanently leak?"

At this point in the series the telemetry code is still built-in. So no
leak.

--- Patch 14: [PATCH v10 14/17] x86/resctrl: Export interface to report telemetry unbind/remove ---

  "Is it possible for domid to be 64 or greater when evaluated here?"

This patch is new in the series and I'd like discussion on the approach.
My first internal version used an aggressive approach. If any of the
MMIO regions were unmapped by unbind/remove, then I disabled all AET
event reporting. An AI review said this was overkill, and I should only
disable event counts for the unmapped MMIO regions. So I added this
"per-package" logic. Using a u64 as a bitmap of affected packages seems
like it will handle real systems. But now I need to checks for
theoretical systems with 64 or more packages. Seems like make-work.

--- Patch 15: [PATCH v10 15/17] platform/x86/intel/pmt: Inform resctrl when MMIO maps are being removed ---

  "Does this code safely handle non-PCI telemetry devices?"

No. But there are no such things. All telemetry devices are PCIe with
VSEC enumeration of events.

  "This is a pre-existing issue, but does dropping this kref before unregistering
   the sysfs file create a use-after-free window?"

  "This is also a pre-existing issue, but does removing the PMT device leave
   external consumers vulnerable to accessing unmapped memory?"

I can pass these two pre-existing issues to the telemetry maintainer.


--- Patch 16: [PATCH v10 16/17] x86/resctrl: Simplify Kconfig options for resctrl ---

  "Does using a private mount point here break mount propagation to child mount
   namespaces? This could isolate the final filesystem mount, preventing
   container engines or virtualization daemons from accessing the resctrl
   directory."

Yes. Deliberately so. Those things don't need access. Propagating the
mount to them makes the system harder to manage. Users must use the "-l"
option to umount to get the file system propely unmounted.

  "Will this 10-second sleep inside a oneshot service cause the systemctl start
   command in the udev rule to hang? This appears to synchronously block the udev
   event worker during boot."

I haven't observed a hang. The udev rule seems to be running
asynchronously (on Fedora). The system boot completes, and then
the sleep completes and the filesystem is mounted.

  "Does this rule make the mount entirely dependent on the pmt_telemetry module
   load event?"

I'd expect users to only install this rule on systems that support AET.

I'm open to better suggestions on how to trigger resctrl mount after the telemetry
module loads.

-Tony