[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] NUMA: limit first_valid_mfn exposure



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.