From owner-cvs-src@FreeBSD.ORG Mon Sep 24 16:13:24 2007 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 918BF16A418; Mon, 24 Sep 2007 16:13:24 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from speedfactory.net (mail6.speedfactory.net [66.23.216.219]) by mx1.freebsd.org (Postfix) with ESMTP id 2DE6813C4A8; Mon, 24 Sep 2007 16:13:23 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (unverified [66.23.211.162]) by speedfactory.net (SurgeMail 3.8p) with ESMTP id 211141630-1834499 for multiple; Mon, 24 Sep 2007 12:11:45 -0400 Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.13.8/8.13.8) with ESMTP id l8OGD3hI098542; Mon, 24 Sep 2007 12:13:03 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: "Attilio Rao" Date: Mon, 24 Sep 2007 11:47:48 -0400 User-Agent: KMail/1.9.6 References: <200709112254.l8BMsB7P074637@repoman.freebsd.org> <200709211436.15444.jhb@freebsd.org> <3bbf2fe10709211338j6dbab59am1ad67c86c1a05baa@mail.gmail.com> In-Reply-To: <3bbf2fe10709211338j6dbab59am1ad67c86c1a05baa@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200709241147.49288.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Mon, 24 Sep 2007 12:13:03 -0400 (EDT) X-Virus-Scanned: ClamAV 0.88.3/4378/Mon Sep 24 08:25:35 2007 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: 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 X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 24 Sep 2007 16:13:24 -0000 On Friday 21 September 2007 04:38:47 pm Attilio Rao wrote: > 2007/9/21, John Baldwin : > > 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