| Message ID | 20220708145424.1848572-2-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device tree based NUMA support for Arm - Part#2 | expand |
On 08.07.2022 16:54, Wei Chen wrote: > x86 provides a mem_hotplug variable to maintain the memory hotplug > end address. We want to move some codes from x86 to common, so that > it can be reused by other architectures. But not all architectures > have supported memory hotplug. So in this patch, we introduce this > helper to replace mem_hotplug direct access in these codes. This > will give the ability of stubbing this helper to those architectures > without memory hotplug support. What remains unclear to me is why Arm can't simply gain the necessary variable. Sooner or later you'll want to support hotplug there anyway, I suppose. Mechanically the change is fine. Albeit maybe "top" instead of "boundary", and maybe also pass "node" even if x86 doesn't need it? Jan > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > v1 -> v2: > 1. Refine the commit message. > 2. Merge v1 patch#9,10 into one patch. Introduce the new functions > in the same patch that this patch will be used first time. > 3. Fold if ( end > mem_hotplug ) to mem_hotplug_update_boundary, > in this case, we can drop mem_hotplug_boundary. > --- > xen/arch/x86/include/asm/mm.h | 6 ++++++ > xen/arch/x86/srat.c | 3 +-- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h > index 07b59c982b..b3dfbdb52b 100644 > --- a/xen/arch/x86/include/asm/mm.h > +++ b/xen/arch/x86/include/asm/mm.h > @@ -476,6 +476,12 @@ static inline int get_page_and_type(struct page_info *page, > > extern paddr_t mem_hotplug; > > +static inline void mem_hotplug_update_boundary(paddr_t end) > +{ > + if ( end > mem_hotplug ) > + mem_hotplug = end; > +} > + > /****************************************************************************** > * With shadow pagetables, the different kinds of address start > * to get get confusing. > diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c > index b62a152911..f53431f5e8 100644 > --- a/xen/arch/x86/srat.c > +++ b/xen/arch/x86/srat.c > @@ -418,8 +418,7 @@ acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) > memblk_nodeid[num_node_memblks] = node; > if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) { > __set_bit(num_node_memblks, memblk_hotplug); > - if (end > mem_hotplug) > - mem_hotplug = end; > + mem_hotplug_update_boundary(end); > } > num_node_memblks++; > }
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年7月12日 19:54 > To: Wei Chen <Wei.Chen@arm.com> > Cc: nd <nd@arm.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau > Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v2 1/9] xen/x86: introduce a helper to update memory > hotplug end > > On 08.07.2022 16:54, Wei Chen wrote: > > x86 provides a mem_hotplug variable to maintain the memory hotplug > > end address. We want to move some codes from x86 to common, so that > > it can be reused by other architectures. But not all architectures > > have supported memory hotplug. So in this patch, we introduce this > > helper to replace mem_hotplug direct access in these codes. This > > will give the ability of stubbing this helper to those architectures > > without memory hotplug support. > > What remains unclear to me is why Arm can't simply gain the necessary > variable. Sooner or later you'll want to support hotplug there anyway, I am not sure my limited memory hotplug knowledge can answer your question or not. As memory hotplug depends on hardware and firmware support. Now for Arm, we only have ACPI firmware that can notify OS through GED event (not very sure). But ACPI and device tree couldn't be enabled at the same time from OS booting. In device tree based NUMA, we will enable device tree, ACPI service will not be initialized, that means we can not get notification of memory hotplug events from ACPI firmware. Of course, adding GED device nodes to the device tree, and getting these events through GED interrupts should not be a big technical problem. But there may be other reasons, and no one is planning to add memory hotplug support to the device tree (perhaps need an ACPI-like specification or a firmware abstraction interface). So if we want to implement Xen Arm memory hotplug, it should also start from ACPI. So currently even if we gain the variable in Arm, it will not be used. Device tree does not have property to indicate a memory block can be hotplug or not. > I suppose. Mechanically the change is fine. Albeit maybe "top" instead > of "boundary", and maybe also pass "node" even if x86 doesn't need it? > Sorry, I am not very clear about these comments: It makes sense to use mem_hotplug_update_top instead of mem_hotplug_update_boundary. But how can I understand pass "node"? did you mean mem_hotplug_update_top(node, end)? But mem_hotplug is a global top for memory hotplug ranges. I don't think pass "node" to this function can be useful. Cheers, Wei Chen > Jan > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > v1 -> v2: > > 1. Refine the commit message. > > 2. Merge v1 patch#9,10 into one patch. Introduce the new functions > > in the same patch that this patch will be used first time. > > 3. Fold if ( end > mem_hotplug ) to mem_hotplug_update_boundary, > > in this case, we can drop mem_hotplug_boundary. > > --- > > xen/arch/x86/include/asm/mm.h | 6 ++++++ > > xen/arch/x86/srat.c | 3 +-- > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/x86/include/asm/mm.h > b/xen/arch/x86/include/asm/mm.h > > index 07b59c982b..b3dfbdb52b 100644 > > --- a/xen/arch/x86/include/asm/mm.h > > +++ b/xen/arch/x86/include/asm/mm.h > > @@ -476,6 +476,12 @@ static inline int get_page_and_type(struct > page_info *page, > > > > extern paddr_t mem_hotplug; > > > > +static inline void mem_hotplug_update_boundary(paddr_t end) > > +{ > > + if ( end > mem_hotplug ) > > + mem_hotplug = end; > > +} > > + > > > /************************************************************************* > ***** > > * With shadow pagetables, the different kinds of address start > > * to get get confusing. > > diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c > > index b62a152911..f53431f5e8 100644 > > --- a/xen/arch/x86/srat.c > > +++ b/xen/arch/x86/srat.c > > @@ -418,8 +418,7 @@ acpi_numa_memory_affinity_init(const struct > acpi_srat_mem_affinity *ma) > > memblk_nodeid[num_node_memblks] = node; > > if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) { > > __set_bit(num_node_memblks, memblk_hotplug); > > - if (end > mem_hotplug) > > - mem_hotplug = end; > > + mem_hotplug_update_boundary(end); > > } > > num_node_memblks++; > > }
On 13.07.2022 10:22, Wei Chen wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年7月12日 19:54 >> >> On 08.07.2022 16:54, Wei Chen wrote: >>> x86 provides a mem_hotplug variable to maintain the memory hotplug >>> end address. We want to move some codes from x86 to common, so that >>> it can be reused by other architectures. But not all architectures >>> have supported memory hotplug. So in this patch, we introduce this >>> helper to replace mem_hotplug direct access in these codes. This >>> will give the ability of stubbing this helper to those architectures >>> without memory hotplug support. >> >> What remains unclear to me is why Arm can't simply gain the necessary >> variable. Sooner or later you'll want to support hotplug there anyway, > > I am not sure my limited memory hotplug knowledge can answer your question > or not. As memory hotplug depends on hardware and firmware support. Now > for Arm, we only have ACPI firmware that can notify OS through GED event > (not very sure). But ACPI and device tree couldn't be enabled at the same > time from OS booting. In device tree based NUMA, we will enable device > tree, ACPI service will not be initialized, that means we can not get > notification of memory hotplug events from ACPI firmware. > > Of course, adding GED device nodes to the device tree, and getting these > events through GED interrupts should not be a big technical problem. But > there may be other reasons, and no one is planning to add memory hotplug > support to the device tree (perhaps need an ACPI-like specification or a > firmware abstraction interface). So if we want to implement Xen Arm memory > hotplug, it should also start from ACPI. So currently even if we gain the > variable in Arm, it will not be used. Device tree does not have property > to indicate a memory block can be hotplug or not. But ACPI is possible to be enabled for Arm64, and hence hotplug could be made work that way. Therefore I view the variable as potentially useful on Arm as well, and hence introducing the variable might be less trouble than introducing the per-arch helper. >> I suppose. Mechanically the change is fine. Albeit maybe "top" instead >> of "boundary", and maybe also pass "node" even if x86 doesn't need it? >> > > Sorry, I am not very clear about these comments: > It makes sense to use mem_hotplug_update_top instead > of mem_hotplug_update_boundary. But how can I understand pass "node"? > did you mean mem_hotplug_update_top(node, end)? But mem_hotplug is > a global top for memory hotplug ranges. I don't think pass "node" > to this function can be useful. Please separate "current implementation" from "abstract model". In the latter it would seem quite reasonable to me to have per-node upper bounds (or even per-node ranges). Therefore adding node (and, on top of what I did suggest before, region start) to the parameters of the new per-arch hook would seem a good move to me, even if at present only / at most the "end" parameter would actually be used. Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年7月13日 16:46 > To: Wei Chen <Wei.Chen@arm.com> > Cc: nd <nd@arm.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau > Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v2 1/9] xen/x86: introduce a helper to update memory > hotplug end > > On 13.07.2022 10:22, Wei Chen wrote: > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: 2022年7月12日 19:54 > >> > >> On 08.07.2022 16:54, Wei Chen wrote: > >>> x86 provides a mem_hotplug variable to maintain the memory hotplug > >>> end address. We want to move some codes from x86 to common, so that > >>> it can be reused by other architectures. But not all architectures > >>> have supported memory hotplug. So in this patch, we introduce this > >>> helper to replace mem_hotplug direct access in these codes. This > >>> will give the ability of stubbing this helper to those architectures > >>> without memory hotplug support. > >> > >> What remains unclear to me is why Arm can't simply gain the necessary > >> variable. Sooner or later you'll want to support hotplug there anyway, > > > > I am not sure my limited memory hotplug knowledge can answer your > question > > or not. As memory hotplug depends on hardware and firmware support. Now > > for Arm, we only have ACPI firmware that can notify OS through GED event > > (not very sure). But ACPI and device tree couldn't be enabled at the > same > > time from OS booting. In device tree based NUMA, we will enable device > > tree, ACPI service will not be initialized, that means we can not get > > notification of memory hotplug events from ACPI firmware. > > > > Of course, adding GED device nodes to the device tree, and getting these > > events through GED interrupts should not be a big technical problem. But > > there may be other reasons, and no one is planning to add memory hotplug > > support to the device tree (perhaps need an ACPI-like specification or a > > firmware abstraction interface). So if we want to implement Xen Arm > memory > > hotplug, it should also start from ACPI. So currently even if we gain > the > > variable in Arm, it will not be used. Device tree does not have property > > to indicate a memory block can be hotplug or not. > > But ACPI is possible to be enabled for Arm64, and hence hotplug could be > made work that way. Therefore I view the variable as potentially useful > on Arm as well, and hence introducing the variable might be less trouble > than introducing the per-arch helper. > Ok, I will try to expose mem_hotplug to common/mm.c. As both Arm and x86 code can access it, we can move related NUMA code to common Without this helper. And mem_hotplug will always be zero for Arm until Arm support memory hotplug. This should be harmless for Arm. > >> I suppose. Mechanically the change is fine. Albeit maybe "top" instead > >> of "boundary", and maybe also pass "node" even if x86 doesn't need it? > >> > > > > Sorry, I am not very clear about these comments: > > It makes sense to use mem_hotplug_update_top instead > > of mem_hotplug_update_boundary. But how can I understand pass "node"? > > did you mean mem_hotplug_update_top(node, end)? But mem_hotplug is > > a global top for memory hotplug ranges. I don't think pass "node" > > to this function can be useful. > > Please separate "current implementation" from "abstract model". In the > latter it would seem quite reasonable to me to have per-node upper > bounds (or even per-node ranges). Therefore adding node (and, on top > of what I did suggest before, region start) to the parameters of the > new per-arch hook would seem a good move to me, even if at present > only / at most the "end" parameter would actually be used. > As we will export mem_hotplug to common, I think these changes are not needed any more? Cheers, Wei Chen > Jan
On 13.07.2022 11:55, Wei Chen wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年7月13日 16:46 >> >> On 13.07.2022 10:22, Wei Chen wrote: >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: 2022年7月12日 19:54 >>>> >>>> Mechanically the change is fine. Albeit maybe "top" instead >>>> of "boundary", and maybe also pass "node" even if x86 doesn't need it? >>>> >>> >>> Sorry, I am not very clear about these comments: >>> It makes sense to use mem_hotplug_update_top instead >>> of mem_hotplug_update_boundary. But how can I understand pass "node"? >>> did you mean mem_hotplug_update_top(node, end)? But mem_hotplug is >>> a global top for memory hotplug ranges. I don't think pass "node" >>> to this function can be useful. >> >> Please separate "current implementation" from "abstract model". In the >> latter it would seem quite reasonable to me to have per-node upper >> bounds (or even per-node ranges). Therefore adding node (and, on top >> of what I did suggest before, region start) to the parameters of the >> new per-arch hook would seem a good move to me, even if at present >> only / at most the "end" parameter would actually be used. >> > > As we will export mem_hotplug to common, I think these changes are > not needed any more? Indeed. I merely wanted to address your question nevertheless, or in case there was still a reason to avoid making the global variable common. Jan
diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h index 07b59c982b..b3dfbdb52b 100644 --- a/xen/arch/x86/include/asm/mm.h +++ b/xen/arch/x86/include/asm/mm.h @@ -476,6 +476,12 @@ static inline int get_page_and_type(struct page_info *page, extern paddr_t mem_hotplug; +static inline void mem_hotplug_update_boundary(paddr_t end) +{ + if ( end > mem_hotplug ) + mem_hotplug = end; +} + /****************************************************************************** * With shadow pagetables, the different kinds of address start * to get get confusing. diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c index b62a152911..f53431f5e8 100644 --- a/xen/arch/x86/srat.c +++ b/xen/arch/x86/srat.c @@ -418,8 +418,7 @@ acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) memblk_nodeid[num_node_memblks] = node; if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) { __set_bit(num_node_memblks, memblk_hotplug); - if (end > mem_hotplug) - mem_hotplug = end; + mem_hotplug_update_boundary(end); } num_node_memblks++; }
x86 provides a mem_hotplug variable to maintain the memory hotplug end address. We want to move some codes from x86 to common, so that it can be reused by other architectures. But not all architectures have supported memory hotplug. So in this patch, we introduce this helper to replace mem_hotplug direct access in these codes. This will give the ability of stubbing this helper to those architectures without memory hotplug support. Signed-off-by: Wei Chen <wei.chen@arm.com> --- v1 -> v2: 1. Refine the commit message. 2. Merge v1 patch#9,10 into one patch. Introduce the new functions in the same patch that this patch will be used first time. 3. Fold if ( end > mem_hotplug ) to mem_hotplug_update_boundary, in this case, we can drop mem_hotplug_boundary. --- xen/arch/x86/include/asm/mm.h | 6 ++++++ xen/arch/x86/srat.c | 3 +-- 2 files changed, 7 insertions(+), 2 deletions(-)