Re: [PATCH v2] NUMA: limit first_valid_mfn exposure

[Jan Beulich <jbeulich@xxxxxxxx>]

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).
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