| Message ID | 20191210084413.21957-1-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm/hotplug: Only respect mem= parameter during boot stage | expand |
On 10.12.19 09:44, Baoquan He wrote: > In commit 357b4da50a62 ("x86: respect memory size limiting via mem= > parameter") a global varialbe global max_mem_size is added to store > the value parsed from 'mem= ', then checked when memory region is > added. This truly stops those DIMM from being added into system memory > during boot-time. > > However, it also limits the later memory hotplug functionality. Any > memory board can't be hot added any more if its region is beyond the > max_mem_size. System will print error like below: > > [ 216.387164] acpi PNP0C80:02: add_memory failed > [ 216.389301] acpi PNP0C80:02: acpi_memory_enable_device() error > [ 216.392187] acpi PNP0C80:02: Enumeration failure > > From document of 'mem= ' parameter, it should be a restriction during > boot, but not impact the system memory adding/removing after booting. > > mem=nn[KMG] [KNL,BOOT] Force usage of a specific amount of memory > ... > > So fix it by also checking if it's during boot-time when restrict memory > adding. Otherwise, skip the restriction. > > Fixes: 357b4da50a62 ("x86: respect memory size limiting via mem= parameter") > Signed-off-by: Baoquan He <bhe@redhat.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
On 10/12/19 7:44 pm, Baoquan He wrote: > In commit 357b4da50a62 ("x86: respect memory size limiting via mem= > parameter") a global varialbe global max_mem_size is added to store typo ^^^ > the value parsed from 'mem= ', then checked when memory region is > added. This truly stops those DIMM from being added into system memory > during boot-time. > > However, it also limits the later memory hotplug functionality. Any > memory board can't be hot added any more if its region is beyond the > max_mem_size. System will print error like below: > > [ 216.387164] acpi PNP0C80:02: add_memory failed > [ 216.389301] acpi PNP0C80:02: acpi_memory_enable_device() error > [ 216.392187] acpi PNP0C80:02: Enumeration failure > > From document of 'mem= ' parameter, it should be a restriction during > boot, but not impact the system memory adding/removing after booting. > > mem=nn[KMG] [KNL,BOOT] Force usage of a specific amount of memory > ... > > So fix it by also checking if it's during boot-time when restrict memory > adding. Otherwise, skip the restriction. > The fix looks reasonable, but I don't get the use case. Booting with mem= is generally a debug option, is this for debugging memory hotplug + limited memory? Balbir
On 10.12.19 09:44, Baoquan He wrote: > In commit 357b4da50a62 ("x86: respect memory size limiting via mem= > parameter") a global varialbe global max_mem_size is added to store Only some nits: s/varialbe global/variable/ > the value parsed from 'mem= ', then checked when memory region is > added. This truly stops those DIMM from being added into system memory s/DIMM/DIMMs/ > during boot-time. > > However, it also limits the later memory hotplug functionality. Any > memory board can't be hot added any more if its region is beyond the s/memory board/DIMM/ ? s/hot added/hotplugged/ ? > max_mem_size. System will print error like below: "We will get errors like:" > > [ 216.387164] acpi PNP0C80:02: add_memory failed > [ 216.389301] acpi PNP0C80:02: acpi_memory_enable_device() error > [ 216.392187] acpi PNP0C80:02: Enumeration failure > > From document of 'mem= ' parameter, it should be a restriction during "the documentation of the" > boot, but not impact the system memory adding/removing after booting. "but not impact memory hotplug once booted." > > mem=nn[KMG] [KNL,BOOT] Force usage of a specific amount of memory > ... > > So fix it by also checking if it's during boot-time when restrict memory s/when restrict memory adding/restricting to add memory/ > adding. Otherwise, skip the restriction. > > Fixes: 357b4da50a62 ("x86: respect memory size limiting via mem= parameter") > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > mm/memory_hotplug.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 55ac23ef11c1..989707295d15 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -105,7 +105,11 @@ static struct resource *register_memory_resource(u64 start, u64 size) > unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > char *resource_name = "System RAM"; > > - if (start + size > max_mem_size) > + /* > + * Make sure value parsed from 'mem=' only restricts memory adding > + * during boot-time, so that memory hotplug won't be impacted. s/during boot-time/while booting/ Only some nits, thanks! Reviewed-by: David Hildenbrand <david@redhat.com>
On 12/10/19 at 08:24pm, Balbir Singh wrote: > > > On 10/12/19 7:44 pm, Baoquan He wrote: > > In commit 357b4da50a62 ("x86: respect memory size limiting via mem= > > parameter") a global varialbe global max_mem_size is added to store > typo ^^^ > > the value parsed from 'mem= ', then checked when memory region is > > added. This truly stops those DIMM from being added into system memory > > during boot-time. > > > > However, it also limits the later memory hotplug functionality. Any > > memory board can't be hot added any more if its region is beyond the > > max_mem_size. System will print error like below: > > > > [ 216.387164] acpi PNP0C80:02: add_memory failed > > [ 216.389301] acpi PNP0C80:02: acpi_memory_enable_device() error > > [ 216.392187] acpi PNP0C80:02: Enumeration failure > > > > From document of 'mem= ' parameter, it should be a restriction during > > boot, but not impact the system memory adding/removing after booting. > > > > mem=nn[KMG] [KNL,BOOT] Force usage of a specific amount of memory > > ... > > > > So fix it by also checking if it's during boot-time when restrict memory > > adding. Otherwise, skip the restriction. > > > > The fix looks reasonable, but I don't get the use case. Booting with mem= is > generally a debug option, is this for debugging memory hotplug + limited memory? Not really. My understanding is whether 'mem= ' is for debugging or not, it should only take effect during boot-time. Even though people add 'mem= ' for debugging only, they will have to reboot if they want to do futher testing, e.g memory hotplug, or any other feature which makes use of the memory hotplug mechanism to implement their functionality, like ballon, or pmem. Thanks Baoquan
On 10.12.19 10:24, Balbir Singh wrote: > > > On 10/12/19 7:44 pm, Baoquan He wrote: >> In commit 357b4da50a62 ("x86: respect memory size limiting via mem= >> parameter") a global varialbe global max_mem_size is added to store > typo ^^^ >> the value parsed from 'mem= ', then checked when memory region is >> added. This truly stops those DIMM from being added into system memory >> during boot-time. >> >> However, it also limits the later memory hotplug functionality. Any >> memory board can't be hot added any more if its region is beyond the >> max_mem_size. System will print error like below: >> >> [ 216.387164] acpi PNP0C80:02: add_memory failed >> [ 216.389301] acpi PNP0C80:02: acpi_memory_enable_device() error >> [ 216.392187] acpi PNP0C80:02: Enumeration failure >> >> From document of 'mem= ' parameter, it should be a restriction during >> boot, but not impact the system memory adding/removing after booting. >> >> mem=nn[KMG] [KNL,BOOT] Force usage of a specific amount of memory >> ... >> >> So fix it by also checking if it's during boot-time when restrict memory >> adding. Otherwise, skip the restriction. >> > > The fix looks reasonable, but I don't get the use case. Booting with mem= is > generally a debug option, is this for debugging memory hotplug + limited memory? Some people/companies use "mem=" along with KVM e.g., to avoid allocating memmaps for guest backing memory and to not expose it to the buddy across kexec's. The excluded physical memory is then memmap into the hypervisor process and KVM can deal with that. I can imagine that hotplug might be desirable as well for such use cases. > > Balbir >
On 10.12.19 10:36, David Hildenbrand wrote: > On 10.12.19 10:24, Balbir Singh wrote: >> >> >> On 10/12/19 7:44 pm, Baoquan He wrote: >>> In commit 357b4da50a62 ("x86: respect memory size limiting via mem= >>> parameter") a global varialbe global max_mem_size is added to store >> typo ^^^ >>> the value parsed from 'mem= ', then checked when memory region is >>> added. This truly stops those DIMM from being added into system memory >>> during boot-time. >>> >>> However, it also limits the later memory hotplug functionality. Any >>> memory board can't be hot added any more if its region is beyond the >>> max_mem_size. System will print error like below: >>> >>> [ 216.387164] acpi PNP0C80:02: add_memory failed >>> [ 216.389301] acpi PNP0C80:02: acpi_memory_enable_device() error >>> [ 216.392187] acpi PNP0C80:02: Enumeration failure >>> >>> From document of 'mem= ' parameter, it should be a restriction during >>> boot, but not impact the system memory adding/removing after booting. >>> >>> mem=nn[KMG] [KNL,BOOT] Force usage of a specific amount of memory >>> ... >>> >>> So fix it by also checking if it's during boot-time when restrict memory >>> adding. Otherwise, skip the restriction. >>> >> >> The fix looks reasonable, but I don't get the use case. Booting with mem= is >> generally a debug option, is this for debugging memory hotplug + limited memory? > > Some people/companies use "mem=" along with KVM e.g., to avoid > allocating memmaps for guest backing memory and to not expose it to the > buddy across kexec's. The excluded physical memory is then memmap into s/memmap/mmaped/ > the hypervisor process and KVM can deal with that. I can imagine that > hotplug might be desirable as well for such use cases. > >> >> Balbir >> > >
On 10/12/19 8:37 pm, David Hildenbrand wrote: > On 10.12.19 10:36, David Hildenbrand wrote: >> On 10.12.19 10:24, Balbir Singh wrote: >>> >>> >>> On 10/12/19 7:44 pm, Baoquan He wrote: >>>> In commit 357b4da50a62 ("x86: respect memory size limiting via mem= >>>> parameter") a global varialbe global max_mem_size is added to store >>> typo ^^^ >>>> the value parsed from 'mem= ', then checked when memory region is >>>> added. This truly stops those DIMM from being added into system memory >>>> during boot-time. >>>> >>>> However, it also limits the later memory hotplug functionality. Any >>>> memory board can't be hot added any more if its region is beyond the >>>> max_mem_size. System will print error like below: >>>> >>>> [ 216.387164] acpi PNP0C80:02: add_memory failed >>>> [ 216.389301] acpi PNP0C80:02: acpi_memory_enable_device() error >>>> [ 216.392187] acpi PNP0C80:02: Enumeration failure >>>> >>>> From document of 'mem= ' parameter, it should be a restriction during >>>> boot, but not impact the system memory adding/removing after booting. >>>> >>>> mem=nn[KMG] [KNL,BOOT] Force usage of a specific amount of memory >>>> ... >>>> >>>> So fix it by also checking if it's during boot-time when restrict memory >>>> adding. Otherwise, skip the restriction. >>>> >>> >>> The fix looks reasonable, but I don't get the use case. Booting with mem= is >>> generally a debug option, is this for debugging memory hotplug + limited memory? >> >> Some people/companies use "mem=" along with KVM e.g., to avoid >> allocating memmaps for guest backing memory and to not expose it to the >> buddy across kexec's. The excluded physical memory is then memmap into > > s/memmap/mmaped/ > >> the hypervisor process and KVM can deal with that. I can imagine that >> hotplug might be desirable as well for such use cases. Makes sense, sounds hacky, but it seems like mem= is no longer a debug option Acked-by: Balbir Singh <bsingharora@gmail.com> >> >>> >>> Balbir >>> >> >> > >
On Tue 10-12-19 10:36:19, David Hildenbrand wrote: > On 10.12.19 10:24, Balbir Singh wrote: > > > > > > On 10/12/19 7:44 pm, Baoquan He wrote: > >> In commit 357b4da50a62 ("x86: respect memory size limiting via mem= > >> parameter") a global varialbe global max_mem_size is added to store > > typo ^^^ > >> the value parsed from 'mem= ', then checked when memory region is > >> added. This truly stops those DIMM from being added into system memory > >> during boot-time. > >> > >> However, it also limits the later memory hotplug functionality. Any > >> memory board can't be hot added any more if its region is beyond the > >> max_mem_size. System will print error like below: > >> > >> [ 216.387164] acpi PNP0C80:02: add_memory failed > >> [ 216.389301] acpi PNP0C80:02: acpi_memory_enable_device() error > >> [ 216.392187] acpi PNP0C80:02: Enumeration failure > >> > >> From document of 'mem= ' parameter, it should be a restriction during > >> boot, but not impact the system memory adding/removing after booting. > >> > >> mem=nn[KMG] [KNL,BOOT] Force usage of a specific amount of memory > >> ... > >> > >> So fix it by also checking if it's during boot-time when restrict memory > >> adding. Otherwise, skip the restriction. > >> > > > > The fix looks reasonable, but I don't get the use case. Booting with mem= is > > generally a debug option, is this for debugging memory hotplug + limited memory? > > Some people/companies use "mem=" along with KVM e.g., to avoid > allocating memmaps for guest backing memory and to not expose it to the > buddy across kexec's. The excluded physical memory is then memmap into > the hypervisor process and KVM can deal with that. I can imagine that > hotplug might be desirable as well for such use cases. If this is really the usecase (it makes some sense to me) then it should be folded into the changelog. Because the real semantic is not really clear as I've pointed out in the previous version of this patch [1]. The restriction to BOOT is documented since ever long before the memory hotplug was a thing. Thanks! [1] Btw. it would have been much better if you posted the version 2 only after all the feedback got discussed properly.
On 12/10/19 at 10:50am, Michal Hocko wrote: > On Tue 10-12-19 10:36:19, David Hildenbrand wrote: > > On 10.12.19 10:24, Balbir Singh wrote: > > > > > > > > > On 10/12/19 7:44 pm, Baoquan He wrote: > > >> In commit 357b4da50a62 ("x86: respect memory size limiting via mem= > > >> parameter") a global varialbe global max_mem_size is added to store > > > typo ^^^ > > >> the value parsed from 'mem= ', then checked when memory region is > > >> added. This truly stops those DIMM from being added into system memory > > >> during boot-time. > > >> > > >> However, it also limits the later memory hotplug functionality. Any > > >> memory board can't be hot added any more if its region is beyond the > > >> max_mem_size. System will print error like below: > > >> > > >> [ 216.387164] acpi PNP0C80:02: add_memory failed > > >> [ 216.389301] acpi PNP0C80:02: acpi_memory_enable_device() error > > >> [ 216.392187] acpi PNP0C80:02: Enumeration failure > > >> > > >> From document of 'mem= ' parameter, it should be a restriction during > > >> boot, but not impact the system memory adding/removing after booting. > > >> > > >> mem=nn[KMG] [KNL,BOOT] Force usage of a specific amount of memory > > >> ... > > >> > > >> So fix it by also checking if it's during boot-time when restrict memory > > >> adding. Otherwise, skip the restriction. > > >> > > > > > > The fix looks reasonable, but I don't get the use case. Booting with mem= is > > > generally a debug option, is this for debugging memory hotplug + limited memory? > > > > Some people/companies use "mem=" along with KVM e.g., to avoid > > allocating memmaps for guest backing memory and to not expose it to the > > buddy across kexec's. The excluded physical memory is then memmap into > > the hypervisor process and KVM can deal with that. I can imagine that > > hotplug might be desirable as well for such use cases. > > If this is really the usecase (it makes some sense to me) then it should > be folded into the changelog. Because the real semantic is not really > clear as I've pointed out in the previous version of this patch [1]. > The restriction to BOOT is documented since ever long before the memory > hotplug was a thing. I will hold a while and post v3 to take this into log. > > [1] Btw. it would have been much better if you posted the version 2 only > after all the feedback got discussed properly. You could have replied to wrong person. I don't know the use case David told. My motivation was to make memory hotplug not impacted after boot. I thought I have got your question and answered it. I will wait a little longer when post next time. Thanks Baoquan
On Tue 10-12-19 18:11:00, Baoquan He wrote: > On 12/10/19 at 10:50am, Michal Hocko wrote: [...] > > [1] Btw. it would have been much better if you posted the version 2 only > > after all the feedback got discussed properly. > > You could have replied to wrong person. > > I don't know the use case David told. My motivation was to make memory hotplug > not impacted after boot. I thought I have got your question and answered > it. I will wait a little longer when post next time. let me reply to your original email.
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 55ac23ef11c1..989707295d15 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -105,7 +105,11 @@ static struct resource *register_memory_resource(u64 start, u64 size) unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; char *resource_name = "System RAM"; - if (start + size > max_mem_size) + /* + * Make sure value parsed from 'mem=' only restricts memory adding + * during boot-time, so that memory hotplug won't be impacted. + */ + if (start + size > max_mem_size && system_state < SYSTEM_RUNNING) return ERR_PTR(-E2BIG); /*
In commit 357b4da50a62 ("x86: respect memory size limiting via mem= parameter") a global varialbe global max_mem_size is added to store the value parsed from 'mem= ', then checked when memory region is added. This truly stops those DIMM from being added into system memory during boot-time. However, it also limits the later memory hotplug functionality. Any memory board can't be hot added any more if its region is beyond the max_mem_size. System will print error like below: [ 216.387164] acpi PNP0C80:02: add_memory failed [ 216.389301] acpi PNP0C80:02: acpi_memory_enable_device() error [ 216.392187] acpi PNP0C80:02: Enumeration failure From document of 'mem= ' parameter, it should be a restriction during boot, but not impact the system memory adding/removing after booting. mem=nn[KMG] [KNL,BOOT] Force usage of a specific amount of memory ... So fix it by also checking if it's during boot-time when restrict memory adding. Otherwise, skip the restriction. Fixes: 357b4da50a62 ("x86: respect memory size limiting via mem= parameter") Signed-off-by: Baoquan He <bhe@redhat.com> --- mm/memory_hotplug.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)