intel_turbo_max_3 non-modularity

intel_turbo_max_3 non-modularity

[Jean Delvare]

Hi all,

I see that the intel_turbo_max_3 driver was originally supposed to be
modular, and then support for that possibility was removed. Is there
any fundamental reason why this driver can't be built as a module?

I am asking because the description of the driver says:

"This driver is only required when the system is not using Hardware
P-States (HWP). In HWP mode, priority can be read from ACPI tables."

This pretty much implies that this driver will be useless on a wide
range (maybe even the majority?) of systems. Given this, not being
forced to build the driver into the kernel would seem preferable.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: intel_turbo_max_3 non-modularity

[Paul Gortmaker]

[intel_turbo_max_3 non-modularity] On 24/04/2017 (Mon 11:31) Jean Delvare wrote:

> Hi all,
> 
> I see that the intel_turbo_max_3 driver was originally supposed to be
> modular, and then support for that possibility was removed. Is there
> any fundamental reason why this driver can't be built as a module?

Re-reading the thread, it was stated that the driver needed exports of
some scheduler functions that weren't currently exported.  To me, that
means one of two things -- the driver is poking at scheduler internals
that it shouldn't be and needs to be modified accordingly, or that
someone needs to convince the sched folks that there is valid use cases
for exports of these functions, and _then_ enable modularity.

I didn't try to build it as a module, so I can't say what the sched
dependencies were, or if they looked valid.  But I have seen the sched
folks being not impressed at how certain arch specific PM/freq stuff has
been implemented in the past, which is why I mention it as a possibility.

P.

Re: intel_turbo_max_3 non-modularity

[Darren Hart]

On Mon, Apr 24, 2017 at 10:02:18AM -0400, Paul Gortmaker wrote:
> [intel_turbo_max_3 non-modularity] On 24/04/2017 (Mon 11:31) Jean Delvare wrote:
> 
> > Hi all,
> > 
> > I see that the intel_turbo_max_3 driver was originally supposed to be
> > modular, and then support for that possibility was removed. Is there
> > any fundamental reason why this driver can't be built as a module?

Thanks for asking the question Jean,

+Srinivas
+Peterz

> 
> Re-reading the thread, it was stated that the driver needed exports of
> some scheduler functions that weren't currently exported.  To me, that
> means one of two things -- the driver is poking at scheduler internals
> that it shouldn't be and needs to be modified accordingly, or that
> someone needs to convince the sched folks that there is valid use cases
> for exports of these functions, and _then_ enable modularity.
> 
> I didn't try to build it as a module, so I can't say what the sched
> dependencies were, or if they looked valid.  But I have seen the sched

Reverting Paul's non-module patch and adding "tristate" "default m", it builds
cleanly, but fails modpost:

ERROR: "sched_set_itmt_core_prio" [drivers/platform/x86/intel_turbo_max_3.ko] undefined!
ERROR: "sched_set_itmt_support" [drivers/platform/x86/intel_turbo_max_3.ko] undefined!

Peter, Google is failing me for a statement from you on EXPORT_SYMBOL'ing sched_
functions, so can you weigh in here? Have you and Srinivas already discussed
this and determined it was unacceptable to export the two sched itmt related
functions above?

Thanks,

> folks being not impressed at how certain arch specific PM/freq stuff has
> been implemented in the past, which is why I mention it as a possibility.
> 
> P.
> --
> 
> > 
> > I am asking because the description of the driver says:
> > 
> > "This driver is only required when the system is not using Hardware
> > P-States (HWP). In HWP mode, priority can be read from ACPI tables."
> > 
> > This pretty much implies that this driver will be useless on a wide
> > range (maybe even the majority?) of systems. Given this, not being
> > forced to build the driver into the kernel would seem preferable.
> > 
> > Thanks, -- Jean Delvare SUSE L3 Support
> 

-- 
Darren Hart
VMware Open Source Technology Center

Re: intel_turbo_max_3 non-modularity

[Srinivas Pandruvada]

On Thu, 2017-04-27 at 14:48 -0700, Darren Hart wrote:
> On Mon, Apr 24, 2017 at 10:02:18AM -0400, Paul Gortmaker wrote:
> > 
> > [intel_turbo_max_3 non-modularity] On 24/04/2017 (Mon 11:31) Jean
> > Delvare wrote:
> > 
[...]

