From owner-freebsd-current@FreeBSD.ORG Fri Dec 9 12:48:57 2005 Return-Path: X-Original-To: current@freebsd.org Delivered-To: freebsd-current@FreeBSD.ORG Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 4A07E16A41F; Fri, 9 Dec 2005 12:48:57 +0000 (GMT) (envelope-from nate@root.org) Received: from www.cryptography.com (li-22.members.linode.com [64.5.53.22]) by mx1.FreeBSD.org (Postfix) with ESMTP id A6B9F43D96; Fri, 9 Dec 2005 12:48:05 +0000 (GMT) (envelope-from nate@root.org) Received: from [192.168.1.66] (p3248-ipad30akatuka.ibaraki.ocn.ne.jp [60.33.34.248]) by www.cryptography.com (8.12.8/8.12.8) with ESMTP id jB9Clsdu010363 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Fri, 9 Dec 2005 04:47:58 -0800 Message-ID: <43997CEC.1070607@root.org> Date: Fri, 09 Dec 2005 21:47:40 +0900 From: Nate Lawson User-Agent: Mozilla Thunderbird 1.0.6 (Windows/20050716) X-Accept-Language: en-us, en MIME-Version: 1.0 To: =?ISO-8859-1?Q?Dag-Erling_Sm=F8rgrav?= References: <43938F61.1050202@terranova.net> <4393F60E.2040106@shapeshifter.se> <86mzjflc97.fsf@xps.des.no> <439495B1.5060305@shapeshifter.se> <861x0qmuen.fsf@xps.des.no> <43956ADF.4050504@shapeshifter.se> <86slt6lb9s.fsf@xps.des.no> <4395A265.8080006@root.org> <86d5k9eric.fsf@xps.des.no> In-Reply-To: <86d5k9eric.fsf@xps.des.no> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Cc: njl@freebsd.org, Fredrik Lindberg , Travis Mikalson , current@freebsd.org Subject: Re: powerd X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Dec 2005 12:48:57 -0000 Dag-Erling Smørgrav wrote: > Nate Lawson writes: > >>I'd prefer to move forward, not backward. When using AC modes, it is >>an advantage to be devd-driven. The current implementation is not >>right, I agree, but there shouldn't be any actual problem other than >>suboptimal performance. Changing the thread to be a select() seems >>good. I welcome any patches. > > > You're welcome. > > powerd is a mess, BTW. I've tried to fix the most blatant mistakes > (poor understanding of signal handling), but it basically needs a > rewrite. My comments below. Overall I like this approach and just have a few questions. > ------------------------------------------------------------------------ > > Index: usr.sbin/powerd/Makefile > =================================================================== > RCS file: /home/ncvs/src/usr.sbin/powerd/Makefile,v > retrieving revision 1.4 > diff -u -r1.4 Makefile > --- usr.sbin/powerd/Makefile 19 Oct 2005 04:48:44 -0000 1.4 > +++ usr.sbin/powerd/Makefile 6 Dec 2005 23:20:44 -0000 > @@ -3,9 +3,12 @@ > PROG= powerd > MAN= powerd.8 > WARNS?= 6 > -LDFLAGS+= -lpthread > > DPADD= ${LIBUTIL} > LDADD= -lutil > > +.if ${MACHINE_ARCH} == "i386" > +CFLAGS+=-DUSE_APM > +.endif > + > .include > Index: usr.sbin/powerd/powerd.c > =================================================================== > RCS file: /home/ncvs/src/usr.sbin/powerd/powerd.c,v > retrieving revision 1.16 > diff -u -r1.16 powerd.c > --- usr.sbin/powerd/powerd.c 24 Oct 2005 18:34:54 -0000 1.16 > +++ usr.sbin/powerd/powerd.c 6 Dec 2005 23:23:11 -0000 > @@ -28,19 +28,18 @@ > #include > __FBSDID("$FreeBSD: src/usr.sbin/powerd/powerd.c,v 1.16 2005/10/24 18:34:54 njl Exp $"); > > -#include > #include > #include > #include > #include > #include > +#include > #include > > #include > #include > #include > #include > -#include > #include > #include > #include > @@ -55,17 +54,17 @@ > #define DEFAULT_IDLE_PERCENT 90 > #define DEFAULT_POLL_INTERVAL 500 /* Poll interval in milliseconds */ > > -enum modes_t { > +typedef enum { > MODE_MIN, > MODE_ADAPTIVE, > MODE_MAX, > -}; > +} modes_t; > > -enum power_src_t { > +typedef enum { > SRC_AC, > SRC_BATTERY, > SRC_UNKNOWN, > -}; > +} power_src_t; > > const char *modes[] = { > "AC", > @@ -82,10 +81,9 @@ > static int read_freqs(int *numfreqs, int **freqs, int **power); > static int set_freq(int freq); > static void acline_init(void); > -static int acline_read(void); > +static void acline_read(void); > static int devd_init(void); > static void devd_close(void); > -static void *devd_read(void *arg); > static void handle_sigs(int sig); > static void parse_mode(char *arg, int *mode, int ch); > static void usage(void); > @@ -96,19 +94,29 @@ > static int levels_mib[4]; > static int acline_mib[3]; > > -/* devd-cached value provided by our thread. */ > -static int devd_acline; > - > /* Configuration */ > static int cpu_running_mark; > static int cpu_idle_mark; > static int poll_ival; > static int vflag; > > -static int apm_fd; > -static int devd_pipe; > -static pthread_t devd_thread; > -static int exit_requested; > +static volatile sig_atomic_t exit_requested; > +static power_src_t acline_status; > +static enum { > + ac_none, > + ac_acpi_sysctl, > + ac_acpi_devd, > +#ifdef USE_APM > + ac_apm, > +#endif > +} acline_mode; Prefer enums be all CAPS, seems like ac_apm can be left in the enum without an #ifdef because it's only checked by code in an #ifdef below. > +#ifdef USE_APM > +static int apm_fd = -1; > +#endif > +static int devd_pipe = -1; > + > +#define DEVD_RETRY_INTERVAL 60 /* seconds */ > +static struct timeval tried_devd; > > static int > read_usage_times(long *idle, long *total) > @@ -195,168 +203,132 @@ > > /* > * Try to use ACPI to find the AC line status. If this fails, fall back > - * to APM. If nothing succeeds, we'll just run in default mode. If we are > - * using ACPI, try opening a pipe to devd to detect AC line events. > + * to APM. If nothing succeeds, we'll just run in default mode. > */ > static void > acline_init() > { > - int acline; > size_t len; > > - apm_fd = -1; > - devd_pipe = -1; > - len = sizeof(acline); > - if (sysctlbyname(ACPIAC, &acline, &len, NULL, 0) == 0) { > - len = 3; > - if (sysctlnametomib(ACPIAC, acline_mib, &len)) > - err(1, "lookup acline"); > - > - /* Read line status once so that we have an initial value. */ > - devd_acline = acline_read(); > - > - /* > - * Try connecting to the devd pipe and start a read thread > - * if we succeed. > - */ > - if ((devd_pipe = devd_init()) >= 0) { > - if (pthread_create(&devd_thread, NULL, devd_read, > - &devd_pipe)) > - err(1, "pthread_create devd thread"); > - } else if (vflag) { > - warnx( > - "unable to connect to devd pipe, using polling mode instead"); > - } > + len = 3; > + if (sysctlnametomib(ACPIAC, acline_mib, &len) == 0) { > + acline_mode = ac_acpi_sysctl; > + if (vflag) > + warnx("using sysctl for AC line status"); > +#ifdef __i386__ > + } else if ((apm_fd = open(APMDEV, O_RDONLY)) >= 0) { > + if (vflag) > + warnx("using APM for AC line status"); > + acline_mode = ac_apm; > +#endif Don't you want to use your new USE_APM define here? > } else { > - apm_fd = open(APMDEV, O_RDONLY); > - if (apm_fd == -1) > - warnx( > - "cannot read AC line status, using default settings"); > + warnx("unable to determine AC line status"); > + acline_mode = ac_none; > } > } > > -static int > -acline_read() > +static void > +acline_read(void) Is this correct? I thought only the prototype (above) should have void as an arg. > { > - int acline; > - size_t len; > -#ifdef __i386__ > - struct apm_info info; > -#endif > - > - acline = SRC_UNKNOWN; > - len = sizeof(acline); > + if (acline_mode == ac_acpi_devd) { > + char buf[DEVCTL_MAXBUF], *ptr; > + ssize_t rlen; > + int notify; Prefer variables declared at start of function. > - /* > - * Get state from our devd thread, the ACPI sysctl, or APM. We > - * prefer sources in this order. > - */ > - if (devd_pipe >= 0) > - acline = devd_acline; > - else if (sysctl(acline_mib, 3, &acline, &len, NULL, 0) == 0) > - acline = acline ? SRC_AC : SRC_BATTERY; > -#ifdef __i386__ > - else if (apm_fd != -1 && ioctl(apm_fd, APMIO_GETINFO, &info) == 0) > - acline = info.ai_acline ? SRC_AC : SRC_BATTERY; > + rlen = read(devd_pipe, buf, sizeof(buf)); > + if (rlen == 0 || (rlen < 0 && errno != EWOULDBLOCK)) { > + if (vflag) > + warnx("lost devd connection, switching to sysctl"); > + devd_close(); > + acline_mode = ac_acpi_sysctl; > + /* FALLTHROUGH */ > + } > + if (rlen > 0 && > + (ptr = strstr(buf, "system=ACPI")) != NULL && > + (ptr = strstr(ptr, "subsystem=ACAD")) != NULL && > + (ptr = strstr(ptr, "notify=")) != NULL && > + sscanf(ptr, "notify=%x", ¬ify) == 1) > + acline_status = (notify ? SRC_AC : SRC_BATTERY); > + } > + if (acline_mode == ac_acpi_sysctl) { > + int acline; > + size_t len; Prefer vars at start of function. > + len = sizeof(acline); > + if (sysctl(acline_mib, 3, &acline, &len, NULL, 0) == 0) > + acline_status = (acline ? SRC_AC : SRC_BATTERY); > + else > + acline_status = SRC_UNKNOWN; > + } > +#ifdef USE_APM > + if (acline_mode == ac_apm) { > + struct apm_info info; > + > + if (ioctl(apm_fd, APMIO_GETINFO, &info) == 0) { > + acline_status = (info.ai_acline ? SRC_AC : SRC_BATTERY); > + } else { > + close(apm_fd); > + apm_fd = -1; > + acline_mode = ac_none; > + acline_status = SRC_UNKNOWN; > + } > + } > #endif > - > - return (acline); > + /* try to (re)connect to devd */ > + if (acline_mode == ac_acpi_sysctl) { > + struct timeval now; > + > + gettimeofday(&now, NULL); > + if (now.tv_sec > tried_devd.tv_sec + DEVD_RETRY_INTERVAL) { > + if (devd_init() >= 0) { > + if (vflag) > + warnx("using devd for AC line status"); > + acline_mode = ac_acpi_devd; > + } > + tried_devd = now; > + } > + } > } > > static int > devd_init(void) > { > struct sockaddr_un devd_addr; > - int devd_sock; > > bzero(&devd_addr, sizeof(devd_addr)); > - if ((devd_sock = socket(PF_LOCAL, SOCK_STREAM, 0)) < 0) { > + if ((devd_pipe = socket(PF_LOCAL, SOCK_STREAM, 0)) < 0) { > if (vflag) > - warn("failed to create devd socket"); > + warn("%s(): socket()", __func__); > return (-1); > } > > devd_addr.sun_family = PF_LOCAL; > strlcpy(devd_addr.sun_path, DEVDPIPE, sizeof(devd_addr.sun_path)); > - if (connect(devd_sock, (struct sockaddr *)&devd_addr, > + if (connect(devd_pipe, (struct sockaddr *)&devd_addr, > sizeof(devd_addr)) == -1) { > - close(devd_sock); > + if (vflag) > + warn("%s(): connect()", __func__); > + close(devd_pipe); > + devd_pipe = -1; > return (-1); > } > > - return (devd_sock); > + if (fcntl(devd_pipe, F_SETFL, O_NONBLOCK) == -1) { > + if (vflag) > + warn("%s(): fcntl()", __func__); > + close(devd_pipe); > + return (-1); > + } > + > + return (devd_pipe); > } > > static void > devd_close(void) > { > > - if (devd_pipe < 0) > - return; > - > - pthread_kill(devd_thread, SIGTERM); > close(devd_pipe); > -} Seems like this function can go away now and we can just conditionally close devd_pipe (if != -1) at the end of main(). > - > -/* > - * This loop runs as a separate thread. It reads events from devd, but > - * spends most of its time blocked in select(2). > - */ > -static void * > -devd_read(void *arg) > -{ > - char buf[DEVCTL_MAXBUF], *ptr; > - fd_set fdset; > - int fd, notify, rlen; > - > - fd = *(int *)arg; > - notify = -1; > - FD_ZERO(&fdset); > - while (!exit_requested) { > - FD_SET(fd, &fdset); > - if (select(fd + 1, &fdset, NULL, NULL, NULL) < 0) > - break; > - if (!FD_ISSET(fd, &fdset)) > - continue; > - > - /* Read the notify string, devd NULL-terminates it. */ > - rlen = read(fd, buf, sizeof(buf)); > - if (rlen <= 0) { > - close(devd_pipe); > - devd_pipe = -1; > - if (vflag) > - warnx( > - "devd disappeared, downgrading to polling mode"); > - > - /* > - * Keep trying to reconnect to devd but sleep in > - * between to avoid wasting CPU cycles. > - */ > - while (!exit_requested && (fd = devd_init()) < 0) > - sleep(300); > - > - if (fd >= 0) { > - devd_pipe = fd; > - if (vflag) > - warnx( > - "devd came back, upgrading to event mode"); > - } > - continue; > - } > - > - /* Loosely match the notify string. */ > - if ((ptr = strstr(buf, "system=ACPI")) != NULL && > - (ptr = strstr(ptr, "subsystem=ACAD")) != NULL && > - (ptr = strstr(ptr, "notify=")) != NULL) { > - if (sscanf(ptr, "notify=%x", ¬ify) != 1) { > - warnx("bad devd notify string"); > - continue; > - } > - devd_acline = notify ? SRC_AC : SRC_BATTERY; > - } > - } > - > - return (NULL); > + devd_pipe = -1; > } > > static void > @@ -392,10 +364,13 @@ > int > main(int argc, char * argv[]) > { > + struct timeval timeout; > + fd_set fdset; > + int nfds; > struct pidfh *pfh = NULL; > const char *pidfile = NULL; > long idle, total; > - int acline, curfreq, *freqs, i, *mwatts, numfreqs; > + int curfreq, *freqs, i, *mwatts, numfreqs; > int ch, mode, mode_ac, mode_battery, mode_none; > uint64_t mjoules_used; > size_t len; > @@ -407,7 +382,6 @@ > poll_ival = DEFAULT_POLL_INTERVAL; > mjoules_used = 0; > vflag = 0; > - apm_fd = -1; > > /* User must be root to control frequencies. */ > if (geteuid() != 0) > @@ -479,15 +453,6 @@ > if (read_freqs(&numfreqs, &freqs, &mwatts)) > err(1, "error reading supported CPU frequencies"); > > - /* > - * Exit cleanly on signals; devd may send a SIGPIPE if it dies. We > - * do this before acline_init() since it may create a thread and we > - * want it to inherit our signal mask. > - */ > - signal(SIGINT, handle_sigs); > - signal(SIGTERM, handle_sigs); > - signal(SIGPIPE, SIG_IGN); > - Don't we still need SIGPIPE handling if reading from devd? Or does opening the fd nonblocking mean you're guaranteed not to get this signal? -- Nate