From owner-cvs-src@FreeBSD.ORG Fri Sep 21 20:38:55 2007 Return-Path: <owner-cvs-src@FreeBSD.ORG> 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 12F1D16A565 for <cvs-src@freebsd.org>; Fri, 21 Sep 2007 20:38:53 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from ug-out-1314.google.com (ug-out-1314.google.com [66.249.92.169]) by mx1.freebsd.org (Postfix) with ESMTP id 5F4E813C469 for <cvs-src@freebsd.org>; Fri, 21 Sep 2007 20:38:53 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: by ug-out-1314.google.com with SMTP id a2so711715ugf for <cvs-src@freebsd.org>; Fri, 21 Sep 2007 13:38:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=beta; h=domainkey-signature:received:received:message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; bh=E2tHcgl7mD8UoP6fZANzH3Soa2Iyzre0b1Ngu38wBP0=; b=d0j+jRPESjyQ3D4x/yMsXDDcWjLHZjMSU+uA/MTfjuL65mMP4Quq5xcQkYkxpgtMYTwBOjTEmHC5m7rVeHpWV9l8yuEaXGQMmunEJptpqePv70a11Irlj5K5tONKCjp9UUOMtQ1UtHwr1OCmf3+rbbgEkXfbtVEXT3PR/FtfPWE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=beta; h=received:message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; b=P7RnRPpKh+I04rDQC9m/aJTlf2Ekgmc3Bks4vI+BcMjpsalowEpcjOdeJEQb7vJG/virJbbWBJ/Z9hXypZG9Gghf7NWlzPvCGkg9fSWB7oEsdXMaRPux9oKpETWpTlRWmo+XNScSqQ1tI2nY+DLRYS1QtMulLnB6/4MqafBiR8Y= Received: by 10.78.179.12 with SMTP id b12mr2442618huf.1190407127978; Fri, 21 Sep 2007 13:38:47 -0700 (PDT) Received: by 10.78.120.9 with HTTP; Fri, 21 Sep 2007 13:38:47 -0700 (PDT) Message-ID: <3bbf2fe10709211338j6dbab59am1ad67c86c1a05baa@mail.gmail.com> Date: Fri, 21 Sep 2007 22:38:47 +0200 From: "Attilio Rao" <attilio@freebsd.org> Sender: asmrookie@gmail.com To: "John Baldwin" <jhb@freebsd.org> In-Reply-To: <200709211436.15444.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200709112254.l8BMsB7P074637@repoman.freebsd.org> <200709211436.15444.jhb@freebsd.org> X-Google-Sender-Auth: 17ecf39ace1b002a 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 <cvs-src.freebsd.org> List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-src>, <mailto:cvs-src-request@freebsd.org?subject=unsubscribe> List-Archive: <http://lists.freebsd.org/pipermail/cvs-src> List-Post: <mailto:cvs-src@freebsd.org> List-Help: <mailto:cvs-src-request@freebsd.org?subject=help> List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-src>, <mailto:cvs-src-request@freebsd.org?subject=subscribe> X-List-Received-Date: Fri, 21 Sep 2007 20:38:55 -0000 2007/9/21, John Baldwin <jhb@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