> > > 
> > Re-reading the thread, it was stated that the driver needed exports
> > of
> > some scheduler functions that weren't currently exported.  To me,
> > that
> > means one of two things -- the driver is poking at scheduler
> > internals
> > that it shouldn't be and needs to be modified accordingly, or that
> > someone needs to convince the sched folks that there is valid use
> > cases
> > for exports of these functions, and _then_ enable modularity.
> > 
> > I didn't try to build it as a module, so I can't say what the sched
> > dependencies were, or if they looked valid.  But I have seen the
> > sched
> 
> Reverting Paul's non-module patch and adding "tristate" "default m",
> it builds
> cleanly, but fails modpost:
> 
> ERROR: "sched_set_itmt_core_prio"
> [drivers/platform/x86/intel_turbo_max_3.ko] undefined!
> ERROR: "sched_set_itmt_support"
> [drivers/platform/x86/intel_turbo_max_3.ko] undefined!
> 
> Peter, Google is failing me for a statement from you on
> EXPORT_SYMBOL'ing sched_
> functions, so can you weigh in here? Have you and Srinivas already
> discussed
> this and determined it was unacceptable to export the two sched itmt
> related
> functions above?
Before to this patch, only intel_pstate driver called these two
sched_xx() functions. intel_pstate can be only be built-in, so no
effort was made to export these functions when they were developed to
add ITMT support.
But turbo_max_3 driver driver will only load on a subset of non HWP
Broadwell systems, making module is not a bad idea. To do that we can
export these two sched_xx() functions as they are only related to ITMT
function anyway.
If Peter agrees, I can send a patch to change this.


Thanks,
Srinivas

Re: intel_turbo_max_3 non-modularity

[Peter Zijlstra]

On Thu, Apr 27, 2017 at 04:18:46PM -0700, Srinivas Pandruvada wrote:

> Before to this patch, only intel_pstate driver called these two
> sched_xx() functions. intel_pstate can be only be built-in, so no
> effort was made to export these functions when they were developed to
> add ITMT support.
> But turbo_max_3 driver driver will only load on a subset of non HWP
> Broadwell systems, making module is not a bad idea. To do that we can
> export these two sched_xx() functions as they are only related to ITMT
> function anyway.
> If Peter agrees, I can send a patch to change this.

I would really hate to expose sched_set_itmt_core_prio() to modules. Too
much potential for trainwrecks.

Also, this driver is dinky, making it a modules doesn't save anything
much,

Re: intel_turbo_max_3 non-modularity

[Jean Delvare]

Hi Peter,

On Fri, 28 Apr 2017 09:55:14 +0200, Peter Zijlstra wrote:
> On Thu, Apr 27, 2017 at 04:18:46PM -0700, Srinivas Pandruvada wrote:
> 
> > Before to this patch, only intel_pstate driver called these two
> > sched_xx() functions. intel_pstate can be only be built-in, so no
> > effort was made to export these functions when they were developed to
> > add ITMT support.
> > But turbo_max_3 driver driver will only load on a subset of non HWP
> > Broadwell systems, making module is not a bad idea. To do that we can
> > export these two sched_xx() functions as they are only related to ITMT
> > function anyway.
> > If Peter agrees, I can send a patch to change this.
> 
> I would really hate to expose sched_set_itmt_core_prio() to modules. Too
> much potential for trainwrecks.

These functions are already available. Not exporting them only forces
users to be built-in. This doesn't prevent anything, and in practice
makes things worse.

I don't really buy the argument of not letting legitimate users access
something they need in the name of preventing hypothetical abusers from
breaking things. If that ever happens, they will get the pain they
deserve. How do you care? If you want to put any limitations on what
people should be using these functions for, let's just document it?

> Also, this driver is dinky, making it a modules doesn't save anything
> much,

Not buying this either, sorry. Yes, size is one thing to consider for
such decisions, but that's only one factor. Amongst other factors is
how many users need the code in question. With:

