Re: [PATCH v2] NUMA: limit first_valid_mfn exposure

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxx>
> > > > > > > ---
> > > > > > > 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,

--
Julien Grall