| Message ID | 224db25e-bd4c-4415-aff8-6ff3e84343d8@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] NUMA: limit first_valid_mfn exposure | expand |
Hi Jan, On 08/01/2024 11:31, Jan Beulich wrote: > Address the TODO regarding first_valid_mfn by making the variable static > when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on > x86). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Julien suggests something like > > STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; > > but I view this as non-scalable (or at least I can't see how to > implement such in a scalabale way) and hence undesirable to introduce. I don't really see the scalability problem. Can you explain a bit more? Cheers,
On 08.01.2024 12:37, Julien Grall wrote: > On 08/01/2024 11:31, Jan Beulich wrote: >> Address the TODO regarding first_valid_mfn by making the variable static >> when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on >> x86). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> Julien suggests something like >> >> STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; >> >> but I view this as non-scalable (or at least I can't see how to >> implement such in a scalabale way) and hence undesirable to introduce. > > I don't really see the scalability problem. Can you explain a bit more? Well, when seeing your original suggestion, I first considered it quite reasonable. But when thinking how to implement it, I couldn't see what #define STATIC_IF(cfg) should expand to. That's simply because a macro body cannot itself have pre-processor directives. Hence all I could think of was #ifdef CONFIG_NUMA # define static_if_CONFIG_NUMA static #else # define static_if_CONFIG_NUMA #endif #define STATIC_IF(cfg) static_if_ ## cfg And I think it is easy to see how this wouldn't scale across CONFIG_xyz. Plus that that point STATIC_IF() itself would be pretty much redundant. But maybe I'm simply overlooking the obvious ... Jan
Hi Jan, On 08/01/2024 11:43, Jan Beulich wrote: > On 08.01.2024 12:37, Julien Grall wrote: >> On 08/01/2024 11:31, Jan Beulich wrote: >>> Address the TODO regarding first_valid_mfn by making the variable static >>> when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on >>> x86). >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> Julien suggests something like >>> >>> STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; >>> >>> but I view this as non-scalable (or at least I can't see how to >>> implement such in a scalabale way) and hence undesirable to introduce. >> >> I don't really see the scalability problem. Can you explain a bit more? > > Well, when seeing your original suggestion, I first considered it quite > reasonable. But when thinking how to implement it, I couldn't see what > > #define STATIC_IF(cfg) > > should expand to. That's simply because a macro body cannot itself have > pre-processor directives. Hence all I could think of was > > #ifdef CONFIG_NUMA > # define static_if_CONFIG_NUMA static > #else > # define static_if_CONFIG_NUMA > #endif > #define STATIC_IF(cfg) static_if_ ## cfg > > And I think it is easy to see how this wouldn't scale across CONFIG_xyz. > Plus that that point STATIC_IF() itself would be pretty much redundant. > But maybe I'm simply overlooking the obvious ... You can use the same trick as for IS_ENABLED. The code below will select static or nothing: #define static_enabled(cfg) _static_enabled(cfg) #define _static_enabled(value) __static_enabled(__ARG_PLACEHOLDER_##value) #define __static_enabled(arg1_or_junk) ___static_enabled(arg1_or_junk static,) #define ___static_enabled(__ignored, val, ...) val #define STATIC_IF(option) static_enabled(option) I have tested both with CONFIG_NUMA and !CONFIG_NUMA to confirm the visibility of the variable will be correct. Cheers,
On 08.01.2024 15:13, Julien Grall wrote: > Hi Jan, > > On 08/01/2024 11:43, Jan Beulich wrote: >> On 08.01.2024 12:37, Julien Grall wrote: >>> On 08/01/2024 11:31, Jan Beulich wrote: >>>> Address the TODO regarding first_valid_mfn by making the variable static >>>> when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on >>>> x86). >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> Julien suggests something like >>>> >>>> STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; >>>> >>>> but I view this as non-scalable (or at least I can't see how to >>>> implement such in a scalabale way) and hence undesirable to introduce. >>> >>> I don't really see the scalability problem. Can you explain a bit more? >> >> Well, when seeing your original suggestion, I first considered it quite >> reasonable. But when thinking how to implement it, I couldn't see what >> >> #define STATIC_IF(cfg) >> >> should expand to. That's simply because a macro body cannot itself have >> pre-processor directives. Hence all I could think of was >> >> #ifdef CONFIG_NUMA >> # define static_if_CONFIG_NUMA static >> #else >> # define static_if_CONFIG_NUMA >> #endif >> #define STATIC_IF(cfg) static_if_ ## cfg >> >> And I think it is easy to see how this wouldn't scale across CONFIG_xyz. >> Plus that that point STATIC_IF() itself would be pretty much redundant. >> But maybe I'm simply overlooking the obvious ... > > You can use the same trick as for IS_ENABLED. The code below will select > static or nothing: > > #define static_enabled(cfg) _static_enabled(cfg) > #define _static_enabled(value) __static_enabled(__ARG_PLACEHOLDER_##value) > #define __static_enabled(arg1_or_junk) ___static_enabled(arg1_or_junk > static,) > #define ___static_enabled(__ignored, val, ...) val > > #define STATIC_IF(option) static_enabled(option) > > I have tested both with CONFIG_NUMA and !CONFIG_NUMA to confirm the > visibility of the variable will be correct. Hmm, okay. Then my 2nd scalability concern, in another dimension: What if static-ness ends up depending on two (or more) CONFIG_*? Jan
Hi Jan, On 08/01/2024 14:47, Jan Beulich wrote: > On 08.01.2024 15:13, Julien Grall wrote: >> Hi Jan, >> >> On 08/01/2024 11:43, Jan Beulich wrote: >>> On 08.01.2024 12:37, Julien Grall wrote: >>>> On 08/01/2024 11:31, Jan Beulich wrote: >>>>> Address the TODO regarding first_valid_mfn by making the variable static >>>>> when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on >>>>> x86). >>>>> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>> --- >>>>> Julien suggests something like >>>>> >>>>> STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; >>>>> >>>>> but I view this as non-scalable (or at least I can't see how to >>>>> implement such in a scalabale way) and hence undesirable to introduce. >>>> >>>> I don't really see the scalability problem. Can you explain a bit more? >>> >>> Well, when seeing your original suggestion, I first considered it quite >>> reasonable. But when thinking how to implement it, I couldn't see what >>> >>> #define STATIC_IF(cfg) >>> >>> should expand to. That's simply because a macro body cannot itself have >>> pre-processor directives. Hence all I could think of was >>> >>> #ifdef CONFIG_NUMA >>> # define static_if_CONFIG_NUMA static >>> #else >>> # define static_if_CONFIG_NUMA >>> #endif >>> #define STATIC_IF(cfg) static_if_ ## cfg >>> >>> And I think it is easy to see how this wouldn't scale across CONFIG_xyz. >>> Plus that that point STATIC_IF() itself would be pretty much redundant. >>> But maybe I'm simply overlooking the obvious ... >> >> You can use the same trick as for IS_ENABLED. The code below will select >> static or nothing: >> >> #define static_enabled(cfg) _static_enabled(cfg) >> #define _static_enabled(value) __static_enabled(__ARG_PLACEHOLDER_##value) >> #define __static_enabled(arg1_or_junk) ___static_enabled(arg1_or_junk >> static,) >> #define ___static_enabled(__ignored, val, ...) val >> >> #define STATIC_IF(option) static_enabled(option) >> >> I have tested both with CONFIG_NUMA and !CONFIG_NUMA to confirm the >> visibility of the variable will be correct. > > Hmm, okay. Then my 2nd scalability concern, in another dimension: What > if static-ness ends up depending on two (or more) CONFIG_*? Do you have any concrete example where this would be useful? If not, then I suggest to go with this solution and we can cross the bridge when we have an example. We don't have to solve everything at once and at least with the approach I proposed we can start to use STATIC_IF() (or EXTERN_IF) a bit more often without open-coding it. Cheers,
On 08.01.2024 15:57, Julien Grall wrote: > Hi Jan, > > On 08/01/2024 14:47, Jan Beulich wrote: >> On 08.01.2024 15:13, Julien Grall wrote: >>> Hi Jan, >>> >>> On 08/01/2024 11:43, Jan Beulich wrote: >>>> On 08.01.2024 12:37, Julien Grall wrote: >>>>> On 08/01/2024 11:31, Jan Beulich wrote: >>>>>> Address the TODO regarding first_valid_mfn by making the variable static >>>>>> when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on >>>>>> x86). >>>>>> >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>> --- >>>>>> Julien suggests something like >>>>>> >>>>>> STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; >>>>>> >>>>>> but I view this as non-scalable (or at least I can't see how to >>>>>> implement such in a scalabale way) and hence undesirable to introduce. >>>>> >>>>> I don't really see the scalability problem. Can you explain a bit more? >>>> >>>> Well, when seeing your original suggestion, I first considered it quite >>>> reasonable. But when thinking how to implement it, I couldn't see what >>>> >>>> #define STATIC_IF(cfg) >>>> >>>> should expand to. That's simply because a macro body cannot itself have >>>> pre-processor directives. Hence all I could think of was >>>> >>>> #ifdef CONFIG_NUMA >>>> # define static_if_CONFIG_NUMA static >>>> #else >>>> # define static_if_CONFIG_NUMA >>>> #endif >>>> #define STATIC_IF(cfg) static_if_ ## cfg >>>> >>>> And I think it is easy to see how this wouldn't scale across CONFIG_xyz. >>>> Plus that that point STATIC_IF() itself would be pretty much redundant. >>>> But maybe I'm simply overlooking the obvious ... >>> >>> You can use the same trick as for IS_ENABLED. The code below will select >>> static or nothing: >>> >>> #define static_enabled(cfg) _static_enabled(cfg) >>> #define _static_enabled(value) __static_enabled(__ARG_PLACEHOLDER_##value) >>> #define __static_enabled(arg1_or_junk) ___static_enabled(arg1_or_junk >>> static,) >>> #define ___static_enabled(__ignored, val, ...) val >>> >>> #define STATIC_IF(option) static_enabled(option) >>> >>> I have tested both with CONFIG_NUMA and !CONFIG_NUMA to confirm the >>> visibility of the variable will be correct. >> >> Hmm, okay. Then my 2nd scalability concern, in another dimension: What >> if static-ness ends up depending on two (or more) CONFIG_*? > > Do you have any concrete example where this would be useful? If not, > then I suggest to go with this solution and we can cross the bridge when > we have an example. > > We don't have to solve everything at once and at least with the approach > I proposed we can start to use STATIC_IF() (or EXTERN_IF) a bit more > often without open-coding it. Well. IS_ENABLED() is okay in this regard because you can combine multiple of them (with && or ||). The same isn't true here (afaict). After all I could equally well say that as long as we don't have a sufficient number of such examples, but just one, not introducing a special construct is going to be okay for the time being. Jan
Hi Jan, On 08/01/2024 15:03, Jan Beulich wrote: > On 08.01.2024 15:57, Julien Grall wrote: >> Hi Jan, >> >> On 08/01/2024 14:47, Jan Beulich wrote: >>> On 08.01.2024 15:13, Julien Grall wrote: >>>> Hi Jan, >>>> >>>> On 08/01/2024 11:43, Jan Beulich wrote: >>>>> On 08.01.2024 12:37, Julien Grall wrote: >>>>>> On 08/01/2024 11:31, Jan Beulich wrote: >>>>>>> Address the TODO regarding first_valid_mfn by making the variable static >>>>>>> when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on >>>>>>> x86). >>>>>>> >>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>>> --- >>>>>>> Julien suggests something like >>>>>>> >>>>>>> STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; >>>>>>> >>>>>>> but I view this as non-scalable (or at least I can't see how to >>>>>>> implement such in a scalabale way) and hence undesirable to introduce. >>>>>> >>>>>> I don't really see the scalability problem. Can you explain a bit more? >>>>> >>>>> Well, when seeing your original suggestion, I first considered it quite >>>>> reasonable. But when thinking how to implement it, I couldn't see what >>>>> >>>>> #define STATIC_IF(cfg) >>>>> >>>>> should expand to. That's simply because a macro body cannot itself have >>>>> pre-processor directives. Hence all I could think of was >>>>> >>>>> #ifdef CONFIG_NUMA >>>>> # define static_if_CONFIG_NUMA static >>>>> #else >>>>> # define static_if_CONFIG_NUMA >>>>> #endif >>>>> #define STATIC_IF(cfg) static_if_ ## cfg >>>>> >>>>> And I think it is easy to see how this wouldn't scale across CONFIG_xyz. >>>>> Plus that that point STATIC_IF() itself would be pretty much redundant. >>>>> But maybe I'm simply overlooking the obvious ... >>>> >>>> You can use the same trick as for IS_ENABLED. The code below will select >>>> static or nothing: >>>> >>>> #define static_enabled(cfg) _static_enabled(cfg) >>>> #define _static_enabled(value) __static_enabled(__ARG_PLACEHOLDER_##value) >>>> #define __static_enabled(arg1_or_junk) ___static_enabled(arg1_or_junk >>>> static,) >>>> #define ___static_enabled(__ignored, val, ...) val >>>> >>>> #define STATIC_IF(option) static_enabled(option) >>>> >>>> I have tested both with CONFIG_NUMA and !CONFIG_NUMA to confirm the >>>> visibility of the variable will be correct. >>> >>> Hmm, okay. Then my 2nd scalability concern, in another dimension: What >>> if static-ness ends up depending on two (or more) CONFIG_*? >> >> Do you have any concrete example where this would be useful? If not, >> then I suggest to go with this solution and we can cross the bridge when >> we have an example. >> >> We don't have to solve everything at once and at least with the approach >> I proposed we can start to use STATIC_IF() (or EXTERN_IF) a bit more >> often without open-coding it. > > Well. IS_ENABLED() is okay in this regard because you can combine > multiple of them (with && or ||). The same isn't true here (afaict). Indeed it is not. But I can't think of any place where it would be useful at the moment. > After all I could equally well say that as long as we don't have > a sufficient number of such examples, but just one, not introducing > a special construct is going to be okay for the time being. The thing is I expect more place in the code requiring to use STATIC_IF() (or EXTERN_IF()). So it is not clear to me why we would delay solving the open-coding... In fact, this is a matter of taste, but the cost (less readable) vs benefit (not exposing) is not really worth it to me here. Mostly, because I don't entirely see the problem of keeping first_valid_mfn exposed to everyone. This balance would change if we introduce STATIC_IF(). Anyway, I will let other share their opinions. Cheers,
--- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -254,12 +254,13 @@ static PAGE_LIST_HEAD(page_broken_list); * BOOT-TIME ALLOCATOR */ +#ifndef CONFIG_NUMA /* * 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. */ +#else +static +#endif mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER; struct bootmem_region {
Address the TODO regarding first_valid_mfn by making the variable static when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on x86). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Julien suggests something like STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; but I view this as non-scalable (or at least I can't see how to implement such in a scalabale way) and hence undesirable to introduce. --- v2: New, split off.