static const struct x86_cpu_id itmt_legacy_cpu_ids[] = {
	ICPU(INTEL_FAM6_BROADWELL_X),
	{}
};
MODULE_DEVICE_TABLE(x86cpu, itmt_legacy_cpu_ids);

and

	if (boot_cpu_has(X86_FEATURE_HWP))
		return -ENODEV;

it is pretty clear to me that the share of users that will need this
driver is fairly limited.

If you would only consider the code size then there are several hundreds
of in-tree device drivers that shouldn't be modularized either, as they
are the same size or less. But you have to think globally, not
per-driver. Generic distribution kernels can only fly as long as all
hardware-specific code can be modularized.

-- 
Jean Delvare
SUSE L3 Support

Re: intel_turbo_max_3 non-modularity

[Peter Zijlstra]

On Fri, Apr 28, 2017 at 03:20:21PM +0200, Jean Delvare wrote:
> Hi Peter,
> 
> On Fri, 28 Apr 2017 09:55:14 +0200, Peter Zijlstra wrote:
> > On Thu, Apr 27, 2017 at 04:18:46PM -0700, Srinivas Pandruvada wrote:
> > 
> > > Before to this patch, only intel_pstate driver called these two
> > > sched_xx() functions. intel_pstate can be only be built-in, so no
> > > effort was made to export these functions when they were developed to
> > > add ITMT support.
> > > But turbo_max_3 driver driver will only load on a subset of non HWP
> > > Broadwell systems, making module is not a bad idea. To do that we can
> > > export these two sched_xx() functions as they are only related to ITMT
> > > function anyway.
> > > If Peter agrees, I can send a patch to change this.
> > 
> > I would really hate to expose sched_set_itmt_core_prio() to modules. Too
> > much potential for trainwrecks.
> 
> These functions are already available. Not exporting them only forces
> users to be built-in. This doesn't prevent anything, and in practice
> makes things worse.

How are they available? The only way a module can access this is by
grubbing around kernel internals, basically doing a manual link stage.

Its obvious that anybody doing something that like that gets to keep
whatever pieces they end up with.

Exporting a symbol OTOH will announce that its OK to use; something very
much not the case here. I really don't want joe-random module tinkerer
to _ever_ even consider using this function.

In fact, if I could I would restrict / remove a number of functions
already exported. I'd love to have something like

  EXPORT_SYMBOL_MOD(symbol, "modnames");

That only allows those whitelisted modules to touch that symbol.

Randomly exporting everything just because one stupid little module
thinks it might need it, hell no.

I would really rather sooner delete this driver than export that symbol.

Re: intel_turbo_max_3 non-modularity

[Darren Hart]

On Fri, Apr 28, 2017 at 03:46:58PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 28, 2017 at 03:20:21PM +0200, Jean Delvare wrote:
> > Hi Peter,
> > 
> > On Fri, 28 Apr 2017 09:55:14 +0200, Peter Zijlstra wrote:
> > > On Thu, Apr 27, 2017 at 04:18:46PM -0700, Srinivas Pandruvada wrote:
> > > 
> > > > Before to this patch, only intel_pstate driver called these two
> > > > sched_xx() functions. intel_pstate can be only be built-in, so no
> > > > effort was made to export these functions when they were developed to
> > > > add ITMT support.
> > > > But turbo_max_3 driver driver will only load on a subset of non HWP
> > > > Broadwell systems, making module is not a bad idea. To do that we can
> > > > export these two sched_xx() functions as they are only related to ITMT
> > > > function anyway.
> > > > If Peter agrees, I can send a patch to change this.
> > > 
> > > I would really hate to expose sched_set_itmt_core_prio() to modules. Too
> > > much potential for trainwrecks.
> > 
> > These functions are already available. Not exporting them only forces
> > users to be built-in. This doesn't prevent anything, and in practice
> > makes things worse.
> 
> How are they available? The only way a module can access this is by
> grubbing around kernel internals, basically doing a manual link stage.

Well it's a rare thing, but I don't agree. The driver can use this function
already, to do so it just builds itself in and not as a module. So as Jean
points out, the only affect this policy has is to encourage people not to build
as modules, which makes the job of the distributions all that much harder.

