Date: Tue, 30 Jul 2013 07:22:10 -0400 From: Glen Barber <gjb@FreeBSD.org> To: Alfred Perlstein <alfred@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r253719 - in head: sys/conf sys/dev/watchdog sys/libkern sys/sys usr.sbin/watchdogd Message-ID: <20130730112210.GE2571@glenbarber.us> In-Reply-To: <201307272047.r6RKl2vI023259@svn.freebsd.org> References: <201307272047.r6RKl2vI023259@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--phCU5ROyZO6kBE05 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 >=20 > Log: > Fix watchdog pretimeout. > =20 > The original API calls for pow2ns, however the new APIs from > Linux call for seconds. > =20 > 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. >=20 > 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 >=20 > Modified: head/sys/conf/files > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- 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 >=20 > Modified: head/sys/dev/watchdog/watchdog.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- 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 <sys/kernel.h> > #include <sys/malloc.h> > #include <sys/module.h> > +#include <sys/sysctl.h> > #include <sys/syslog.h> > #include <sys/watchdog.h> > #include <sys/bus.h> > @@ -60,10 +61,56 @@ static int wd_softtimeout_act =3D WD_SOFT_ > =20 > 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_p= at */ > +static u_int wd_last_u_sysctl_secs; /* wd_last_u in seconds */ > + > +SYSCTL_NODE(_hw, OID_AUTO, watchdog, CTLFLAG_RD, 0, "Main watchdog devic= e"); > +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"); > =20 > static int wd_lastpat_valid =3D 0; > static time_t wd_lastpat =3D 0; /* when the watchdog was last patted */ > =20 > +static void > +pow2ns_to_ts(int pow2ns, struct timespec *ts) > +{ > + uint64_t ns; > + > + ns =3D 1ULL << pow2ns; > + ts->tv_sec =3D ns / 1000000000ULL; > + ts->tv_nsec =3D 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 =3D ((uint64_t)seconds) * 1000000000ULL; > + power =3D flsll(ns); > + shifted =3D 1ULL << power; > + if (shifted <=3D 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 =3D (utim & WD_INTERVAL); > + wd_last_u_sysctl =3D wd_last_u; > + wd_last_u_sysctl_secs =3D pow2ns_to_ticks(wd_last_u) / hz; > } > if ((utim & WD_INTERVAL) =3D=3D WD_TO_NEVER) { > utim =3D 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 =3D 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; > =20 > utime =3D wdog_kern_last_timeout(); > + pow2ns_to_ts(utime, &utime_ts); > /* do not permit a pre-timeout >=3D than the timeout. */ > - if (newtimeout >=3D utime) { > + if (newtimeout >=3D 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; > } > =20 > + timeout_ticks =3D 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 =3D newtimeout; > return 0; > @@ -282,7 +348,7 @@ wd_ioctl(struct cdev *dev __unused, u_lo > break; > case WDIOC_SETTIMEOUT: > u =3D *(u_int *)data; > - error =3D wdog_kern_pat(u); > + error =3D wdog_kern_pat(seconds_to_pow2ns(u)); > break; > case WDIOC_GETTIMEOUT: > u =3D wdog_kern_last_timeout(); >=20 > Added: head/sys/libkern/flsll.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- /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 distributio= n. > + * 4. Neither the name of the University nor the names of its contributo= rs > + * may be used to endorse or promote products derived from this softw= are > + * without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' A= ND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PU= RPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIA= BLE > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUE= NTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOO= DS > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, S= TRICT > + * 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 <sys/cdefs.h> > +#include <sys/libkern.h> > +__FBSDID("$FreeBSD$"); > + > +/* > + * Find Last Set bit > + */ > +int > +flsll(long long mask) > +{ > + int bit; > + > + if (mask =3D=3D 0) > + return (0); > + for (bit =3D 1; mask !=3D 1; bit++) > + mask =3D (unsigned long long)mask >> 1; > + return (bit); > +} >=20 > Modified: head/sys/sys/libkern.h > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- 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); >=20 > Modified: head/usr.sbin/watchdogd/watchdogd.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- 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 <sys/rtprio.h> > #include <sys/stat.h> > #include <sys/time.h> > +#include <sys/sysctl.h> > #include <sys/watchdog.h> > =20 > #include <err.h> > @@ -58,19 +59,24 @@ __FBSDID("$FreeBSD$"); > =20 > #include <getopt.h> > =20 > +static long fetchtimeout(int opt, const char *longopt, const char *myopt= arg); > 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); > =20 > static int debugging =3D 0; > static int end_program =3D 0; > static const char *pidfile =3D _PATH_VARRUN "watchdogd.pid"; > static u_int timeout =3D WD_TO_128SEC; > static u_int pretimeout =3D 0; > +static u_int timeout_sec; > static u_int passive =3D 0; > static int is_daemon =3D 0; > static int is_dry_run =3D 0; /* do not arm the watchdog, only > @@ -183,6 +189,59 @@ main(int argc, char *argv[]) > } > } > =20 > +static void > +pow2ns_to_ts(int pow2ns, struct timespec *ts) > +{ > + uint64_t ns; > + > + ns =3D 1ULL << pow2ns; > + ts->tv_sec =3D ns / 1000000000ULL; > + ts->tv_nsec =3D 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 form= at". > + */ > +static u_int > +parse_timeout_to_pow2ns(char opt, const char *longopt, const char *myopt= arg) > +{ > + double a; > + u_int rv; > + struct timespec ts; > + struct timeval tv; > + int ticks; > + char shortopt[] =3D "- "; > + > + if (!longopt) > + shortopt[1] =3D opt; > + > + a =3D fetchtimeout(opt, longopt, myoptarg); > + > + if (a =3D=3D 0) > + rv =3D WD_TO_NEVER; > + else > + rv =3D seconds_to_pow2ns(a); > + pow2ns_to_ts(rv, &ts); > + tstotv(&tv, &ts); > + ticks =3D 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 <=3D 0) { > + errx(1, "Timeout for %s%s is too small, please choose a higher timeout= =2E", 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; > } > =20 > +int > +tstotv(struct timeval *tv, struct timespec *ts) > +{ > + > + tv->tv_sec =3D ts->tv_sec; > + tv->tv_usec =3D 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 =3D sizeof(hz); > + > + error =3D sysctlbyname("kern.hz", &hz, &hzsize, NULL, 0); > + if (error) > + err(1, "sysctlbyname kern.hz"); > + > + tick =3D 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 =3D tv->tv_sec; > + usec =3D tv->tv_usec; > + if (usec < 0) { > + sec--; > + usec +=3D 1000000; > + } > + if (sec < 0) { > +#ifdef DIAGNOSTIC > + if (usec > 0) { > + sec++; > + usec -=3D 1000000; > + } > + printf("tvotohz: negative time difference %ld sec %ld usec\n", > + sec, usec); > +#endif > + ticks =3D 1; > + } else if (sec <=3D LONG_MAX / 1000000) > + ticks =3D (sec * 1000000 + (unsigned long)usec + (tick - 1)) > + / tick + 1; > + else if (sec <=3D LONG_MAX / hz) > + ticks =3D sec * hz > + + ((unsigned long)usec + (tick - 1)) / tick + 1; > + else > + ticks =3D LONG_MAX; > + if (ticks > INT_MAX) > + ticks =3D INT_MAX; > + return ((int)ticks); > +} > + > +static int > +seconds_to_pow2ns(int seconds) > +{ > + uint64_t power; > + uint64_t ns; > + uint64_t shifted; > + > + if (seconds <=3D 0) > + errx(1, "seconds %d < 0", seconds); > + ns =3D ((uint64_t)seconds) * 1000000000ULL; > + power =3D flsll(ns); > + shifted =3D 1ULL << power; > + if (shifted <=3D 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; > =20 > /* > * 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 =3D 0; > break; > case 't': > - p =3D NULL; > - errno =3D 0; > - a =3D strtod(optarg, &p); > - if ((p !=3D NULL && *p !=3D '\0') || errno !=3D 0) > - errx(EX_USAGE, "-t argument is not a number"); > - if (a < 0) > - errx(EX_USAGE, "-t argument must be positive"); > - > - if (a =3D=3D 0) > - timeout =3D WD_TO_NEVER; > - else > - timeout =3D flsll(a * 1e9); > - if (debugging) > - printf("Timeout is 2^%d nanoseconds\n", > - timeout); > + timeout_sec =3D atoi(optarg); > + timeout =3D parse_timeout_to_pow2ns(c, NULL, optarg); > + if (debugging) > + printf("Timeout is 2^%d nanoseconds\n", > + timeout); > break; > case 'T': > carp_thresh_seconds =3D 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 >=3D ts.tv_sec) { > + errx(EX_USAGE, > + "pretimeout (%d) >=3D 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. =3D=3D=3D> usr.sbin/watchdogd (all) cc -O2 -pipe -std=3Dgnu99 -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 >=3D ts.tv_sec) { ~~~~~~~~~~ ^ ~~~~~~~~~ Glen --phCU5ROyZO6kBE05 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (FreeBSD) iQEcBAEBCAAGBQJR96HiAAoJEFJPDDeguUajazUH/1sy6pxuubcNOqtXlYKTsThQ OuEyILCaS7ddu+Tje33a54vnn4lloLaKdRKJiBiOFfOToIapJgZrZL3kWNWDCtDx UgHA7PTYBlw7WpoDEmRM4zqhkMu0vj0kkaTPuhleVug6zZAWdWtDUCMICEsnJBxV bLMtNnJ+ylIh187PwTiWTUWSRkYBm71sBcVmsVYCqeon1hBHmTgr9UfaTrqSGafo bltEys1bxUpzDztcMG1HS1FPf9RPjdLXtkIhlY95lhDigAbfqiBC5mYo0T7vGXJZ O//csLlwLAEIK2s3sBXyMtTDrQEGIHn2FcucoxmQgYC4tOX7KK6YNdBQQ+pdGB0= =5+1a -----END PGP SIGNATURE----- --phCU5ROyZO6kBE05--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130730112210.GE2571>