Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU

[Eduardo Habkost]

On Tue, Nov 24, 2020 at 07:29:50PM +0100, Claudio Fontana wrote:
> On 11/24/20 6:08 PM, Eduardo Habkost wrote:
> > On Tue, Nov 24, 2020 at 05:22:07PM +0100, Claudio Fontana wrote:
> >> apply this to the registration of the cpus accel interfaces,
> >>
> >> but this will be also in preparation for later use of this
> >> new module init step to also register per-accel x86 cpu type
> >> interfaces.
> >>
> >> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> >> ---
> > [...]
> >> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
> >> index b4e731cb2b..482f89729f 100644
> >> --- a/accel/qtest/qtest.c
> >> +++ b/accel/qtest/qtest.c
> >> @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = {
> >>  
> >>  static int qtest_init_accel(MachineState *ms)
> >>  {
> >> -    cpus_register_accel(&qtest_cpus);
> >>      return 0;
> >>  }
> >>  
> >> @@ -58,3 +57,12 @@ static void qtest_type_init(void)
> >>  }
> >>  
> >>  type_init(qtest_type_init);
> >> +
> >> +static void qtest_accel_cpu_init(void)
> >> +{
> >> +    if (qtest_enabled()) {
> >> +        cpus_register_accel(&qtest_cpus);
> >> +    }
> >> +}
> >> +
> >> +accel_cpu_init(qtest_accel_cpu_init);
> > 
> > I don't understand why this (and the similar changes on other
> > accelerators) is an improvement.
> > 
> > You are replacing a trivial AccelClass-specific init method with
> > a module_init() function that has a hidden dependency on runtime
> > state.
> > 
> 
> Not a big advantage I agree,
> I think however there is one, in using the existing framework that exists, 
> for the purposes that it was built for.
> 
> As I understand it, the global module init framework is supposed to mark the 
> major initialization steps,
> and this seems to fit the bill.

That seems to be the main source of disagreement.  I don't agree
that's the purpose of module_init().

The module init framework is used to unconditionally register
module-provided entities like option names, QOM types, block
drivers, trace events, etc.  The entities registered by module
init functions represent a passive dynamically loadable piece of
code.

module_init() was never used for initialization of machines,
devices, CPUs, or other runtime state.  We don't have
MODULE_INIT_MONITOR, MODULE_INIT_OBJECTS, MODULE_INIT_MACHINE,
MODULE_INIT_CPUS, MODULE_INIT_DEVICES, etc.

And I'm not convinced we should, because it would hide
dependencies between initialization steps.  It would force us to
make initialization functions affect and depend on global state.

I believe this:

  int main()
  {
    result_of_A = init_A(input_for_A);
    result_of_B = init_B(input_for_B);
    result_of_C = init_C(input_for_C);
  }

is clearer and more maintainable than:

  int main()
  {
    module_init_call(MODULE_INIT_A);  /* result_of_A hidden in global state */
    module_init_call(MODULE_INIT_B);  /* result_of_B hidden in global state */
    module_init_call(MODULE_INIT_C);  /* result_of_C hidden in global state */
  }

> 
> The "hidden" dependency on the fact that accels need to be initialized at 
> that time, is not hidden at all I think,
> it is what this module init step is all about.
> 
> It is explicitly meaning, "_now that the current accelerator is chosen_, 
> perform these initializations".
> 
> But, as you mentioned elsewhere, I will in the meantime anyway squash these 
> things so they do not start fragmented at all, and centralize immediately.

Agreed.  We still need to sort out the disagreement above, or
we'll spend a lot of energy arguing about code.

-- 
Eduardo