Further, we (the kernel community) have made it abundantly clear that we
couldn't care less about out of tree drivers, so they shouldn't even be part of
this discussion. So the only code that matters is in-tree drivers, and they can
already use this....

Now for my part, I'm starting to take the very unpopular position that the
"stable-abi-nonsense" isn't all that it's cracked up to be, and we would see
better upstream behavior if we provided a couple of layers of APIs, core and
subsystem, and guaranteed some term of stability for them so leaf node drivers
could be developed without such a strong binding to the kernel. But - I suspect
I'm fairly alone in this position among kernel maintainers - but if that's the
case, then this argument for *not* exporting a function seems to be coming from
the wrong side of this divide.

> 
> Its obvious that anybody doing something that like that gets to keep
> whatever pieces they end up with.
> 
> Exporting a symbol OTOH will announce that its OK to use; something very
> much not the case here. I really don't want joe-random module tinkerer
> to _ever_ even consider using this function.

What makes this one so special? It is really unclear to me what criteria we use
to decide which functions we export and which we do not. Given that any in tree
driver can use most any function, this policy doesn't appear to be able to have
a positive impact on in-tree drivers.

> In fact, if I could I would restrict / remove a number of functions
> already exported. I'd love to have something like
> 
>   EXPORT_SYMBOL_MOD(symbol, "modnames");
> 
> That only allows those whitelisted modules to touch that symbol.
> 
> Randomly exporting everything just because one stupid little module
> thinks it might need it, hell no.
> 
> I would really rather sooner delete this driver than export that symbol.
> 

So what makes the driver stupid? Is it doing something fundamentally wrong? If
so, then perhaps I failed to spot it and never should have merged it? If not,
then the fault doesn't lie with the driver.

Like I said, it's rare that I will disagree with Peter, but as things stand, I
don't have the context that I would need to support this policy independently.
So the next time something similar comes up, we'll be right back here. So I
either need to dig up that context, or we may need to address the issue
differently.

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

Re: intel_turbo_max_3 non-modularity

[Peter Zijlstra]

On Fri, Apr 28, 2017 at 08:49:54AM -0700, Darren Hart wrote:
> On Fri, Apr 28, 2017 at 03:46:58PM +0200, Peter Zijlstra wrote:

> > How are they available? The only way a module can access this is by
> > grubbing around kernel internals, basically doing a manual link stage.
> 
> Well it's a rare thing, but I don't agree. The driver can use this function
> already, to do so it just builds itself in and not as a module.

And _that_ is the crucial difference. It means that the source needs to
be available to whomever builds the kernel.

It means that whoever builds the kernel can validate behaviour.

All that goes out the window the moment you expose an interface to
modules.

> Further, we (the kernel community) have made it abundantly clear that we
> couldn't care less about out of tree drivers, so they shouldn't even be part of
> this discussion. So the only code that matters is in-tree drivers, and they can
> already use this....

