| Message ID | 163553708697.2509508.16523059414830959692.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce acpi_table_parse_cedt and extra nodes for CXL.mem | expand |
On Fri, 29 Oct 2021 12:51:27 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > Hi Rafael, > > While reviewing "[PATCH v3] ACPI: NUMA: Add a node and memblk for each > CFMWS not in SRAT" [1]. I noticed that it was open coding CEDT sub-table > parsing in a similar fashion as drivers/cxl/acpi.c. The driver open > coded the parsing because the ACPI sub-table helpers are marked __init. > In order to avoid the ongoing maintenance burden of a split between > "early" and "late" ACPI sub-table parsing this series proposes to make > those helpers available to drivers. > > The savings in drivers/cxl/ are: > > drivers/cxl/Kconfig | 1 > drivers/cxl/acpi.c | 234 +++++++++++++++++++-------------------------------- > 2 files changed, 88 insertions(+), 147 deletions(-) > > ...and 15 lines new code not added are saved in this new version of > "ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT". > > Let me know if this looks ok to you and I can carry it in the CXL tree > (i.e. after the merge window, for v5.17 consideration). > > [1]: https://lore.kernel.org/r/20211019050908.449231-1-alison.schofield@intel.com Is it worth the complexity of the __init_or_acpilib and export part? Seems like a fiddly dance for what looks to be minor savings... Jonathan > > --- > > Alison Schofield (1): > ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT > > Dan Williams (5): > ACPI: Keep sub-table parsing infrastructure available for modules > ACPI: Teach ACPI table parsing about the CEDT header format > ACPI: Add a context argument for table parsing handlers > cxl/acpi: Convert CFMWS parsing to ACPI sub-table helpers > cxl/test: Mock acpi_table_parse_cedt() > > > drivers/acpi/Kconfig | 3 + > drivers/acpi/numa/srat.c | 59 ++++++++++ > drivers/acpi/tables.c | 87 +++++++++++---- > drivers/cxl/Kconfig | 1 > drivers/cxl/acpi.c | 237 ++++++++++++++++------------------------- > include/linux/acpi.h | 34 +++++- > tools/testing/cxl/Kbuild | 3 - > tools/testing/cxl/test/cxl.c | 68 ++++++++---- > tools/testing/cxl/test/mock.c | 30 ++--- > tools/testing/cxl/test/mock.h | 6 + > 10 files changed, 304 insertions(+), 224 deletions(-) > > base-commit: c6d7e1341cc99ba49df1384c8c5b3f534a5463b1
On Mon, Nov 1, 2021 at 5:01 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Fri, 29 Oct 2021 12:51:27 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > Hi Rafael, > > > > While reviewing "[PATCH v3] ACPI: NUMA: Add a node and memblk for each > > CFMWS not in SRAT" [1]. I noticed that it was open coding CEDT sub-table > > parsing in a similar fashion as drivers/cxl/acpi.c. The driver open > > coded the parsing because the ACPI sub-table helpers are marked __init. > > In order to avoid the ongoing maintenance burden of a split between > > "early" and "late" ACPI sub-table parsing this series proposes to make > > those helpers available to drivers. > > > > The savings in drivers/cxl/ are: > > > > drivers/cxl/Kconfig | 1 > > drivers/cxl/acpi.c | 234 +++++++++++++++++++-------------------------------- > > 2 files changed, 88 insertions(+), 147 deletions(-) > > > > ...and 15 lines new code not added are saved in this new version of > > "ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT". > > > > Let me know if this looks ok to you and I can carry it in the CXL tree > > (i.e. after the merge window, for v5.17 consideration). > > > > [1]: https://lore.kernel.org/r/20211019050908.449231-1-alison.schofield@intel.com > > Is it worth the complexity of the __init_or_acpilib and export part? > Seems like a fiddly dance for what looks to be minor savings... It follows the __initdata_or_meminfo precedent that identifies data that is normally __init unless a specific driver needs it. The lesson from the tinyconfig effort was that image size dies a death of many cuts unless care is taken to preserve minor savings. Yes, it's likely trivial in this case, but it's at least a gesture to avoid bloating the kernel image size unnecessarily when the kernel has gotten by so long with this infrastructure being purely __init.
On Mon, 1 Nov 2021 20:41:34 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > On Mon, Nov 1, 2021 at 5:01 AM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > On Fri, 29 Oct 2021 12:51:27 -0700 > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > Hi Rafael, > > > > > > While reviewing "[PATCH v3] ACPI: NUMA: Add a node and memblk for each > > > CFMWS not in SRAT" [1]. I noticed that it was open coding CEDT sub-table > > > parsing in a similar fashion as drivers/cxl/acpi.c. The driver open > > > coded the parsing because the ACPI sub-table helpers are marked __init. > > > In order to avoid the ongoing maintenance burden of a split between > > > "early" and "late" ACPI sub-table parsing this series proposes to make > > > those helpers available to drivers. > > > > > > The savings in drivers/cxl/ are: > > > > > > drivers/cxl/Kconfig | 1 > > > drivers/cxl/acpi.c | 234 +++++++++++++++++++-------------------------------- > > > 2 files changed, 88 insertions(+), 147 deletions(-) > > > > > > ...and 15 lines new code not added are saved in this new version of > > > "ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT". > > > > > > Let me know if this looks ok to you and I can carry it in the CXL tree > > > (i.e. after the merge window, for v5.17 consideration). > > > > > > [1]: https://lore.kernel.org/r/20211019050908.449231-1-alison.schofield@intel.com > > > > Is it worth the complexity of the __init_or_acpilib and export part? > > Seems like a fiddly dance for what looks to be minor savings... > > It follows the __initdata_or_meminfo precedent that identifies data > that is normally __init unless a specific driver needs it. The lesson > from the tinyconfig effort was that image size dies a death of many > cuts unless care is taken to preserve minor savings. Yes, it's likely > trivial in this case, but it's at least a gesture to avoid bloating > the kernel image size unnecessarily when the kernel has gotten by so > long with this infrastructure being purely __init. I'm in favor avoiding bloat, but this is ACPI code so rarely very small machines and very like that all distros will turn it on anyway on basis they will want to support CXL (hopefully!) I guess let's see what Rafael's opinion is. I don't feel that strongly about it if general view is that it is worth the small amount of complexity. Jonathan
On Tue, Nov 2, 2021 at 6:44 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Mon, 1 Nov 2021 20:41:34 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > On Mon, Nov 1, 2021 at 5:01 AM Jonathan Cameron > > <Jonathan.Cameron@huawei.com> wrote: > > > > > > On Fri, 29 Oct 2021 12:51:27 -0700 > > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > > > Hi Rafael, > > > > > > > > While reviewing "[PATCH v3] ACPI: NUMA: Add a node and memblk for each > > > > CFMWS not in SRAT" [1]. I noticed that it was open coding CEDT sub-table > > > > parsing in a similar fashion as drivers/cxl/acpi.c. The driver open > > > > coded the parsing because the ACPI sub-table helpers are marked __init. > > > > In order to avoid the ongoing maintenance burden of a split between > > > > "early" and "late" ACPI sub-table parsing this series proposes to make > > > > those helpers available to drivers. > > > > > > > > The savings in drivers/cxl/ are: > > > > > > > > drivers/cxl/Kconfig | 1 > > > > drivers/cxl/acpi.c | 234 +++++++++++++++++++-------------------------------- > > > > 2 files changed, 88 insertions(+), 147 deletions(-) > > > > > > > > ...and 15 lines new code not added are saved in this new version of > > > > "ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT". > > > > > > > > Let me know if this looks ok to you and I can carry it in the CXL tree > > > > (i.e. after the merge window, for v5.17 consideration). > > > > > > > > [1]: https://lore.kernel.org/r/20211019050908.449231-1-alison.schofield@intel.com > > > > > > Is it worth the complexity of the __init_or_acpilib and export part? > > > Seems like a fiddly dance for what looks to be minor savings... > > > > It follows the __initdata_or_meminfo precedent that identifies data > > that is normally __init unless a specific driver needs it. The lesson > > from the tinyconfig effort was that image size dies a death of many > > cuts unless care is taken to preserve minor savings. Yes, it's likely > > trivial in this case, but it's at least a gesture to avoid bloating > > the kernel image size unnecessarily when the kernel has gotten by so > > long with this infrastructure being purely __init. > > I'm in favor avoiding bloat, but this is ACPI code so rarely very small machines > and very like that all distros will turn it on anyway on basis they will want > to support CXL (hopefully!) > > I guess let's see what Rafael's opinion is. I don't feel that strongly about > it if general view is that it is worth the small amount of complexity. The general ACPI changes in this series are fine with me, so Dan please feel free to add Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> to it. Thanks!
Hi Rafael, While reviewing "[PATCH v3] ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT" [1]. I noticed that it was open coding CEDT sub-table parsing in a similar fashion as drivers/cxl/acpi.c. The driver open coded the parsing because the ACPI sub-table helpers are marked __init. In order to avoid the ongoing maintenance burden of a split between "early" and "late" ACPI sub-table parsing this series proposes to make those helpers available to drivers. The savings in drivers/cxl/ are: drivers/cxl/Kconfig | 1 drivers/cxl/acpi.c | 234 +++++++++++++++++++-------------------------------- 2 files changed, 88 insertions(+), 147 deletions(-) ...and 15 lines new code not added are saved in this new version of "ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT". Let me know if this looks ok to you and I can carry it in the CXL tree (i.e. after the merge window, for v5.17 consideration). [1]: https://lore.kernel.org/r/20211019050908.449231-1-alison.schofield@intel.com --- Alison Schofield (1): ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT Dan Williams (5): ACPI: Keep sub-table parsing infrastructure available for modules ACPI: Teach ACPI table parsing about the CEDT header format ACPI: Add a context argument for table parsing handlers cxl/acpi: Convert CFMWS parsing to ACPI sub-table helpers cxl/test: Mock acpi_table_parse_cedt() drivers/acpi/Kconfig | 3 + drivers/acpi/numa/srat.c | 59 ++++++++++ drivers/acpi/tables.c | 87 +++++++++++---- drivers/cxl/Kconfig | 1 drivers/cxl/acpi.c | 237 ++++++++++++++++------------------------- include/linux/acpi.h | 34 +++++- tools/testing/cxl/Kbuild | 3 - tools/testing/cxl/test/cxl.c | 68 ++++++++---- tools/testing/cxl/test/mock.c | 30 ++--- tools/testing/cxl/test/mock.h | 6 + 10 files changed, 304 insertions(+), 224 deletions(-) base-commit: c6d7e1341cc99ba49df1384c8c5b3f534a5463b1