| Message ID | d80309f31fea24ea75c4994e924da069472811fc.1702285639.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address some violations of MISRA C Rule 8.4 | expand |
On Mon, 11 Dec 2023, Nicola Vetrini wrote: > Such declaration is needed because a compatible declaration > is not visible in xen/common/page_alloc.c, where the variable > is defined. That variable can't yet be static because of the lack of > support from ARM and PPC for NUMA. > > On the occasion, use drop a use of u8. > > No functional change. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > Having this declaration essentially sidesteps the current impossibility > of having a static variable, as described in the comments in > ARM and PCC's asm/numa.h. > > Changes in v3: > - Drop redundant declarations of first_valid_mfn in asm/numa.h for Arm and PPC. > --- > xen/arch/arm/include/asm/numa.h | 8 ++++---- > xen/arch/ppc/include/asm/numa.h | 7 +++---- > xen/include/xen/mm.h | 2 ++ > 3 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h > index e2bee2bd8223..44b689f67db8 100644 > --- a/xen/arch/arm/include/asm/numa.h > +++ b/xen/arch/arm/include/asm/numa.h > @@ -2,8 +2,9 @@ > #define __ARCH_ARM_NUMA_H > > #include <xen/mm.h> > +#include <xen/types.h> Why the addition of types.h? It doesn't seem to be necessary. Also on PPC below doesn't seem to be necessary. Everything else looks fine. > -typedef u8 nodeid_t; > +typedef uint8_t nodeid_t; > > #ifndef CONFIG_NUMA > > @@ -12,10 +13,9 @@ typedef u8 nodeid_t; > #define node_to_cpumask(node) (cpu_online_map) > > /* > - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this > - * is required because the dummy helpers are using it. > + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm, > + * this is required because the dummy helpers are using it. > */ > -extern mfn_t first_valid_mfn; > > /* XXX: implement NUMA support */ > #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) > diff --git a/xen/arch/ppc/include/asm/numa.h b/xen/arch/ppc/include/asm/numa.h > index 7fdf66c3da74..152305ebe446 100644 > --- a/xen/arch/ppc/include/asm/numa.h > +++ b/xen/arch/ppc/include/asm/numa.h > @@ -1,8 +1,8 @@ > #ifndef __ASM_PPC_NUMA_H__ > #define __ASM_PPC_NUMA_H__ > > -#include <xen/types.h> > #include <xen/mm.h> > +#include <xen/types.h> > > typedef uint8_t nodeid_t; > > @@ -11,10 +11,9 @@ typedef uint8_t nodeid_t; > #define node_to_cpumask(node) (cpu_online_map) > > /* > - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this > - * is required because the dummy helpers are using it. > + * TODO: define here first_valid_mfn as static when NUMA is supported on PPC, > + * this is required because the dummy helpers are using it. > */ > -extern mfn_t first_valid_mfn; > > /* XXX: implement NUMA support */ > #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index 3d9b2d05a5c8..a13a9a46ced7 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned long e); > /* Retrieve the MFN mapped by VA in Xen virtual address space. */ > mfn_t xen_map_to_mfn(unsigned long va); > > +extern mfn_t first_valid_mfn; > + > /* > * Create only non-leaf page table entries for the > * page range in Xen virtual address space. > -- > 2.34.1 >
On 11.12.2023 10:14, Nicola Vetrini wrote: > --- a/xen/arch/arm/include/asm/numa.h > +++ b/xen/arch/arm/include/asm/numa.h > @@ -2,8 +2,9 @@ > #define __ARCH_ARM_NUMA_H > > #include <xen/mm.h> With this, ... > +#include <xen/types.h> > > -typedef u8 nodeid_t; > +typedef uint8_t nodeid_t; > > #ifndef CONFIG_NUMA > > @@ -12,10 +13,9 @@ typedef u8 nodeid_t; > #define node_to_cpumask(node) (cpu_online_map) > > /* > - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this > - * is required because the dummy helpers are using it. > + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm, > + * this is required because the dummy helpers are using it. > */ > -extern mfn_t first_valid_mfn; ... and this declaration moved to xen/mm.h, how is it going to be possible to do as the adjusted comment says? The compiler will choke on the static after having seen the extern. I also firmly disagree with the entirely unrelated u8 -> uint8_t change above. Such tidying can be done when somewhat related to what a patch is doing anyway, but here there's (imo) not even a vague connection. Jan
On Wed, 13 Dec 2023, Jan Beulich wrote: > On 11.12.2023 10:14, Nicola Vetrini wrote: > > --- a/xen/arch/arm/include/asm/numa.h > > +++ b/xen/arch/arm/include/asm/numa.h > > @@ -2,8 +2,9 @@ > > #define __ARCH_ARM_NUMA_H > > > > #include <xen/mm.h> > > With this, ... > > > +#include <xen/types.h> > > > > -typedef u8 nodeid_t; > > +typedef uint8_t nodeid_t; > > > > #ifndef CONFIG_NUMA > > > > @@ -12,10 +13,9 @@ typedef u8 nodeid_t; > > #define node_to_cpumask(node) (cpu_online_map) > > > > /* > > - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this > > - * is required because the dummy helpers are using it. > > + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm, > > + * this is required because the dummy helpers are using it. > > */ > > -extern mfn_t first_valid_mfn; > > ... and this declaration moved to xen/mm.h, how is it going to be possible > to do as the adjusted comment says? The compiler will choke on the static > after having seen the extern. Hi Jan, Nicola was just following a review comment by Julien. NUMA has been pending for a while and I wouldn't hold this patch back because of it. I suggest we go with Julien's request (this version of the patch). If you feel strongly about it, please suggest an alternative. > I also firmly disagree with the entirely unrelated u8 -> uint8_t change > above. Such tidying can be done when somewhat related to what a patch is > doing anyway, but here there's (imo) not even a vague connection. Can be removed on commit
On 14.12.2023 03:05, Stefano Stabellini wrote: > On Wed, 13 Dec 2023, Jan Beulich wrote: >> On 11.12.2023 10:14, Nicola Vetrini wrote: >>> --- a/xen/arch/arm/include/asm/numa.h >>> +++ b/xen/arch/arm/include/asm/numa.h >>> @@ -2,8 +2,9 @@ >>> #define __ARCH_ARM_NUMA_H >>> >>> #include <xen/mm.h> >> >> With this, ... >> >>> +#include <xen/types.h> >>> >>> -typedef u8 nodeid_t; >>> +typedef uint8_t nodeid_t; >>> >>> #ifndef CONFIG_NUMA >>> >>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t; >>> #define node_to_cpumask(node) (cpu_online_map) >>> >>> /* >>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this >>> - * is required because the dummy helpers are using it. >>> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm, >>> + * this is required because the dummy helpers are using it. >>> */ >>> -extern mfn_t first_valid_mfn; >> >> ... and this declaration moved to xen/mm.h, how is it going to be possible >> to do as the adjusted comment says? The compiler will choke on the static >> after having seen the extern. > > Nicola was just following a review comment by Julien. NUMA has been > pending for a while and I wouldn't hold this patch back because of it. > I suggest we go with Julien's request (this version of the patch). > > If you feel strongly about it, please suggest an alternative. Leaving a TODO comment which cannot actually be carried out is just wrong imo. And I consider in unfair to ask me to suggest an alternative. The (imo obvious) alternative is to drop this patch, if no proper change can be proposed. There's nothing wrong with leaving a violation in place, when that violation is far from causing any kind of harm. The more that the place is already accompanied by a (suitable afaict) comment. Jan
Hi Jan, On 14/12/2023 07:53, Jan Beulich wrote: > On 14.12.2023 03:05, Stefano Stabellini wrote: >> On Wed, 13 Dec 2023, Jan Beulich wrote: >>> On 11.12.2023 10:14, Nicola Vetrini wrote: >>>> --- a/xen/arch/arm/include/asm/numa.h >>>> +++ b/xen/arch/arm/include/asm/numa.h >>>> @@ -2,8 +2,9 @@ >>>> #define __ARCH_ARM_NUMA_H >>>> >>>> #include <xen/mm.h> >>> >>> With this, ... >>> >>>> +#include <xen/types.h> >>>> >>>> -typedef u8 nodeid_t; >>>> +typedef uint8_t nodeid_t; >>>> >>>> #ifndef CONFIG_NUMA >>>> >>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t; >>>> #define node_to_cpumask(node) (cpu_online_map) >>>> >>>> /* >>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this >>>> - * is required because the dummy helpers are using it. >>>> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm, >>>> + * this is required because the dummy helpers are using it. >>>> */ >>>> -extern mfn_t first_valid_mfn; >>> >>> ... and this declaration moved to xen/mm.h, how is it going to be possible >>> to do as the adjusted comment says? The compiler will choke on the static >>> after having seen the extern. >> >> Nicola was just following a review comment by Julien. NUMA has been >> pending for a while and I wouldn't hold this patch back because of it. >> I suggest we go with Julien's request (this version of the patch). >> >> If you feel strongly about it, please suggest an alternative. > > Leaving a TODO comment which cannot actually be carried out is just wrong > imo. And I consider in unfair to ask me to suggest an alternative. The > (imo obvious) alternative is to drop this patch, if no proper change can > be proposed. There's nothing wrong with leaving a violation in place, > when that violation is far from causing any kind of harm. The more that > the place is already accompanied by a (suitable afaict) comment. Just to clarify, are you saying you would be happy if the proposal if the TODO is removed? Cheers,
On 2023-12-14 08:53, Jan Beulich wrote: > On 14.12.2023 03:05, Stefano Stabellini wrote: >> On Wed, 13 Dec 2023, Jan Beulich wrote: >>> On 11.12.2023 10:14, Nicola Vetrini wrote: >>>> --- a/xen/arch/arm/include/asm/numa.h >>>> +++ b/xen/arch/arm/include/asm/numa.h >>>> @@ -2,8 +2,9 @@ >>>> #define __ARCH_ARM_NUMA_H >>>> >>>> #include <xen/mm.h> >>> >>> With this, ... >>> >>>> +#include <xen/types.h> >>>> >>>> -typedef u8 nodeid_t; >>>> +typedef uint8_t nodeid_t; >>>> >>>> #ifndef CONFIG_NUMA >>>> >>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t; >>>> #define node_to_cpumask(node) (cpu_online_map) >>>> >>>> /* >>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, >>>> this >>>> - * is required because the dummy helpers are using it. >>>> + * TODO: define here first_valid_mfn as static when NUMA is >>>> supported on Arm, >>>> + * this is required because the dummy helpers are using it. >>>> */ >>>> -extern mfn_t first_valid_mfn; >>> >>> ... and this declaration moved to xen/mm.h, how is it going to be >>> possible >>> to do as the adjusted comment says? The compiler will choke on the >>> static >>> after having seen the extern. >> >> Nicola was just following a review comment by Julien. NUMA has been >> pending for a while and I wouldn't hold this patch back because of it. >> I suggest we go with Julien's request (this version of the patch). >> >> If you feel strongly about it, please suggest an alternative. > > Leaving a TODO comment which cannot actually be carried out is just > wrong > imo. And I consider in unfair to ask me to suggest an alternative. The > (imo obvious) alternative is to drop this patch, if no proper change > can > be proposed. There's nothing wrong with leaving a violation in place, > when that violation is far from causing any kind of harm. The more that > the place is already accompanied by a (suitable afaict) comment. > > Jan I have a proposal: deviate first_valid_mfn as a deliberate workaround to NUMA not being supported on ARM and PPC. This still supplies a justification and does not imply any other code change.
On 14.12.2023 09:32, Julien Grall wrote: > Hi Jan, > > On 14/12/2023 07:53, Jan Beulich wrote: >> On 14.12.2023 03:05, Stefano Stabellini wrote: >>> On Wed, 13 Dec 2023, Jan Beulich wrote: >>>> On 11.12.2023 10:14, Nicola Vetrini wrote: >>>>> --- a/xen/arch/arm/include/asm/numa.h >>>>> +++ b/xen/arch/arm/include/asm/numa.h >>>>> @@ -2,8 +2,9 @@ >>>>> #define __ARCH_ARM_NUMA_H >>>>> >>>>> #include <xen/mm.h> >>>> >>>> With this, ... >>>> >>>>> +#include <xen/types.h> >>>>> >>>>> -typedef u8 nodeid_t; >>>>> +typedef uint8_t nodeid_t; >>>>> >>>>> #ifndef CONFIG_NUMA >>>>> >>>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t; >>>>> #define node_to_cpumask(node) (cpu_online_map) >>>>> >>>>> /* >>>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this >>>>> - * is required because the dummy helpers are using it. >>>>> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm, >>>>> + * this is required because the dummy helpers are using it. >>>>> */ >>>>> -extern mfn_t first_valid_mfn; >>>> >>>> ... and this declaration moved to xen/mm.h, how is it going to be possible >>>> to do as the adjusted comment says? The compiler will choke on the static >>>> after having seen the extern. >>> >>> Nicola was just following a review comment by Julien. NUMA has been >>> pending for a while and I wouldn't hold this patch back because of it. >>> I suggest we go with Julien's request (this version of the patch). >>> >>> If you feel strongly about it, please suggest an alternative. >> >> Leaving a TODO comment which cannot actually be carried out is just wrong >> imo. And I consider in unfair to ask me to suggest an alternative. The >> (imo obvious) alternative is to drop this patch, if no proper change can >> be proposed. There's nothing wrong with leaving a violation in place, >> when that violation is far from causing any kind of harm. The more that >> the place is already accompanied by a (suitable afaict) comment. > > Just to clarify, are you saying you would be happy if the proposal if > the TODO is removed? Removing the bad (new) TODO here is an option. But the previous TODO shouldn't go away. However, you asking now required me to actually look into Stefano's request to make an alternative proposal (I still don't see though why it needed to be me to think about an appropriate solution; probably in the time I've spent writing replies on this thread, I could have made the change myself). First, Arm's and PPC's header have this in their !NUMA sections. Much like Oleksii's putting in quite a bit of effort to reduce redundancy in order to not further grow it with RISC-V, what's wrong with sorting the !NUMA case properly as a first step? Move the entire !NUMA section either into xen/numa.h (eliminating the need for arch-es not supporting NUMA to even have such a header), or (if need be) to asm-generic/. Then, as a 2nd step (albeit that could also be done on its own, just the result would be less neat imo), make the variable in question here extern _only_ when !NUMA. This would then address the original TODO, so that could then legitimately be dropped. This would further be a good opportunity to adjust the already stale comment in page_alloc.c (it's no longer applicable to Arm only). Jan
On Thu, Dec 14, 2023 at 8:51 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 14.12.2023 09:32, Julien Grall wrote: > > Hi Jan, > > > > On 14/12/2023 07:53, Jan Beulich wrote: > >> On 14.12.2023 03:05, Stefano Stabellini wrote: > >>> On Wed, 13 Dec 2023, Jan Beulich wrote: > >>>> On 11.12.2023 10:14, Nicola Vetrini wrote: > >>>>> --- a/xen/arch/arm/include/asm/numa.h > >>>>> +++ b/xen/arch/arm/include/asm/numa.h > >>>>> @@ -2,8 +2,9 @@ > >>>>> #define __ARCH_ARM_NUMA_H > >>>>> > >>>>> #include <xen/mm.h> > >>>> > >>>> With this, ... > >>>> > >>>>> +#include <xen/types.h> > >>>>> > >>>>> -typedef u8 nodeid_t; > >>>>> +typedef uint8_t nodeid_t; > >>>>> > >>>>> #ifndef CONFIG_NUMA > >>>>> > >>>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t; > >>>>> #define node_to_cpumask(node) (cpu_online_map) > >>>>> > >>>>> /* > >>>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this > >>>>> - * is required because the dummy helpers are using it. > >>>>> + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm, > >>>>> + * this is required because the dummy helpers are using it. > >>>>> */ > >>>>> -extern mfn_t first_valid_mfn; > >>>> > >>>> ... and this declaration moved to xen/mm.h, how is it going to be possible > >>>> to do as the adjusted comment says? The compiler will choke on the static > >>>> after having seen the extern. > >>> > >>> Nicola was just following a review comment by Julien. NUMA has been > >>> pending for a while and I wouldn't hold this patch back because of it. > >>> I suggest we go with Julien's request (this version of the patch). > >>> > >>> If you feel strongly about it, please suggest an alternative. > >> > >> Leaving a TODO comment which cannot actually be carried out is just wrong > >> imo. And I consider in unfair to ask me to suggest an alternative. The > >> (imo obvious) alternative is to drop this patch, if no proper change can > >> be proposed. There's nothing wrong with leaving a violation in place, > >> when that violation is far from causing any kind of harm. The more that > >> the place is already accompanied by a (suitable afaict) comment. > > > > Just to clarify, are you saying you would be happy if the proposal if > > the TODO is removed? > > Removing the bad (new) TODO here is an option. But the previous TODO shouldn't > go away. However, you asking now required me to actually look into Stefano's > request to make an alternative proposal (I still don't see though why it > needed to be me to think about an appropriate solution; In general, it is unreasonable to expect other people to come up with suggestions to make you happy. You're ultimately the only person who knows what would make you satisfied. You're also more senior and know the code better, and thus in a better position to be able to come up with ideas. "What about X?" "No." "What about Y?" "No." "What about Z?" "No." Contributors experience it as caustic behavior. The only time it's acceptable is if you can specify, precisely and reasonably completely, your criteria for acceptance, such that there's a clear way forward. In this case, for instance, it sounds like "Has a TODO which isn't technically inaccurate" might be the criteria. In which case, for instance, a solution might be along the lines of: "TODO: When NUMA is supported on Arm we'll need to do something about defining first_valid_mfn, which is used by the dummy helpers." This punts the actual solution down the road until we need it. But I do think that it's fair to ask Julien to think about a suitable wording, since the comment is in a sense to remind him (or other ARM maintainers) what's needed, and since the eventual solution will be something to do with the ARM code and architecture anyway. -George > > First, Arm's and PPC's header have this in their !NUMA sections. Much like > Oleksii's putting in quite a bit of effort to reduce redundancy in order to > not further grow it with RISC-V, what's wrong with sorting the !NUMA case > properly as a first step? Move the entire !NUMA section either into xen/numa.h > (eliminating the need for arch-es not supporting NUMA to even have such a > header), or (if need be) to asm-generic/. Then, as a 2nd step (albeit that > could also be done on its own, just the result would be less neat imo), make > the variable in question here extern _only_ when !NUMA. This would then > address the original TODO, so that could then legitimately be dropped. This > would further be a good opportunity to adjust the already stale comment in > page_alloc.c (it's no longer applicable to Arm only). > > Jan
Hi George, On 14/12/2023 14:15, George Dunlap wrote: > But I do think that it's fair to ask Julien to think about a suitable > wording, since the comment is in a sense to remind him (or other ARM > maintainers) what's needed, and since the eventual solution will be > something to do with the ARM code and architecture anyway. The comment is for anyone using !NUMA (i.e. all architectures but x86) :). What about the following (this is Nicola's patch with the comments reworked): diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h index e2bee2bd8223..4bf7c304ea3c 100644 --- a/xen/arch/arm/include/asm/numa.h +++ b/xen/arch/arm/include/asm/numa.h @@ -2,8 +2,9 @@ #define __ARCH_ARM_NUMA_H #include <xen/mm.h> +#include <xen/types.h> -typedef u8 nodeid_t; +typedef uint8_t nodeid_t; #ifndef CONFIG_NUMA @@ -11,12 +12,6 @@ typedef u8 nodeid_t; #define cpu_to_node(cpu) 0 #define node_to_cpumask(node) (cpu_online_map) -/* - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this - * is required because the dummy helpers are using it. - */ -extern mfn_t first_valid_mfn; - /* XXX: implement NUMA support */ #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) diff --git a/xen/arch/ppc/include/asm/numa.h b/xen/arch/ppc/include/asm/numa.h index 7fdf66c3da74..888de2dbd1eb 100644 --- a/xen/arch/ppc/include/asm/numa.h +++ b/xen/arch/ppc/include/asm/numa.h @@ -1,8 +1,8 @@ #ifndef __ASM_PPC_NUMA_H__ #define __ASM_PPC_NUMA_H__ -#include <xen/types.h> #include <xen/mm.h> +#include <xen/types.h> typedef uint8_t nodeid_t; @@ -10,12 +10,6 @@ typedef uint8_t nodeid_t; #define cpu_to_node(cpu) 0 #define node_to_cpumask(node) (cpu_online_map) -/* - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this - * is required because the dummy helpers are using it. - */ -extern mfn_t first_valid_mfn; - /* XXX: implement NUMA support */ #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 9b5df74fddab..d874525916ea 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list); */ /* - * first_valid_mfn is exported because it is use in ARM specific NUMA - * helpers. See comment in arch/arm/include/asm/numa.h. + * first_valid_mfn is exported because it is used when !CONFIG_NUMA. + * + * TODO: Consider if we can conditionally export first_valid_mfn based + * on whether NUMA is selected. */ mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER; diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 3d9b2d05a5c8..a13a9a46ced7 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned long e); /* Retrieve the MFN mapped by VA in Xen virtual address space. */ mfn_t xen_map_to_mfn(unsigned long va); +extern mfn_t first_valid_mfn; + /* * Create only non-leaf page table entries for the * page range in Xen virtual address space. Cheers,
On Thu, 14 Dec 2023, Julien Grall wrote: > Hi George, > > On 14/12/2023 14:15, George Dunlap wrote: > > But I do think that it's fair to ask Julien to think about a suitable > > wording, since the comment is in a sense to remind him (or other ARM > > maintainers) what's needed, and since the eventual solution will be > > something to do with the ARM code and architecture anyway. > > The comment is for anyone using !NUMA (i.e. all architectures but x86) :). > What about the following (this is Nicola's patch with the comments reworked): The below looks good to me. > diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h > index e2bee2bd8223..4bf7c304ea3c 100644 > --- a/xen/arch/arm/include/asm/numa.h > +++ b/xen/arch/arm/include/asm/numa.h > @@ -2,8 +2,9 @@ > #define __ARCH_ARM_NUMA_H > > #include <xen/mm.h> > +#include <xen/types.h> > > -typedef u8 nodeid_t; > +typedef uint8_t nodeid_t; > > #ifndef CONFIG_NUMA > > @@ -11,12 +12,6 @@ typedef u8 nodeid_t; > #define cpu_to_node(cpu) 0 > #define node_to_cpumask(node) (cpu_online_map) > > -/* > - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this > - * is required because the dummy helpers are using it. > - */ > -extern mfn_t first_valid_mfn; > - > /* XXX: implement NUMA support */ > #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) > #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) > diff --git a/xen/arch/ppc/include/asm/numa.h b/xen/arch/ppc/include/asm/numa.h > index 7fdf66c3da74..888de2dbd1eb 100644 > --- a/xen/arch/ppc/include/asm/numa.h > +++ b/xen/arch/ppc/include/asm/numa.h > @@ -1,8 +1,8 @@ > #ifndef __ASM_PPC_NUMA_H__ > #define __ASM_PPC_NUMA_H__ > > -#include <xen/types.h> > #include <xen/mm.h> > +#include <xen/types.h> > > typedef uint8_t nodeid_t; > > @@ -10,12 +10,6 @@ typedef uint8_t nodeid_t; > #define cpu_to_node(cpu) 0 > #define node_to_cpumask(node) (cpu_online_map) > > -/* > - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this > - * is required because the dummy helpers are using it. > - */ > -extern mfn_t first_valid_mfn; > - > /* XXX: implement NUMA support */ > #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) > #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 9b5df74fddab..d874525916ea 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list); > */ > > /* > - * first_valid_mfn is exported because it is use in ARM specific NUMA > - * helpers. See comment in arch/arm/include/asm/numa.h. > + * first_valid_mfn is exported because it is used when !CONFIG_NUMA. > + * > + * TODO: Consider if we can conditionally export first_valid_mfn based > + * on whether NUMA is selected. > */ > mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER; > > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index 3d9b2d05a5c8..a13a9a46ced7 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned long > e); > /* Retrieve the MFN mapped by VA in Xen virtual address space. */ > mfn_t xen_map_to_mfn(unsigned long va); > > +extern mfn_t first_valid_mfn; > + > /* > * Create only non-leaf page table entries for the > * page range in Xen virtual address space. > > Cheers, > > -- > Julien Grall >
On 14.12.2023 20:10, Julien Grall wrote: > On 14/12/2023 14:15, George Dunlap wrote: >> But I do think that it's fair to ask Julien to think about a suitable >> wording, since the comment is in a sense to remind him (or other ARM >> maintainers) what's needed, and since the eventual solution will be >> something to do with the ARM code and architecture anyway. > > The comment is for anyone using !NUMA (i.e. all architectures but x86) > :). What about the following (this is Nicola's patch with the comments > reworked): This clearly is better, yet then ... > --- a/xen/arch/arm/include/asm/numa.h > +++ b/xen/arch/arm/include/asm/numa.h > @@ -2,8 +2,9 @@ > #define __ARCH_ARM_NUMA_H > > #include <xen/mm.h> > +#include <xen/types.h> > > -typedef u8 nodeid_t; > +typedef uint8_t nodeid_t; > > #ifndef CONFIG_NUMA > > @@ -11,12 +12,6 @@ typedef u8 nodeid_t; > #define cpu_to_node(cpu) 0 > #define node_to_cpumask(node) (cpu_online_map) > > -/* > - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this > - * is required because the dummy helpers are using it. > - */ > -extern mfn_t first_valid_mfn; > - > /* XXX: implement NUMA support */ > #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) > #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) > diff --git a/xen/arch/ppc/include/asm/numa.h > b/xen/arch/ppc/include/asm/numa.h > index 7fdf66c3da74..888de2dbd1eb 100644 > --- a/xen/arch/ppc/include/asm/numa.h > +++ b/xen/arch/ppc/include/asm/numa.h > @@ -1,8 +1,8 @@ > #ifndef __ASM_PPC_NUMA_H__ > #define __ASM_PPC_NUMA_H__ > > -#include <xen/types.h> > #include <xen/mm.h> > +#include <xen/types.h> > > typedef uint8_t nodeid_t; > > @@ -10,12 +10,6 @@ typedef uint8_t nodeid_t; > #define cpu_to_node(cpu) 0 > #define node_to_cpumask(node) (cpu_online_map) > > -/* > - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this > - * is required because the dummy helpers are using it. > - */ > -extern mfn_t first_valid_mfn; > - > /* XXX: implement NUMA support */ > #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) > #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 9b5df74fddab..d874525916ea 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list); > */ > > /* > - * first_valid_mfn is exported because it is use in ARM specific NUMA > - * helpers. See comment in arch/arm/include/asm/numa.h. > + * first_valid_mfn is exported because it is used when !CONFIG_NUMA. > + * > + * TODO: Consider if we can conditionally export first_valid_mfn based > + * on whether NUMA is selected. > */ > mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER; > > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index 3d9b2d05a5c8..a13a9a46ced7 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned > long e); > /* Retrieve the MFN mapped by VA in Xen virtual address space. */ > mfn_t xen_map_to_mfn(unsigned long va); > > +extern mfn_t first_valid_mfn; > + > /* > * Create only non-leaf page table entries for the > * page range in Xen virtual address space. ... I still disagree with the placement here (should be xen/numa.h imo), and I still don't see why we can't carry out the TODO right away, if we have to touch all of this anyway. If it's really too much to ask from the original contributor, I can certainly see about making a patch myself (and I've now added this to my short-term TODO list). Jan
Hi Jan, On 15/12/2023 08:03, Jan Beulich wrote: > On 14.12.2023 20:10, Julien Grall wrote: >> On 14/12/2023 14:15, George Dunlap wrote: >>> But I do think that it's fair to ask Julien to think about a suitable >>> wording, since the comment is in a sense to remind him (or other ARM >>> maintainers) what's needed, and since the eventual solution will be >>> something to do with the ARM code and architecture anyway. >> >> The comment is for anyone using !NUMA (i.e. all architectures but x86) >> :). What about the following (this is Nicola's patch with the comments >> reworked): > > This clearly is better, yet then ... > >> --- a/xen/arch/arm/include/asm/numa.h >> +++ b/xen/arch/arm/include/asm/numa.h >> @@ -2,8 +2,9 @@ >> #define __ARCH_ARM_NUMA_H >> >> #include <xen/mm.h> >> +#include <xen/types.h> >> >> -typedef u8 nodeid_t; >> +typedef uint8_t nodeid_t; >> >> #ifndef CONFIG_NUMA >> >> @@ -11,12 +12,6 @@ typedef u8 nodeid_t; >> #define cpu_to_node(cpu) 0 >> #define node_to_cpumask(node) (cpu_online_map) >> >> -/* >> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this >> - * is required because the dummy helpers are using it. >> - */ >> -extern mfn_t first_valid_mfn; >> - >> /* XXX: implement NUMA support */ >> #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) >> #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) >> diff --git a/xen/arch/ppc/include/asm/numa.h >> b/xen/arch/ppc/include/asm/numa.h >> index 7fdf66c3da74..888de2dbd1eb 100644 >> --- a/xen/arch/ppc/include/asm/numa.h >> +++ b/xen/arch/ppc/include/asm/numa.h >> @@ -1,8 +1,8 @@ >> #ifndef __ASM_PPC_NUMA_H__ >> #define __ASM_PPC_NUMA_H__ >> >> -#include <xen/types.h> >> #include <xen/mm.h> >> +#include <xen/types.h> >> >> typedef uint8_t nodeid_t; >> >> @@ -10,12 +10,6 @@ typedef uint8_t nodeid_t; >> #define cpu_to_node(cpu) 0 >> #define node_to_cpumask(node) (cpu_online_map) >> >> -/* >> - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this >> - * is required because the dummy helpers are using it. >> - */ >> -extern mfn_t first_valid_mfn; >> - >> /* XXX: implement NUMA support */ >> #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) >> #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) >> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c >> index 9b5df74fddab..d874525916ea 100644 >> --- a/xen/common/page_alloc.c >> +++ b/xen/common/page_alloc.c >> @@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list); >> */ >> >> /* >> - * first_valid_mfn is exported because it is use in ARM specific NUMA >> - * helpers. See comment in arch/arm/include/asm/numa.h. >> + * first_valid_mfn is exported because it is used when !CONFIG_NUMA. >> + * >> + * TODO: Consider if we can conditionally export first_valid_mfn based >> + * on whether NUMA is selected. >> */ >> mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER; >> >> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h >> index 3d9b2d05a5c8..a13a9a46ced7 100644 >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned >> long e); >> /* Retrieve the MFN mapped by VA in Xen virtual address space. */ >> mfn_t xen_map_to_mfn(unsigned long va); >> >> +extern mfn_t first_valid_mfn; >> + >> /* >> * Create only non-leaf page table entries for the >> * page range in Xen virtual address space. > > ... I still disagree with the placement here (should be xen/numa.h imo), I don't care too much about which header is used. So I am ok with numa.h if this is what you really want. However... > and I still don't see why we can't carry out the TODO right away, if we > have to touch all of this anyway. If it's really too much to ask from > the original contributor, I can certainly see about making a patch myself There are a few reasons I didn't update with that approach or want to ask Nicola for this: 1. I assume this will be a macro and it is not clear to me whether Eclair would be able to detect the visibility change based on the config 2. IMHO, this is obfuscating a bit more the code 3. I don't see why we want to special case first_valid_mfn. I am sure we have other external variables in the same situation. So if any solution needs to came up, then it needs to be generic, and I fear this is going to be yet another lengthy work which is not worth it for this case. > (and I've now added this to my short-term TODO list). Thanks. But as I wrote above, I am not sure I agree such proposal. Let see what the code looks like. Cheers,
Hi Jan, Julien >>> >>> +extern mfn_t first_valid_mfn; >>> + >>> /* >>> * Create only non-leaf page table entries for the >>> * page range in Xen virtual address space. >> >> ... I still disagree with the placement here (should be xen/numa.h >> imo), > > I don't care too much about which header is used. So I am ok with > numa.h if this is what you really want. However... > >> and I still don't see why we can't carry out the TODO right away, if >> we >> have to touch all of this anyway. If it's really too much to ask from >> the original contributor, I can certainly see about making a patch >> myself > > There are a few reasons I didn't update with that approach or want to > ask Nicola for this: > 1. I assume this will be a macro and it is not clear to me whether > Eclair would be able to detect the visibility change based on the > config > 2. IMHO, this is obfuscating a bit more the code > 3. I don't see why we want to special case first_valid_mfn. I am sure > we have other external variables in the same situation. > > So if any solution needs to came up, then it needs to be generic, and I > fear this is going to be yet another lengthy work which is not worth it > for this case. > >> (and I've now added this to my short-term TODO list). > > Thanks. But as I wrote above, I am not sure I agree such proposal. Let > see what the code looks like. > Given the controversy that arose on this patch, maybe it's best to drop it now from this series. If then it's agreed that as a temporary workaround you want to leave it as is and deviate, as I proposed elsewhere in the thread, feel free to reach out. Meanwhile, if patch 1 looks ok I can mark this series as done and focus on the v2 for R2.1, which is almost ready.
Hi, On 15/12/2023 09:59, Nicola Vetrini wrote: >>>> +extern mfn_t first_valid_mfn; >>>> + >>>> /* >>>> * Create only non-leaf page table entries for the >>>> * page range in Xen virtual address space. >>> >>> ... I still disagree with the placement here (should be xen/numa.h imo), >> >> I don't care too much about which header is used. So I am ok with >> numa.h if this is what you really want. However... >> >>> and I still don't see why we can't carry out the TODO right away, if we >>> have to touch all of this anyway. If it's really too much to ask from >>> the original contributor, I can certainly see about making a patch >>> myself >> >> There are a few reasons I didn't update with that approach or want to >> ask Nicola for this: >> 1. I assume this will be a macro and it is not clear to me whether >> Eclair would be able to detect the visibility change based on the config >> 2. IMHO, this is obfuscating a bit more the code >> 3. I don't see why we want to special case first_valid_mfn. I am >> sure we have other external variables in the same situation. >> >> So if any solution needs to came up, then it needs to be generic, and >> I fear this is going to be yet another lengthy work which is not worth >> it for this case. >> >>> (and I've now added this to my short-term TODO list). >> >> Thanks. But as I wrote above, I am not sure I agree such proposal. Let >> see what the code looks like. >> > > Given the controversy that arose on this patch, maybe it's best to drop > it now from this series. If then it's agreed that as a temporary > workaround you want to leave it as is and deviate, as I proposed > elsewhere in the thread, feel free to reach out. Meanwhile, if patch 1 > looks ok I can mark this series as done and focus on the v2 for R2.1, > which is almost ready. I am still not in favor of deviation. We would need to create a new SAF-* (because none of the existing one are not correct) and it is not clear how you could argue this is a good deviation. Just to be clear, this is not a Nack for a deviation. I find just silly we are trying to use deviation here just because there is a small roadblock. To me the goal is to try to improve Xen, not trying to deviate everything because this is easier. Cheers,
On Fri, 15 Dec 2023, Jan Beulich wrote: > On 14.12.2023 20:10, Julien Grall wrote: > > On 14/12/2023 14:15, George Dunlap wrote: > >> But I do think that it's fair to ask Julien to think about a suitable > >> wording, since the comment is in a sense to remind him (or other ARM > >> maintainers) what's needed, and since the eventual solution will be > >> something to do with the ARM code and architecture anyway. > > > > The comment is for anyone using !NUMA (i.e. all architectures but x86) > > :). What about the following (this is Nicola's patch with the comments > > reworked): > > This clearly is better, yet then ... > > > --- a/xen/arch/arm/include/asm/numa.h > > +++ b/xen/arch/arm/include/asm/numa.h > > @@ -2,8 +2,9 @@ > > #define __ARCH_ARM_NUMA_H > > > > #include <xen/mm.h> > > +#include <xen/types.h> > > > > -typedef u8 nodeid_t; > > +typedef uint8_t nodeid_t; > > > > #ifndef CONFIG_NUMA > > > > @@ -11,12 +12,6 @@ typedef u8 nodeid_t; > > #define cpu_to_node(cpu) 0 > > #define node_to_cpumask(node) (cpu_online_map) > > > > -/* > > - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this > > - * is required because the dummy helpers are using it. > > - */ > > -extern mfn_t first_valid_mfn; > > - > > /* XXX: implement NUMA support */ > > #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) > > #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) > > diff --git a/xen/arch/ppc/include/asm/numa.h > > b/xen/arch/ppc/include/asm/numa.h > > index 7fdf66c3da74..888de2dbd1eb 100644 > > --- a/xen/arch/ppc/include/asm/numa.h > > +++ b/xen/arch/ppc/include/asm/numa.h > > @@ -1,8 +1,8 @@ > > #ifndef __ASM_PPC_NUMA_H__ > > #define __ASM_PPC_NUMA_H__ > > > > -#include <xen/types.h> > > #include <xen/mm.h> > > +#include <xen/types.h> > > > > typedef uint8_t nodeid_t; > > > > @@ -10,12 +10,6 @@ typedef uint8_t nodeid_t; > > #define cpu_to_node(cpu) 0 > > #define node_to_cpumask(node) (cpu_online_map) > > > > -/* > > - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this > > - * is required because the dummy helpers are using it. > > - */ > > -extern mfn_t first_valid_mfn; > > - > > /* XXX: implement NUMA support */ > > #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) > > #define node_start_pfn(nid) (mfn_x(first_valid_mfn)) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > > index 9b5df74fddab..d874525916ea 100644 > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list); > > */ > > > > /* > > - * first_valid_mfn is exported because it is use in ARM specific NUMA > > - * helpers. See comment in arch/arm/include/asm/numa.h. > > + * first_valid_mfn is exported because it is used when !CONFIG_NUMA. > > + * > > + * TODO: Consider if we can conditionally export first_valid_mfn based > > + * on whether NUMA is selected. > > */ > > mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER; > > > > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > > index 3d9b2d05a5c8..a13a9a46ced7 100644 > > --- a/xen/include/xen/mm.h > > +++ b/xen/include/xen/mm.h > > @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned > > long e); > > /* Retrieve the MFN mapped by VA in Xen virtual address space. */ > > mfn_t xen_map_to_mfn(unsigned long va); > > > > +extern mfn_t first_valid_mfn; > > + > > /* > > * Create only non-leaf page table entries for the > > * page range in Xen virtual address space. > > ... I still disagree with the placement here (should be xen/numa.h imo), This is where I would call it "good enough" and close it for now. I think we should commit such a patch (Julien's patch with the placement in xen/numa.h). Anything else... > and I still don't see why we can't carry out the TODO right away, if we > have to touch all of this anyway. If it's really too much to ask from > the original contributor, I can certainly see about making a patch myself > (and I've now added this to my short-term TODO list). ... can still be done later whenever you get to it.
diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h index e2bee2bd8223..44b689f67db8 100644 --- a/xen/arch/arm/include/asm/numa.h +++ b/xen/arch/arm/include/asm/numa.h @@ -2,8 +2,9 @@ #define __ARCH_ARM_NUMA_H #include <xen/mm.h> +#include <xen/types.h> -typedef u8 nodeid_t; +typedef uint8_t nodeid_t; #ifndef CONFIG_NUMA @@ -12,10 +13,9 @@ typedef u8 nodeid_t; #define node_to_cpumask(node) (cpu_online_map) /* - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this - * is required because the dummy helpers are using it. + * TODO: define here first_valid_mfn as static when NUMA is supported on Arm, + * this is required because the dummy helpers are using it. */ -extern mfn_t first_valid_mfn; /* XXX: implement NUMA support */ #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) diff --git a/xen/arch/ppc/include/asm/numa.h b/xen/arch/ppc/include/asm/numa.h index 7fdf66c3da74..152305ebe446 100644 --- a/xen/arch/ppc/include/asm/numa.h +++ b/xen/arch/ppc/include/asm/numa.h @@ -1,8 +1,8 @@ #ifndef __ASM_PPC_NUMA_H__ #define __ASM_PPC_NUMA_H__ -#include <xen/types.h> #include <xen/mm.h> +#include <xen/types.h> typedef uint8_t nodeid_t; @@ -11,10 +11,9 @@ typedef uint8_t nodeid_t; #define node_to_cpumask(node) (cpu_online_map) /* - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this - * is required because the dummy helpers are using it. + * TODO: define here first_valid_mfn as static when NUMA is supported on PPC, + * this is required because the dummy helpers are using it. */ -extern mfn_t first_valid_mfn; /* XXX: implement NUMA support */ #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn)) diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 3d9b2d05a5c8..a13a9a46ced7 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned long e); /* Retrieve the MFN mapped by VA in Xen virtual address space. */ mfn_t xen_map_to_mfn(unsigned long va); +extern mfn_t first_valid_mfn; + /* * Create only non-leaf page table entries for the * page range in Xen virtual address space.
Such declaration is needed because a compatible declaration is not visible in xen/common/page_alloc.c, where the variable is defined. That variable can't yet be static because of the lack of support from ARM and PPC for NUMA. On the occasion, use drop a use of u8. No functional change. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- Having this declaration essentially sidesteps the current impossibility of having a static variable, as described in the comments in ARM and PCC's asm/numa.h. Changes in v3: - Drop redundant declarations of first_valid_mfn in asm/numa.h for Arm and PPC. --- xen/arch/arm/include/asm/numa.h | 8 ++++---- xen/arch/ppc/include/asm/numa.h | 7 +++---- xen/include/xen/mm.h | 2 ++ 3 files changed, 9 insertions(+), 8 deletions(-)