Re: [PATCH v3 13/38] x86/resctrl: Move resctrl types to a separate header

From: Reinette Chatre
Date: Fri Jun 28 2024 - 12:46:37 EST


Hi James,

On 6/14/24 8:00 AM, James Morse wrote:
When resctrl is fully factored into core and per-arch code, each arch
will need to use some resctrl common definitions in order to define its
own specializations and helpers. Following conventional practice, it
would be desirable to put the dependent arch definitions in an
<asm/resctrl.h> header that is included by the common <linux/resctrl.h>
header. However, this can make it awkward to avoid a circular
dependency between <linux/resctrl.h> and the arch header.

To avoid such dependencies, move the affected common types and
constants into a new header that does not need to depend on
<linux/resctrl.h> or on the arch headers.

The same logic applies to the monitor-configuration defines, move these
too.

Some kind of enumeration for events is needed between the filesystem
and architecture code. Take the x86 definition as its convenient for
x86.

The definition of enum resctrl_event_id is need to allow the architecture

"is need" -> "is needed" ?

code to define resctrl_arch_event_is_free_running(),

Cannot find resctrl_arch_event_is_free_running()

resctrl_arch_set_cdp_enabled(), resctrl_arch_mon_ctx_alloc() and

resctrl_arch_set_cdp_enabled() should not need enum resctrl_event_id


resctrl_arch_mon_ctx_free().

The definition of enum resctrl_res_level is needed to allow the
architecture code to define resctrl_arch_set_cdp_enabled() and
resctrl_arch_get_cdp_enabled().

The bits for mbm_local_bytes_config et al are ABI, and must be the same
on all architectures. These are documented in
Documentation/arch/x86/resctrl.rst

The maintainers entry for these headers was missed when resctrl.h was
created. Add a wildcard entry to match both resctrl.h and
resctrl_types.h.

Signed-off-by: James Morse <james.morse@xxxxxxx>
Tested-by: Peter Newman <peternewman@xxxxxxxxxx>
Tested-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx>
---
Changes since v2:
* Added to the commit message why each of these things is necessary.
* Moved the enum resctrl_conf_type back to resctrl.h - this week arm's
CDP emulation code gets away without this...

Changes since v1:
* [Commit message only] Rewrite commit message to clarify the the
rationale for refactoring the headers in this way.
---
MAINTAINERS | 1 +
arch/x86/kernel/cpu/resctrl/internal.h | 24 ------------
include/linux/resctrl.h | 21 +---------
include/linux/resctrl_types.h | 54 ++++++++++++++++++++++++++

Considering the motivation I also expected to see a change in
arch/x86/include/asm/resctrl.h that adds the #include of the new file.

Reinette