I don't much care for out-of-tree crap (as you well know), but the fact
is that it exists and people do use it. Heck some binary only modules
are even quite common/popular :-(

So I think its realistic to be weary of our module interface.

> What makes this one so special?

It can completely mess up the load-balancer if used in creative ways. I
just don't want to spend a minute having to worry about that.

Re: intel_turbo_max_3 non-modularity

[Darren Hart]

On Fri, Apr 28, 2017 at 06:34:17PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 28, 2017 at 08:49:54AM -0700, Darren Hart wrote:
> > On Fri, Apr 28, 2017 at 03:46:58PM +0200, Peter Zijlstra wrote:
> 
> > > How are they available? The only way a module can access this is by
> > > grubbing around kernel internals, basically doing a manual link stage.
> > 
> > Well it's a rare thing, but I don't agree. The driver can use this function
> > already, to do so it just builds itself in and not as a module.
> 
> And _that_ is the crucial difference. It means that the source needs to
> be available to whomever builds the kernel.
> 
> It means that whoever builds the kernel can validate behaviour.
> 
> All that goes out the window the moment you expose an interface to
> modules.
> 
> > Further, we (the kernel community) have made it abundantly clear that we
> > couldn't care less about out of tree drivers, so they shouldn't even be part of
> > this discussion. So the only code that matters is in-tree drivers, and they can
> > already use this....
> 
> I don't much care for out-of-tree crap (as you well know), but the fact
> is that it exists and people do use it. Heck some binary only modules
> are even quite common/popular :-(
> 
> So I think its realistic to be weary of our module interface.
> 
> > What makes this one so special?
> 
> It can completely mess up the load-balancer if used in creative ways. I
> just don't want to spend a minute having to worry about that.

OK, while I maintain there are some inconsistencies in our policies, I don't
disagree that there are core kernel internals that should not be exposed to
drivers (whether modular or not) and these seem like such functions.

Approaching this from that angle, as I mentioned just a moment ago, Peter would
you be willing to entertain the idea of an interface which allows modular
drivers to advise the scheduler on what they know of the system without giving
them explicit control? And leaving any actuation on that information to the
scheduler?

-- 
Darren Hart
VMware Open Source Technology Center

Re: intel_turbo_max_3 non-modularity

[Peter Zijlstra]

On Fri, Apr 28, 2017 at 01:58:39PM -0700, Darren Hart wrote:
> 
> OK, while I maintain there are some inconsistencies in our policies, I don't
> disagree that there are core kernel internals that should not be exposed to
> drivers (whether modular or not) and these seem like such functions.
> 
> Approaching this from that angle, as I mentioned just a moment ago, Peter would
> you be willing to entertain the idea of an interface which allows modular
> drivers to advise the scheduler on what they know of the system without giving
> them explicit control? And leaving any actuation on that information to the
> scheduler?

People are free to propose patches, they'll land in the regular
(terminally backlogged) queue to stare at :-)

Re: intel_turbo_max_3 non-modularity

[Jean Delvare]

Hi Darren and Srinivas,

Thanks a lot for stepping in.

On Thu, 27 Apr 2017 16:18:46 -0700, Srinivas Pandruvada wrote:
> On Thu, 2017-04-27 at 14:48 -0700, Darren Hart wrote:
> > Reverting Paul's non-module patch and adding "tristate" "default m",
> > it builds cleanly, but fails modpost:

This was on my to-do list, thanks for saving me some time :-)

For the record, Paul's patch also added a few missing includes, which
should be preserved.

> > ERROR: "sched_set_itmt_core_prio"
> > [drivers/platform/x86/intel_turbo_max_3.ko] undefined!
> > ERROR: "sched_set_itmt_support"
> > [drivers/platform/x86/intel_turbo_max_3.ko] undefined!
> > 
> > Peter, Google is failing me for a statement from you on EXPORT_SYMBOL'ing sched_
> > functions, so can you weigh in here? Have you and Srinivas already discussed
> > this and determined it was unacceptable to export the two sched itmt related
> > functions above?
>
> Before to this patch, only intel_pstate driver called these two
> sched_xx() functions. intel_pstate can be only be built-in, so no
> effort was made to export these functions when they were developed to
> add ITMT support.

By the way, is there any fundamental reason why intel_pstate could not
be modularized? Or was it just another instance of needing functions
which were not exported yet? I know that the intel_pstate driver is
widely used nowadays, but still, this is a CPU-specific driver that
not all x86 users need, and it isn't slim. It would be nice to be able
to build it as a module too.

> But turbo_max_3 driver driver will only load on a subset of non HWP
> Broadwell systems, making module is not a bad idea. To do that we can

It was also my understanding that the scope of this driver was limited,
which is why I was worried that it could only be built-in.

> export these two sched_xx() functions as they are only related to ITMT
> function anyway.
> If Peter agrees, I can send a patch to change this.

That would be great.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: intel_turbo_max_3 non-modularity

[Srinivas Pandruvada]

Hi Jean,


[...]

> > 
> By the way, is there any fundamental reason why intel_pstate could
> not
> be modularized? Or was it just another instance of needing functions
> which were not exported yet? I know that the intel_pstate driver is
> widely used nowadays, but still, this is a CPU-specific driver that
> not all x86 users need, and it isn't slim. It would be nice to be
> able
> to build it as a module too.
> 
I wish it can be a module. The problem is the race between acpi-cpufreq 
and intel_pstate when built as modules. Since every distribution will
have both of these configs set.
Based on the load order any of this driver can load as cpufreq scaling
driver. We want intel_pstate should be tried first and if can't be
supported then acpi-cpufreq should be used.

