| 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
| ||
|
Message-ID: <20250815165639.00007354@huawei.com>
Date: Fri, 15 Aug 2025 16:56:39 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Dave Jiang <dave.jiang@...el.com>
CC: <linux-cxl@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <gregkh@...uxfoundation.org>,
<rafael@...nel.org>, <dakr@...nel.org>, <dave@...olabs.net>,
<alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
<ira.weiny@...el.com>, <dan.j.williams@...el.com>,
<marc.herbert@...ux.intel.com>, <akpm@...ux-foundation.org>,
<david@...hat.com>
Subject: Re: [PATCH 3/4] cxl, acpi/hmat: Update CXL access coordinates
directly instead of through HMAT
On Fri, 15 Aug 2025 08:35:31 -0700
Dave Jiang <dave.jiang@...el.com> wrote:
> On 8/15/25 6:31 AM, Jonathan Cameron wrote:
> > On Thu, 14 Aug 2025 10:16:49 -0700
> > Dave Jiang <dave.jiang@...el.com> wrote:
> >
> >> The current implementation of CXL memory hotplug notifier gets called
> >> before the HMAT memory hotplug notifier. The CXL driver calculates the
> >> access coordinates (bandwidth and latency values) for the CXL end to
> >> end path (i.e. CPU to endpoint). When the CXL region is onlined, the CXL
> >> memory hotplug notifier writes the access coordinates to the HMAT target
> >> structs. Then the HMAT memory hotplug notifier is called and it creates
> >> the access coordinates for the node sysfs attributes.
> >>
> >> The original intent of the 'ext_updated' flag in HMAT handling code was to
> >> stop HMAT memory hotplug callback from clobbering the access coordinates
> >> after CXL has injected its calculated coordinates and replaced the generic
> >> target access coordinates provided by the HMAT table in the HMAT target
> >> structs. However the flag is hacky at best and blocks the updates from
> >> other CXL regions that are onlined in the same node later on.
> >
> > After all removed, or when a second region onlined in that node whilst the
> > first is still online? In that second case I think we should not update
> > the access properties as that would surprise anything already using the
> > earlier one.
>
> Are you thinking we'll need some sort of ref counting on the node?
Was more a question on the intent, but indeed some sort of refcount would be
needed to tear down cleanly. Might get messy though as what do we do if
two regions are added with different characteristics then the first one
is removed. Do we update?
Meh. This is a corner case and if we get different properties in one CFMWS
as a common thing then we need to figure out how to spin more NUMA nodes.
So I'm not sure this isn't a 'won't fix until the case turns out to matter'
thing.
Jonathan
>
> DJ
> >
> > Jonathan
> >
> >> Remove the
> >> 'ext_updated' flag usage and just update the access coordinates for the
> >> nodes directly without touching HMAT target data.
> >>
> >> The hotplug memory callback ordering is changed. Instead of changing CXL,
> >> move HMAT back so there's room for the levels rather than have CXL share
> >> the same level as SLAB_CALLBACK_PRI. The change will resulting in the CXL
> >> callback to be executed after the HMAT callback.
> >>
> >> With the change, the CXL hotplug memory notifier runs after the HMAT
> >> callback. The HMAT callback will create the node sysfs attributes for
> >> access coordinates. The CXL callback will write the access coordinates to
> >> the now created node sysfs attributes directly and will not pollute the
> >> HMAT target values.
> >>
> >> Fixes: debdce20c4f2 ("cxl/region: Deal with numa nodes not enumerated by SRAT")
> >> Tested-by: Marc Herbert <marc.herbert@...ux.intel.com>
> >> Signed-off-by: Dave Jiang <dave.jiang@...el.com>
> >> ---
> >> drivers/acpi/numa/hmat.c | 6 ------
> >> drivers/cxl/core/cdat.c | 5 -----
> >> drivers/cxl/core/core.h | 1 -
> >> drivers/cxl/core/region.c | 10 ++--------
> >> include/linux/memory.h | 2 +-
> >> 5 files changed, 3 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> >> index 4958301f5417..5d32490dc4ab 100644
> >> --- a/drivers/acpi/numa/hmat.c
> >> +++ b/drivers/acpi/numa/hmat.c
> >> @@ -74,7 +74,6 @@ struct memory_target {
> >> struct node_cache_attrs cache_attrs;
> >> u8 gen_port_device_handle[ACPI_SRAT_DEVICE_HANDLE_SIZE];
> >> bool registered;
> >> - bool ext_updated; /* externally updated */
> >> };
> >>
> >> struct memory_initiator {
> >> @@ -391,7 +390,6 @@ int hmat_update_target_coordinates(int nid, struct access_coordinate *coord,
> >> coord->read_bandwidth, access);
> >> hmat_update_target_access(target, ACPI_HMAT_WRITE_BANDWIDTH,
> >> coord->write_bandwidth, access);
> >> - target->ext_updated = true;
> >>
> >> return 0;
> >> }
> >> @@ -773,10 +771,6 @@ static void hmat_update_target_attrs(struct memory_target *target,
> >> u32 best = 0;
> >> int i;
> >>
> >> - /* Don't update if an external agent has changed the data. */
> >> - if (target->ext_updated)
> >> - return;
> >> -
> >> /* Don't update for generic port if there's no device handle */
> >> if ((access == NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL ||
> >> access == NODE_ACCESS_CLASS_GENPORT_SINK_CPU) &&
> >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> >> index c0af645425f4..c891fd618cfd 100644
> >> --- a/drivers/cxl/core/cdat.c
> >> +++ b/drivers/cxl/core/cdat.c
> >> @@ -1081,8 +1081,3 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
> >> {
> >> return hmat_update_target_coordinates(nid, &cxlr->coord[access], access);
> >> }
> >> -
> >> -bool cxl_need_node_perf_attrs_update(int nid)
> >> -{
> >> - return !acpi_node_backed_by_real_pxm(nid);
> >> -}
> >> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> >> index 2669f251d677..a253d308f3c9 100644
> >> --- a/drivers/cxl/core/core.h
> >> +++ b/drivers/cxl/core/core.h
> >> @@ -139,7 +139,6 @@ long cxl_pci_get_latency(struct pci_dev *pdev);
> >> int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
> >> int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
> >> enum access_coordinate_class access);
> >> -bool cxl_need_node_perf_attrs_update(int nid);
> >> int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
> >> struct access_coordinate *c);
> >>
> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >> index 71cc42d05248..1580e19f13a5 100644
> >> --- a/drivers/cxl/core/region.c
> >> +++ b/drivers/cxl/core/region.c
> >> @@ -2442,14 +2442,8 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
> >>
> >> for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
> >> if (cxlr->coord[i].read_bandwidth) {
> >> - rc = 0;
> >> - if (cxl_need_node_perf_attrs_update(nid))
> >> - node_set_perf_attrs(nid, &cxlr->coord[i], i);
> >> - else
> >> - rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
> >> -
> >> - if (rc == 0)
> >> - cset++;
> >> + node_update_perf_attrs(nid, &cxlr->coord[i], i);
> >> + cset++;
> >> }
> >> }
> >>
> >> diff --git a/include/linux/memory.h b/include/linux/memory.h
> >> index 02314723e5bd..b41872c478e3 100644
> >> --- a/include/linux/memory.h
> >> +++ b/include/linux/memory.h
> >> @@ -120,8 +120,8 @@ struct mem_section;
> >> */
> >> #define DEFAULT_CALLBACK_PRI 0
> >> #define SLAB_CALLBACK_PRI 1
> >> -#define HMAT_CALLBACK_PRI 2
> >> #define CXL_CALLBACK_PRI 5
> >> +#define HMAT_CALLBACK_PRI 6
> >> #define MM_COMPUTE_BATCH_PRI 10
> >> #define CPUSET_CALLBACK_PRI 10
> >> #define MEMTIER_HOTPLUG_PRI 100
> >
>
>
Powered by blists - more mailing lists