cvs commit: src/sys/amd64/amd64 local_apic.c src/sys/i386/acpica madt.c src/sys/i386/i386 local_apic.c src/sys/kern subr_smp.c src/sys/sun4v/mdesc mdesc_init.c

[John Baldwin]

On Friday 21 September 2007 04:38:47 pm Attilio Rao wrote:
> 2007/9/21, John Baldwin <jhb at freebsd.org>:
> > On Tuesday 11 September 2007 06:54:09 pm Attilio Rao wrote:
> > > attilio     2007-09-11 22:54:09 UTC
> > >
> > >   FreeBSD src repository
> > >
> > >   Modified files:
> > >     sys/amd64/amd64      local_apic.c
> > >     sys/i386/acpica      madt.c
> > >     sys/i386/i386        local_apic.c
> > >     sys/kern             subr_smp.c
> > >     sys/sun4v/mdesc      mdesc_init.c
> > >   Log:
> > >   This is a follow-up, cleaning-up commit about recent changes involving
> > >   topology foo functions.
> > >   Working at the patch for topology problems in ia32/amd64 evicted some
> > >   problems regarding functions ordering in the SI_SUB_CPU family of
> > >   SYSINIT'ed subsystems.
> > >   In order to avoid problems with new modified to involved functions, a
> > >   correct ordering is not semantically specified for SI_SUB_CPU 
functions
> > >   (for a larger view of the issue please visit:
> > >   
http://lists.freebsd.org/pipermail/freebsd-current/2007-July/075409.html )
> >
> > Could you clarify exactly what ordering this does?  AFAICT, nothing in
> > cpu_startup() requires the APIC to be up and running.  If there were a
> > dependency, then that would be something for the MD architecture code to 
fix.
> > IIUC, the problem was that you had 3 sysinit functions like this:
> >
> > A - SI_ORDER_FIRST, cpu_startup()
> > B - SI_ORDER_FIRST, apic_init()
> > C - SI_ORDER_SECOND, mp_start()
> >
> > Now mp_start() requires both A and B to have run on x86.  What Peter did
> > originally was to move B to SI_ORDER_SECOND because he found that B needed 
A
> > to run.  That broke because C could end up running before B.  However, the
> > actual patch that went into CVS left A and B as is and instead made them
> > independent of each other so B no longer depends on A.  So, I don't think
> > you've solved an actual problem.
> 
> I know exactly what was the problem as I diagnosed the problem when B
> was in SI_ORDER_FIRST and I diagnosed the problem when B was moved
> temporally in SI_ORDER_SECOND while C was taking this slot.
> 
> Basically what about people adding code in A which introduces
> dependency with B? It results in a problem
> This ordering doesn't break anything and it makes the code safe for
> further changes.

It's a gratuitous change if you aren't actually _fixing_ anything.   I think 
there is a fundamental thing you are missing here though:  it is ok to have 
multiple SYSINIT's with the same subsystem and order if they are independent.  
All device drivers register themselves with new-bus at the same subsystem and 
order for example.  There is no need to "dream up" a false ordering if it is 
not needed.  Now you have made B always come after A.  What if a future code 
change makes A depend on B?  It is best to solve these problems when they 
appear.

> > The madt.c change in this commit is plain wrong however and should be
> > reverted.  If it was correct it would have needed to be done in amd64's
> > madt.c as well.
> 
> Plain wrong?
> You mean maybe that it doesn't matter that (SI_ORDER_CPU -1) was
> shared with mptable_register()? And what about if someone adds
> dependency between the two? madt changes are perfectly working as they
> don't explicitly require to run before of mptable_register().
> And to be precise, there is no madt_register() in amd64, so I have no
> idea what are you speaking about. :(.

I guess you don't understand what these register routines do.  The APIC code 
basically is a driver framework and APIC drivers (well, I call them APIC 
enumerators) register themselves with the core APIC code.  These 
drivers/enumerators are then probed and the highest priority wins.  Having 
these register routines at the same SYSINIT is identical to having all 
new-bus drivers register themselves with new-bus at the same level.  They 
will never have any dependency on each other.

% grep madt_register amd64/acpica/madt.c
static void     madt_register(void *dummy);
madt_register(void *dummy __unused)
SYSINIT(madt_register, SI_SUB_TUNABLES - 1, SI_ORDER_FIRST,
    madt_register, NULL)

AMD64 looks for CPUs at at different time, so it uses SI_SUB_TUNABLES rather 
than SI_SUB_CPU, but it still has both mptable_register() and madt_register() 
at the same level/order and for a good reason. 

> > The sun4v change is bogus as well as mdesc_init() doesn't depend on
> > cpu_startup() on sun4v at all, and it doesn't matter what order they run 
in.
> 
> Reading this I think I see there is basically a misunderstanding of
> what I wanted to do: I don't want to fix an actual problem but the
> idea is to give to any function in SI_SUB_CPU a precise order in order
> to avoid mistakes as the last ones people get trying to fix topology.
> I think this is a legitimate idea.
> And in particular, I don't like the approach 'just do things when you
> need them'.

See above.  Are you going to go throw and assign a static order to ever device 
driver now just in case the one's registration routine might someday depend 
on another?

> > Basically, I think at the least you should revert all the MD changes, and 
the
> > change to subr_smp.c is basically a NOP.
> 
> I'm not going to do this as the patch gives a precise ordering to all
> functions involved in SI_SUB_CPU, doesn't break anything.
> If you don't like the ordering due please just explain why.

It hardcodes an ordering that isn't there and that might have to be reworked 
in the future if a real dependency does show up that is not the order you 
currently just randomly chose.

-- 
John Baldwin