There are two PM trace event this depends on, which require built in
but that can be solved.

So if anybody has suggestion on how to solve this, this can be done.

Thanks,
Srinivas

Re: intel_turbo_max_3 non-modularity

[Jean Delvare]

Hi Paul,

On Mon, 24 Apr 2017 10:02:18 -0400, Paul Gortmaker wrote:
> [intel_turbo_max_3 non-modularity] On 24/04/2017 (Mon 11:31) Jean Delvare wrote:
> 
> > I see that the intel_turbo_max_3 driver was originally supposed to be
> > modular, and then support for that possibility was removed. Is there
> > any fundamental reason why this driver can't be built as a module?
> 
> Re-reading the thread, it was stated that the driver needed exports of
> some scheduler functions that weren't currently exported.  To me, that
> means one of two things -- the driver is poking at scheduler internals
> that it shouldn't be and needs to be modified accordingly, or that
> someone needs to convince the sched folks that there is valid use cases
> for exports of these functions, and _then_ enable modularity.
> 
> I didn't try to build it as a module, so I can't say what the sched
> dependencies were, or if they looked valid.  But I have seen the sched
> folks being not impressed at how certain arch specific PM/freq stuff has
> been implemented in the past, which is why I mention it as a possibility.

Thanks for the explanations. I understand that the purpose of your
patch (commit af050abb5c2e, "platform/x86: intel_turbo_max_3: make it
explicitly non-modular") was to get things square, and I have seen many
similar patches from you in the past. However please realize that going
for the easiest solution (dropping modular support altogether) isn't
always the best thing to do.

It's the same story as fixing compiler warnings. One should either look
for the right way to fix them, or leave things as they are, to give
others a chance to step in. Similar to compiler warnings, half-modular
code is also an opportunity to make things better. In my experience,
once the code is cleanly non-modular, the chances that someone looks
into why it is so and whether it could be modularized get significantly
lower.

Non-modular drivers do not scale. This is a serious issue considering
the growth rate of the kernel.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: intel_turbo_max_3 non-modularity

[Paul Gortmaker]

[Re: intel_turbo_max_3 non-modularity] On 28/04/2017 (Fri 09:49) Jean Delvare wrote:

> Hi Paul,
> 
> On Mon, 24 Apr 2017 10:02:18 -0400, Paul Gortmaker wrote:
> > [intel_turbo_max_3 non-modularity] On 24/04/2017 (Mon 11:31) Jean Delvare wrote:
> > 
> > > I see that the intel_turbo_max_3 driver was originally supposed to be
> > > modular, and then support for that possibility was removed. Is there
> > > any fundamental reason why this driver can't be built as a module?
> > 
> > Re-reading the thread, it was stated that the driver needed exports of
> > some scheduler functions that weren't currently exported.  To me, that
> > means one of two things -- the driver is poking at scheduler internals
> > that it shouldn't be and needs to be modified accordingly, or that
> > someone needs to convince the sched folks that there is valid use cases
> > for exports of these functions, and _then_ enable modularity.
> > 
> > I didn't try to build it as a module, so I can't say what the sched
> > dependencies were, or if they looked valid.  But I have seen the sched
> > folks being not impressed at how certain arch specific PM/freq stuff has
> > been implemented in the past, which is why I mention it as a possibility.
> 
> Thanks for the explanations. I understand that the purpose of your
> patch (commit af050abb5c2e, "platform/x86: intel_turbo_max_3: make it
> explicitly non-modular") was to get things square, and I have seen many
> similar patches from you in the past. However please realize that going
> for the easiest solution (dropping modular support altogether) isn't
> always the best thing to do.

I don't think saying that "always using the easiest solution" is at all
an accurate characterization.  I've nearly always indicated that
conversion to tristate was an option _if_ there was a use case and/or if
maintainers wanted that; and indeed I've converted several drivers to
tristate when requested.  I've also indicated many times why converting
to tristate can't be the default choice.  I won't repeat them all again
here.

Also, as you've now seen, the scheduler exports that I warned about are
indeed contentious, so the non-modular path taken here was valid, given
that background knowledge.

> 
> It's the same story as fixing compiler warnings. One should either
> look for the right way to fix them, or leave things as they are, to
> give others a chance to step in. Similar to compiler warnings,
> half-modular code is also an opportunity to make things better. In my
> experience, once the code is cleanly non-modular, the chances that
> someone looks into why it is so and whether it could be modularized
> get significantly lower.

I'd counter that the dead code would remain there untouched.  At least
this way we are aware of what is dead code, and by shining a light on
that, we can decide whether to remove it or properly modularize it; and
now because of doing this we do indeed have more modular drivers than we
had before.

> 
> Non-modular drivers do not scale. This is a serious issue considering
> the growth rate of the kernel.

Again, this is an oversimplification that doesn't reflect reality.
There are many valid reasons why some drivers be built-in only.  The
whole world isn't a distro looking to say "=m" to every config option
simply because they don't have the time or resources to actually triage
them all and see what is relevant for their use case(es).

Paul.

Re: intel_turbo_max_3 non-modularity

[Darren Hart]

On Fri, Apr 28, 2017 at 01:44:37PM -0400, Paul Gortmaker wrote:
> [Re: intel_turbo_max_3 non-modularity] On 28/04/2017 (Fri 09:49) Jean Delvare wrote:
> 
> > Hi Paul,
> > 
> > On Mon, 24 Apr 2017 10:02:18 -0400, Paul Gortmaker wrote:
> > > [intel_turbo_max_3 non-modularity] On 24/04/2017 (Mon 11:31) Jean Delvare wrote:
> > > 
> > > > I see that the intel_turbo_max_3 driver was originally supposed to be
> > > > modular, and then support for that possibility was removed. Is there
> > > > any fundamental reason why this driver can't be built as a module?
> > > 
> > > Re-reading the thread, it was stated that the driver needed exports of
> > > some scheduler functions that weren't currently exported.  To me, that
> > > means one of two things -- the driver is poking at scheduler internals
> > > that it shouldn't be and needs to be modified accordingly, or that
> > > someone needs to convince the sched folks that there is valid use cases
> > > for exports of these functions, and _then_ enable modularity.
> > > 
> > > I didn't try to build it as a module, so I can't say what the sched
> > > dependencies were, or if they looked valid.  But I have seen the sched
> > > folks being not impressed at how certain arch specific PM/freq stuff has
> > > been implemented in the past, which is why I mention it as a possibility.
> > 
> > Thanks for the explanations. I understand that the purpose of your
> > patch (commit af050abb5c2e, "platform/x86: intel_turbo_max_3: make it
> > explicitly non-modular") was to get things square, and I have seen many
> > similar patches from you in the past. However please realize that going
> > for the easiest solution (dropping modular support altogether) isn't
> > always the best thing to do.
> 
> I don't think saying that "always using the easiest solution" is at all
> an accurate characterization.  I've nearly always indicated that
> conversion to tristate was an option _if_ there was a use case and/or if
> maintainers wanted that; and indeed I've converted several drivers to
> tristate when requested.  I've also indicated many times why converting
> to tristate can't be the default choice.  I won't repeat them all again
> here.
> 
> Also, as you've now seen, the scheduler exports that I warned about are
> indeed contentious, so the non-modular path taken here was valid, given
> that background knowledge.

All true. I appreciate the contributions, please keep them coming :-)

> 
> > 
> > It's the same story as fixing compiler warnings. One should either
> > look for the right way to fix them, or leave things as they are, to
> > give others a chance to step in. Similar to compiler warnings,
> > half-modular code is also an opportunity to make things better. In my
> > experience, once the code is cleanly non-modular, the chances that
> > someone looks into why it is so and whether it could be modularized
> > get significantly lower.
> 
> I'd counter that the dead code would remain there untouched.  At least
> this way we are aware of what is dead code, and by shining a light on
> that, we can decide whether to remove it or properly modularize it; and
> now because of doing this we do indeed have more modular drivers than we
> had before.
> 
> > 
> > Non-modular drivers do not scale. This is a serious issue considering
> > the growth rate of the kernel.
> 
> Again, this is an oversimplification that doesn't reflect reality.
> There are many valid reasons why some drivers be built-in only.  The
> whole world isn't a distro looking to say "=m" to every config option
> simply because they don't have the time or resources to actually triage
> them all and see what is relevant for their use case(es).
> 

The point is valid though, and something we need to be keeping an eye on,
especially with all the non-architectural SoC feature drivers we're seeing code
roll in for. I appreciate you raising the concern Jean.

To Paul's point, several things should just be off in Generic distro kernels,
and I think SILEAD_DMI is particularly good example of something that can be off
in all such kernels with a high probability of impacting 0 users. This
particular driver (turbo_max_3) however, is one that I would think Intel would
want to see enabled.

+Srinivas
+Peter Z

I'm not going to win this argument with Peter Z on exporting those two
functions, he's taken a strong position on that with specific reasons, and
that's his call to make. My advice to Srinivas would be to discuss the needs of
the driver with Peter and see if some sort of abstraction which can be
EXPORT_SYMBOL'd can be added, which provides the necessary functionality, but
doesn't expose so much control of scheduler internals (load balancing) to all
modules. Something similar to what Steven Rostedt did with sched_clock()
(local_trace_clock()). Once that is available, we can make this one modular.

Failing that, this discussion needs to move to the scheduler as there is nothing
we can do about it here other than delete the driver, which I'm not inclined to do.

-- 
Darren Hart
VMware Open Source Technology Center

Re: intel_turbo_max_3 non-modularity

[Peter Zijlstra]

On Fri, Apr 28, 2017 at 01:49:49PM -0700, Darren Hart wrote:
> > > Non-modular drivers do not scale. This is a serious issue considering
> > > the growth rate of the kernel.
> > 
> > Again, this is an oversimplification that doesn't reflect reality.
> > There are many valid reasons why some drivers be built-in only.  The
> > whole world isn't a distro looking to say "=m" to every config option
> > simply because they don't have the time or resources to actually triage
> > them all and see what is relevant for their use case(es).
> > 
> 
> The point is valid though, and something we need to be keeping an eye on,
> especially with all the non-architectural SoC feature drivers we're seeing code
> roll in for. I appreciate you raising the concern Jean.

FWIW:

There's things like ACPI-Processor-Idle and intel_idle that need to be
built-in for correctness sake.

They can mark the TSC unstable, and this should be done before we run
userspace, which with unstable TSC could see wobbly time before we kill
the TSC.

This is a somewhat non-obvious case, and on the face of it those could
be modular drivers. But building them as modules gets them loaded far
too late if ever.

These are subtle things and easily overlooked.

Re: intel_turbo_max_3 non-modularity

[Darren Hart]

On Fri, Apr 28, 2017 at 11:34:34PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 28, 2017 at 01:49:49PM -0700, Darren Hart wrote:
> > > > Non-modular drivers do not scale. This is a serious issue considering
> > > > the growth rate of the kernel.
> > > 
> > > Again, this is an oversimplification that doesn't reflect reality.
> > > There are many valid reasons why some drivers be built-in only.  The
> > > whole world isn't a distro looking to say "=m" to every config option
> > > simply because they don't have the time or resources to actually triage
> > > them all and see what is relevant for their use case(es).
> > > 
> > 
> > The point is valid though, and something we need to be keeping an eye on,
> > especially with all the non-architectural SoC feature drivers we're seeing code
> > roll in for. I appreciate you raising the concern Jean.
> 
> FWIW:
> 
> There's things like ACPI-Processor-Idle and intel_idle that need to be
> built-in for correctness sake.
> 
> They can mark the TSC unstable, and this should be done before we run
> userspace, which with unstable TSC could see wobbly time before we kill
> the TSC.
> 
> This is a somewhat non-obvious case, and on the face of it those could
> be modular drivers. But building them as modules gets them loaded far
> too late if ever.
> 
> These are subtle things and easily overlooked.

Thanks Peter, I will be sure to do my due diligence in determining why something
may be bool before applying incoming patches to change that. This isn't clearly
documented in intel_idle for example, but it is at least explicitly described in
the git log for the Kconfig.

-- 
Darren Hart
VMware Open Source Technology Center