From owner-svn-src-head@FreeBSD.ORG Wed Jul 31 05:21:06 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 3EBBA466; Wed, 31 Jul 2013 05:21:06 +0000 (UTC) (envelope-from bright@mu.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id BADBE2A42; Wed, 31 Jul 2013 05:21:05 +0000 (UTC) Received: from kruse-124.4.ixsystems.com (drawbridge.ixsystems.com [206.40.55.65]) by elvis.mu.org (Postfix) with ESMTPSA id C67CB1A3C20; Tue, 30 Jul 2013 22:21:04 -0700 (PDT) Message-ID: <51F89EC0.9010205@mu.org> Date: Tue, 30 Jul 2013 22:21:04 -0700 From: Alfred Perlstein User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/20130620 Thunderbird/17.0.7 MIME-Version: 1.0 To: Glen Barber Subject: Re: svn commit: r253719 - in head: sys/conf sys/dev/watchdog sys/libkern sys/sys usr.sbin/watchdogd References: <201307272047.r6RKl2vI023259@svn.freebsd.org> <20130730112210.GE2571@glenbarber.us> In-Reply-To: <20130730112210.GE2571@glenbarber.us> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Alfred Perlstein , src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 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: Wed, 31 Jul 2013 05:21:06 -0000 On 7/30/13 4:22 AM, Glen Barber wrote: > On Sat, Jul 27, 2013 at 08:47:02PM +0000, Alfred Perlstein wrote: >> Author: alfred >> Date: Sat Jul 27 20:47:01 2013 >> New Revision: 253719 >> URL: http://svnweb.freebsd.org/changeset/base/253719 >> >> Log: >> Fix watchdog pretimeout. >> >> The original API calls for pow2ns, however the new APIs from >> Linux call for seconds. >> >> We need to be able to convert to/from 2^Nns to seconds in both >> userland and kernel to fix this and properly compare units. >> >> Added: >> head/sys/libkern/flsll.c (contents, props changed) >> Modified: >> head/sys/conf/files >> head/sys/dev/watchdog/watchdog.c >> head/sys/sys/libkern.h >> head/usr.sbin/watchdogd/watchdogd.c >> >> Modified: head/sys/conf/files >> ============================================================================== >> --- head/sys/conf/files Sat Jul 27 20:15:18 2013 (r253718) >> +++ head/sys/conf/files Sat Jul 27 20:47:01 2013 (r253719) >> @@ -2976,6 +2976,7 @@ libkern/arc4random.c standard >> libkern/bcd.c standard >> libkern/bsearch.c standard >> libkern/crc32.c standard >> +libkern/flsll.c standard >> libkern/fnmatch.c standard >> libkern/iconv.c optional libiconv >> libkern/iconv_converter_if.m optional libiconv >> >> Modified: head/sys/dev/watchdog/watchdog.c >> ============================================================================== >> --- head/sys/dev/watchdog/watchdog.c Sat Jul 27 20:15:18 2013 (r253718) >> +++ head/sys/dev/watchdog/watchdog.c Sat Jul 27 20:47:01 2013 (r253719) >> @@ -39,6 +39,7 @@ __FBSDID("$FreeBSD$"); >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -60,10 +61,56 @@ static int wd_softtimeout_act = WD_SOFT_ >> >> static struct cdev *wd_dev; >> static volatile u_int wd_last_u; /* last timeout value set by kern_do_pat */ >> +static u_int wd_last_u_sysctl; /* last timeout value set by kern_do_pat */ >> +static u_int wd_last_u_sysctl_secs; /* wd_last_u in seconds */ >> + >> +SYSCTL_NODE(_hw, OID_AUTO, watchdog, CTLFLAG_RD, 0, "Main watchdog device"); >> +SYSCTL_UINT(_hw_watchdog, OID_AUTO, wd_last_u, CTLFLAG_RD, >> + &wd_last_u_sysctl, 0, "Watchdog last update time"); >> +SYSCTL_UINT(_hw_watchdog, OID_AUTO, wd_last_u_secs, CTLFLAG_RD, >> + &wd_last_u_sysctl_secs, 0, "Watchdog last update time"); >> >> static int wd_lastpat_valid = 0; >> static time_t wd_lastpat = 0; /* when the watchdog was last patted */ >> >> +static void >> +pow2ns_to_ts(int pow2ns, struct timespec *ts) >> +{ >> + uint64_t ns; >> + >> + ns = 1ULL << pow2ns; >> + ts->tv_sec = ns / 1000000000ULL; >> + ts->tv_nsec = ns % 1000000000ULL; >> +} >> + >> +static int >> +pow2ns_to_ticks(int pow2ns) >> +{ >> + struct timeval tv; >> + struct timespec ts; >> + >> + pow2ns_to_ts(pow2ns, &ts); >> + TIMESPEC_TO_TIMEVAL(&tv, &ts); >> + return (tvtohz(&tv)); >> +} >> + >> +static int >> +seconds_to_pow2ns(int seconds) >> +{ >> + uint64_t power; >> + uint64_t ns; >> + uint64_t shifted; >> + >> + ns = ((uint64_t)seconds) * 1000000000ULL; >> + power = flsll(ns); >> + shifted = 1ULL << power; >> + if (shifted <= ns) { >> + power++; >> + } >> + return (power); >> +} >> + >> + >> int >> wdog_kern_pat(u_int utim) >> { >> @@ -86,6 +133,8 @@ wdog_kern_pat(u_int utim) >> * This can be zero (to disable the watchdog) >> */ >> wd_last_u = (utim & WD_INTERVAL); >> + wd_last_u_sysctl = wd_last_u; >> + wd_last_u_sysctl_secs = pow2ns_to_ticks(wd_last_u) / hz; >> } >> if ((utim & WD_INTERVAL) == WD_TO_NEVER) { >> utim = 0; >> @@ -101,7 +150,7 @@ wdog_kern_pat(u_int utim) >> callout_stop(&wd_softtimeo_handle); >> } else { >> (void) callout_reset(&wd_softtimeo_handle, >> - hz*utim, wd_timeout_cb, "soft"); >> + pow2ns_to_ticks(utim), wd_timeout_cb, "soft"); >> } >> error = 0; >> } else { >> @@ -201,10 +250,13 @@ static int >> wd_set_pretimeout(int newtimeout, int disableiftoolong) >> { >> u_int utime; >> + struct timespec utime_ts; >> + int timeout_ticks; >> >> utime = wdog_kern_last_timeout(); >> + pow2ns_to_ts(utime, &utime_ts); >> /* do not permit a pre-timeout >= than the timeout. */ >> - if (newtimeout >= utime) { >> + if (newtimeout >= utime_ts.tv_sec) { >> /* >> * If 'disableiftoolong' then just fall through >> * so as to disable the pre-watchdog >> @@ -222,8 +274,22 @@ wd_set_pretimeout(int newtimeout, int di >> return 0; >> } >> >> + timeout_ticks = pow2ns_to_ticks(utime) - (hz*newtimeout); >> +#if 0 >> + printf("wd_set_pretimeout: " >> + "newtimeout: %d, " >> + "utime: %d -> utime_ticks: %d, " >> + "hz*newtimeout: %d, " >> + "timeout_ticks: %d -> sec: %d\n", >> + newtimeout, >> + utime, pow2ns_to_ticks(utime), >> + hz*newtimeout, >> + timeout_ticks, timeout_ticks / hz); >> +#endif >> + >> /* We determined the value is sane, so reset the callout */ >> - (void) callout_reset(&wd_pretimeo_handle, hz*(utime - newtimeout), >> + (void) callout_reset(&wd_pretimeo_handle, >> + timeout_ticks, >> wd_timeout_cb, "pre-timeout"); >> wd_pretimeout = newtimeout; >> return 0; >> @@ -282,7 +348,7 @@ wd_ioctl(struct cdev *dev __unused, u_lo >> break; >> case WDIOC_SETTIMEOUT: >> u = *(u_int *)data; >> - error = wdog_kern_pat(u); >> + error = wdog_kern_pat(seconds_to_pow2ns(u)); >> break; >> case WDIOC_GETTIMEOUT: >> u = wdog_kern_last_timeout(); >> >> Added: head/sys/libkern/flsll.c >> ============================================================================== >> --- /dev/null 00:00:00 1970 (empty, because file is newly added) >> +++ head/sys/libkern/flsll.c Sat Jul 27 20:47:01 2013 (r253719) >> @@ -0,0 +1,47 @@ >> +/*- >> + * Copyright (c) 1990, 1993 >> + * The Regents of the University of California. All rights reserved. >> + * >> + * Redistribution and use in source and binary forms, with or without >> + * modification, are permitted provided that the following conditions >> + * are met: >> + * 1. Redistributions of source code must retain the above copyright >> + * notice, this list of conditions and the following disclaimer. >> + * 2. Redistributions in binary form must reproduce the above copyright >> + * notice, this list of conditions and the following disclaimer in the >> + * documentation and/or other materials provided with the distribution. >> + * 4. Neither the name of the University nor the names of its contributors >> + * may be used to endorse or promote products derived from this software >> + * without specific prior written permission. >> + * >> + * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND >> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE >> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE >> + * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE >> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL >> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS >> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) >> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT >> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY >> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF >> + * SUCH DAMAGE. >> + */ >> + >> +#include >> +#include >> +__FBSDID("$FreeBSD$"); >> + >> +/* >> + * Find Last Set bit >> + */ >> +int >> +flsll(long long mask) >> +{ >> + int bit; >> + >> + if (mask == 0) >> + return (0); >> + for (bit = 1; mask != 1; bit++) >> + mask = (unsigned long long)mask >> 1; >> + return (bit); >> +} >> >> Modified: head/sys/sys/libkern.h >> ============================================================================== >> --- head/sys/sys/libkern.h Sat Jul 27 20:15:18 2013 (r253718) >> +++ head/sys/sys/libkern.h Sat Jul 27 20:47:01 2013 (r253719) >> @@ -94,6 +94,10 @@ int fls(int); >> #ifndef HAVE_INLINE_FLSL >> int flsl(long); >> #endif >> +#ifndef HAVE_INLINE_FLSLL >> +int flsll(long long); >> +#endif >> + >> int fnmatch(const char *, const char *, int); >> int locc(int, char *, u_int); >> void *memchr(const void *s, int c, size_t n); >> >> Modified: head/usr.sbin/watchdogd/watchdogd.c >> ============================================================================== >> --- head/usr.sbin/watchdogd/watchdogd.c Sat Jul 27 20:15:18 2013 (r253718) >> +++ head/usr.sbin/watchdogd/watchdogd.c Sat Jul 27 20:47:01 2013 (r253719) >> @@ -39,6 +39,7 @@ __FBSDID("$FreeBSD$"); >> #include >> #include >> #include >> +#include >> #include >> >> #include >> @@ -58,19 +59,24 @@ __FBSDID("$FreeBSD$"); >> >> #include >> >> +static long fetchtimeout(int opt, const char *longopt, const char *myoptarg); >> static void parseargs(int, char *[]); >> +static int seconds_to_pow2ns(int); >> static void sighandler(int); >> static void watchdog_loop(void); >> static int watchdog_init(void); >> static int watchdog_onoff(int onoff); >> static int watchdog_patpat(u_int timeout); >> static void usage(void); >> +static int tstotv(struct timeval *tv, struct timespec *ts); >> +static int tvtohz(struct timeval *tv); >> >> static int debugging = 0; >> static int end_program = 0; >> static const char *pidfile = _PATH_VARRUN "watchdogd.pid"; >> static u_int timeout = WD_TO_128SEC; >> static u_int pretimeout = 0; >> +static u_int timeout_sec; >> static u_int passive = 0; >> static int is_daemon = 0; >> static int is_dry_run = 0; /* do not arm the watchdog, only >> @@ -183,6 +189,59 @@ main(int argc, char *argv[]) >> } >> } >> >> +static void >> +pow2ns_to_ts(int pow2ns, struct timespec *ts) >> +{ >> + uint64_t ns; >> + >> + ns = 1ULL << pow2ns; >> + ts->tv_sec = ns / 1000000000ULL; >> + ts->tv_nsec = ns % 1000000000ULL; >> +} >> + >> +/* >> + * Convert a timeout in seconds to N where 2^N nanoseconds is close to >> + * "seconds". >> + * >> + * The kernel expects the timeouts for watchdogs in "2^N nanosecond format". >> + */ >> +static u_int >> +parse_timeout_to_pow2ns(char opt, const char *longopt, const char *myoptarg) >> +{ >> + double a; >> + u_int rv; >> + struct timespec ts; >> + struct timeval tv; >> + int ticks; >> + char shortopt[] = "- "; >> + >> + if (!longopt) >> + shortopt[1] = opt; >> + >> + a = fetchtimeout(opt, longopt, myoptarg); >> + >> + if (a == 0) >> + rv = WD_TO_NEVER; >> + else >> + rv = seconds_to_pow2ns(a); >> + pow2ns_to_ts(rv, &ts); >> + tstotv(&tv, &ts); >> + ticks = tvtohz(&tv); >> + if (debugging) { >> + printf("Timeout for %s%s " >> + "is 2^%d nanoseconds " >> + "(in: %s sec -> out: %ld sec %ld ns -> %d ticks)\n", >> + longopt ? "-" : "", longopt ? longopt : shortopt, >> + rv, >> + myoptarg, ts.tv_sec, ts.tv_nsec, ticks); >> + } >> + if (ticks <= 0) { >> + errx(1, "Timeout for %s%s is too small, please choose a higher timeout.", longopt ? "-" : "", longopt ? longopt : shortopt); >> + } >> + >> + return (rv); >> +} >> + >> /* >> * Catch signals and begin shutdown process. >> */ >> @@ -513,6 +572,110 @@ timeout_act_str2int(const char *lopt, co >> return rv; >> } >> >> +int >> +tstotv(struct timeval *tv, struct timespec *ts) >> +{ >> + >> + tv->tv_sec = ts->tv_sec; >> + tv->tv_usec = ts->tv_nsec / 1000; >> + return 0; >> +} >> + >> +/* >> + * Convert a timeval to a number of ticks. >> + * Mostly copied from the kernel. >> + */ >> +int >> +tvtohz(struct timeval *tv) >> +{ >> + register unsigned long ticks; >> + register long sec, usec; >> + int hz; >> + size_t hzsize; >> + int error; >> + int tick; >> + >> + hzsize = sizeof(hz); >> + >> + error = sysctlbyname("kern.hz", &hz, &hzsize, NULL, 0); >> + if (error) >> + err(1, "sysctlbyname kern.hz"); >> + >> + tick = 1000000 / hz; >> + >> + /* >> + * If the number of usecs in the whole seconds part of the time >> + * difference fits in a long, then the total number of usecs will >> + * fit in an unsigned long. Compute the total and convert it to >> + * ticks, rounding up and adding 1 to allow for the current tick >> + * to expire. Rounding also depends on unsigned long arithmetic >> + * to avoid overflow. >> + * >> + * Otherwise, if the number of ticks in the whole seconds part of >> + * the time difference fits in a long, then convert the parts to >> + * ticks separately and add, using similar rounding methods and >> + * overflow avoidance. This method would work in the previous >> + * case but it is slightly slower and assumes that hz is integral. >> + * >> + * Otherwise, round the time difference down to the maximum >> + * representable value. >> + * >> + * If ints have 32 bits, then the maximum value for any timeout in >> + * 10ms ticks is 248 days. >> + */ >> + sec = tv->tv_sec; >> + usec = tv->tv_usec; >> + if (usec < 0) { >> + sec--; >> + usec += 1000000; >> + } >> + if (sec < 0) { >> +#ifdef DIAGNOSTIC >> + if (usec > 0) { >> + sec++; >> + usec -= 1000000; >> + } >> + printf("tvotohz: negative time difference %ld sec %ld usec\n", >> + sec, usec); >> +#endif >> + ticks = 1; >> + } else if (sec <= LONG_MAX / 1000000) >> + ticks = (sec * 1000000 + (unsigned long)usec + (tick - 1)) >> + / tick + 1; >> + else if (sec <= LONG_MAX / hz) >> + ticks = sec * hz >> + + ((unsigned long)usec + (tick - 1)) / tick + 1; >> + else >> + ticks = LONG_MAX; >> + if (ticks > INT_MAX) >> + ticks = INT_MAX; >> + return ((int)ticks); >> +} >> + >> +static int >> +seconds_to_pow2ns(int seconds) >> +{ >> + uint64_t power; >> + uint64_t ns; >> + uint64_t shifted; >> + >> + if (seconds <= 0) >> + errx(1, "seconds %d < 0", seconds); >> + ns = ((uint64_t)seconds) * 1000000000ULL; >> + power = flsll(ns); >> + shifted = 1ULL << power; >> + if (shifted <= ns) { >> + power++; >> + } >> + if (debugging) { >> + printf("shifted %lld\n", (long long)shifted); >> + printf("seconds_to_pow2ns: seconds: %d, ns %lld, power %d\n", >> + seconds, (long long)ns, (int)power); >> + } >> + return (power); >> +} >> + >> + >> /* >> * Handle the few command line arguments supported. >> */ >> @@ -521,9 +684,7 @@ parseargs(int argc, char *argv[]) >> { >> int longindex; >> int c; >> - char *p; >> const char *lopt; >> - double a; >> >> /* >> * if we end with a 'd' aka 'watchdogd' then we are the daemon program, >> @@ -565,21 +726,11 @@ parseargs(int argc, char *argv[]) >> do_syslog = 0; >> break; >> case 't': >> - p = NULL; >> - errno = 0; >> - a = strtod(optarg, &p); >> - if ((p != NULL && *p != '\0') || errno != 0) >> - errx(EX_USAGE, "-t argument is not a number"); >> - if (a < 0) >> - errx(EX_USAGE, "-t argument must be positive"); >> - >> - if (a == 0) >> - timeout = WD_TO_NEVER; >> - else >> - timeout = flsll(a * 1e9); >> - if (debugging) >> - printf("Timeout is 2^%d nanoseconds\n", >> - timeout); >> + timeout_sec = atoi(optarg); >> + timeout = parse_timeout_to_pow2ns(c, NULL, optarg); >> + if (debugging) >> + printf("Timeout is 2^%d nanoseconds\n", >> + timeout); >> break; >> case 'T': >> carp_thresh_seconds = fetchtimeout(c, "NULL", optarg); >> @@ -618,4 +769,15 @@ parseargs(int argc, char *argv[]) >> errx(EX_USAGE, "extra arguments."); >> if (is_daemon && timeout < WD_TO_1SEC) >> errx(EX_USAGE, "-t argument is less than one second."); >> + if (pretimeout_set) { >> + struct timespec ts; >> + >> + pow2ns_to_ts(timeout, &ts); >> + if (pretimeout >= ts.tv_sec) { >> + errx(EX_USAGE, >> + "pretimeout (%d) >= timeout (%d -> %ld)\n" >> + "see manual section TIMEOUT RESOLUTION", >> + pretimeout, timeout_sec, (long)ts.tv_sec); >> + } >> + } > Come on. It has been 3 days that tinderbox complains about this. > > ===> usr.sbin/watchdogd (all) > cc -O2 -pipe -std=gnu99 -Qunused-arguments -fstack-protector > -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter > -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wreturn-type > -Wcast-qual -Wwrite-strings -Wswitch -Wshadow -Wunused-parameter > -Wcast-align -Wchar-subscripts -Winline -Wnested-externs > -Wredundant-decls -Wold-style-definition -Wmissing-variable-declarations > -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -c > /src/usr.sbin/watchdogd/watchdogd.c > /src/usr.sbin/watchdogd/watchdogd.c:777:18: error: comparison of > integers of different signs: 'u_int' (aka 'unsigned int') and 'time_t' > (aka 'int') [-Werror,-Wsign-compare] > if (pretimeout >= ts.tv_sec) { > ~~~~~~~~~~ ^ ~~~~~~~~~ > > Glen > Really sorry about this, it seemed that this compiled just fine when I was testing it. Now I'm thinking that maybe only the kernel part wound up compile tested with WARNS set. Then $work went nuts. I will try to update my email filters to catch this quicker next time. -Alfred