From owner-svn-src-head@freebsd.org Sun Jan 14 03:59:08 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 917A2E77209 for ; Sun, 14 Jan 2018 03:59:08 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-it0-x232.google.com (mail-it0-x232.google.com [IPv6:2607:f8b0:4001:c0b::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 52F2368F0C for ; Sun, 14 Jan 2018 03:59:08 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-it0-x232.google.com with SMTP id x42so13291414ita.4 for ; Sat, 13 Jan 2018 19:59:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=IL3DS4ThE33TlnMVBf7OTODmwHMQfFf3dmQ0Ib5ZJ/4=; b=Hfmuv+ut7CYsnA5UATQprmOzrJiMMJaL0fYy3jkFOatbJ3QtCkn+AIldYVslHDJCnK nUlE9KtSkQU5MkBr0/gWjTk0KS+ur4okCqGBC8l+vIBAKrXu6BFT4tSQkA3h0qayc/nm aOa3a8ryQOg+JSvC1v6perotFoMzrPt/YCNaeJXBO0TOsJ5/apMgYkLeFhetr+P76krl Qe9HhggRLiiqb4IBdXCo6/i89cXZxSs85lI7z55/6gT9Nhp8ZF0N+uYi3u4fl37Ndwya qvgUOC96/RL9y9wgz4aOR+1pCoOCJHhQqvdSOBSPa19zwpjV8HG9s8eha7xXN5ue6Bn7 wmMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=IL3DS4ThE33TlnMVBf7OTODmwHMQfFf3dmQ0Ib5ZJ/4=; b=KCNUUtGSQ0XvOKt0Db6Prh7RfR2k1Y/eemcNQyoEdd4LsxViciWu4PdNtMqW3C7MFB qdOR7Nv86Htrq5CMVkTurVEJzdpbxmBGI9fFH8JKm9/9aT2HoF9OY//9QWCajXvdZW5c zQ5q2wg0c95ylTWROHeRdu9LkewyM3skQwO9XFvzGnJ1G5rLGnWJWF46yyN1znWqpNVg MoLSWFAgpbtxJ5L8y21rUI92zUGkIXn0vG/N3QN8HyWJLrJNOxyMI4aYQRUzmm+iSUdo Pv5OPoB3PNjdSvwass8lE+w33yY/JhAH8VIqIgtOEC1PYbHAONMAHzwh6Z37B1nrTzP+ H24Q== X-Gm-Message-State: AKwxyte7iLUYKcSeR9hZ/I/VBfSKhWys6MGjl1zhB64YjHUW8btYniiy nBVurBw5YIcaaMp1F6frbKU9eT5WBaj9Kxjbijv/0w== X-Google-Smtp-Source: ACJfBou8l5r/9QFDanct38Szsg7YLa8VJZ4zlrgRieK/btCIo8brxqrOJ7hf3hhJJLVDWtxWOOjpmbSIufUHvgC8AmI= X-Received: by 10.36.91.210 with SMTP id g201mr10265388itb.50.1515902347470; Sat, 13 Jan 2018 19:59:07 -0800 (PST) MIME-Version: 1.0 Sender: wlosh@bsdimp.com Received: by 10.79.199.131 with HTTP; Sat, 13 Jan 2018 19:59:06 -0800 (PST) X-Originating-IP: [2603:300b:6:5100:18a2:a4f7:170:8dd9] In-Reply-To: <20180113203848.L2336@besplex.bde.org> References: <201801101459.w0AExJWM055025@repo.freebsd.org> <20180113203848.L2336@besplex.bde.org> From: Warner Losh Date: Sat, 13 Jan 2018 20:59:06 -0700 X-Google-Sender-Auth: bHXzn6qta7EPkhTZ2NSdEVRgZ9E Message-ID: Subject: Re: svn commit: r327767 - in head/sys: conf i386/bios i386/conf i386/isa To: Bruce Evans Cc: Warner Losh , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.25 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 14 Jan 2018 03:59:08 -0000 [trimmed, sorry ] On Sat, Jan 13, 2018 at 4:56 AM, Bruce Evans wrote: > On Wed, 10 Jan 2018, Warner Losh wrote: > > Log: >> Retire pmtimer driver. Move time fixing into apm driver. Move >> Iwasaki-san's copyright over. Remove FIXME code that couldn't possibly >> work. Call tc_settime() with our estimate of the delta we've been >> alseep (the one we print) to adjust the time. Not sure what to do >> about callouts, so keep the small #ifdef in place there. >> > > The FIXME code might have worked before timecounters, but never worked in > any committed version. You seem to have updated it correctly for > timecounters in this commit, but then broke it by removing it in a later > commit (see another reply). > > This commit obfuscates the fix by combining moving of the file with > changing > it. svn is also very broken -- it doesn't show history of the file under > its old name. This is more broken than usual -- since the file was merged > into a different file, svn doesn't show history of the file under its new > name either. > > The callout fixup code is more broken. It never compiled in any committed > version, since it uses data structures and logic that went away before it > was committed. The option for it (APM_FIXUP_CALLTODO) is so broken that it > is not a supported option or in LINT, so it is not normally noticed that > the code under it never compiled. > > Long callouts are especially broken. E.g., nanosleep(1 hour) at least > used to sleep for 2 hours if the system is suspended for 1 hour during > the sleep. I haven't noticed any fixes for this. Callouts mostly use > relative timeouts, and relative timeouts just don't work across > suspensions. > > Actually, I have noticed some fixes for nanosleep(). Another bug in it > was that it is specified to sleep in the time specified by the clock id > CLOCK_REALTIME, but used to sleep in the time specified by the clock id > CLOCK_MONOTONIC (the latter should be as close as possible to the former > except for leap seconds adjustments, but is quite broken -- it doesn't > count suspended time, and non-leap seconds settings of CLOCK_REALTIME > also make CLOCK_MONOTONIC useless for determining physical time > differences. > nanosleep() now sleeps on the correct clock id, and clock_nanosleep() is > now implemented. This is complicated, and I haven't checked if the fix > works. It looks to simple to work. To work, it needs to do things like > adjust timers whenever the timecounter jumps (mainly for resume and leap > seconds adjustments). APM_FIXUP_CALLTODO only tries to handle the easier > case of resume. Right, none of that is fixed. Modified: head/sys/conf/files.i386 >> ============================================================ >> ================== >> --- head/sys/conf/files.i386 Wed Jan 10 14:58:58 2018 (r327766) >> +++ head/sys/conf/files.i386 Wed Jan 10 14:59:19 2018 (r327767) >> @@ -520,7 +520,6 @@ i386/ibcs2/ibcs2_util.c optional ibcs2 >> i386/ibcs2/ibcs2_xenix.c optional ibcs2 >> i386/ibcs2/ibcs2_xenix_sysent.c optional ibcs2 >> i386/ibcs2/imgact_coff.c optional ibcs2 >> -i386/isa/pmtimer.c optional pmtimer >> i386/isa/prof_machdep.c optional profiling-routine >> i386/linux/imgact_linux.c optional compat_linux >> i386/linux/linux_dummy.c optional compat_linux >> > > pmtimer used to be needed all systems with acpi except amd64, because the > acpi fixup of timecounters was bogusly ifdefed for amd64 only (or was not > done by acpi in very old versions?). > > The pmtimer code for this fixup is still better tha the acpi code, except > the FIXME made it unused and the next commit dumbs it down to worse than > the acpi code (the acpi code is at least simpler). > > Modified: head/sys/i386/bios/apm.c >> ============================================================ >> ================== >> --- head/sys/i386/bios/apm.c Wed Jan 10 14:58:58 2018 (r327766) >> +++ head/sys/i386/bios/apm.c Wed Jan 10 14:59:19 2018 (r327767) >> Modified: head/sys/i386/bios/apm.c >> ============================================================ >> ================== >> --- head/sys/i386/bios/apm.c Wed Jan 10 14:58:58 2018 (r327766) >> +++ head/sys/i386/bios/apm.c Wed Jan 10 14:59:19 2018 (r327767) >> @@ -205,7 +232,7 @@ static int >> apm_driver_version(int version) >> { >> struct apm_softc *sc = &apm_softc; >> - >> + >> sc->bios.r.eax = (APM_BIOS << 8) | APM_DRVVERSION; >> sc->bios.r.ebx = 0x0; >> sc->bios.r.ecx = version; >> > > Unrelated style fixes make the important changes harder to see. This > patch was to remove trailing whitespace, but some mail program broke it > by removing the whitespace in the source lines. > > [... further mangled patches for whitespace not shown] Yea, I thought about doing two commits, but they got mixed up... > > @@ -331,12 +358,10 @@ apm_battery_low(void) >> static struct apmhook * >> apm_add_hook(struct apmhook **list, struct apmhook *ah) >> { >> - int s; >> struct apmhook *p, *prev; >> >> APM_DPRINT("Add hook \"%s\"\n", ah->ah_name); >> >> - s = splhigh(); >> if (ah == NULL) >> panic("illegal apm_hook!"); >> prev = NULL; >> @@ -351,30 +376,25 @@ apm_add_hook(struct apmhook **list, struct apmhook >> *ah >> ah->ah_next = prev->ah_next; >> prev->ah_next = ah; >> } >> - splx(s); >> return ah; >> } >> > > This commit also removes spl*() with no mention of this in the log message. > In FreeBSD-4 where spl's were not null, splhigh() was probably wrong, but > it > was much better than the splsoftclock() used in pmtimer.c. Hard clock > interrupts should be disabled, and my fixes for this change softclock to > statclock + XXX. It happens that splstatclock() == splclock() == > splhigh(), > so these splhigh()s were good enough. > > Now the locking is obscure. I think it is just Giant, and that is not > enough. The spl's should not be removed until the locking is done > properly. > The splsoftclock()'s were removed even earlier in pmtimer.c, without doing > any proper locking of course. Right, nothing calls these, except in attach, so I removed the broken locking rather than fixing it. static void >> apm_del_hook(struct apmhook **list, struct apmhook *ah) >> { >> - int s; >> struct apmhook *p, *prev; >> >> - s = splhigh(); >> prev = NULL; >> for (p = *list; p != NULL; prev = p, p = p->ah_next) >> if (p == ah) >> goto deleteit; >> panic("Tried to delete unregistered apm_hook."); >> - goto nosuchnode; >> + return; >> deleteit: >> if (prev != NULL) >> prev->ah_next = p->ah_next; >> else >> *list = p->ah_next; >> -nosuchnode: >> - splx(s); >> } >> > > Locking for device removal is especially necessary and badly done. New-bus > forces lots of Giant locking which is probably not enough. what calls that? Warner