svn commit: r253719 - in head: sys/conf sys/dev/watchdog sys/libkern sys/sys usr.sbin/watchdogd

Alfred Perlstein bright at mu.org
Wed Jul 31 05:21:06 UTC 2013


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 <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 = 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 <sys/cdefs.h>
>> +#include <sys/libkern.h>
>> +__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 <sys/rtprio.h>
>>   #include <sys/stat.h>
>>   #include <sys/time.h>
>> +#include <sys/sysctl.h>
>>   #include <sys/watchdog.h>
>>   
>>   #include <err.h>
>> @@ -58,19 +59,24 @@ __FBSDID("$FreeBSD$");
>>   
>>   #include <getopt.h>
>>   
>> +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


More information about the svn-src-all mailing list