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

[Attilio Rao]

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.

> 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. :(.

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

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

Attilio


-- 
Peace can only be achieved by understanding - A. Einstein