powerd
Nate Lawson
nate at root.org
Fri Dec 9 04:48:57 PST 2005
Dag-Erling Smørgrav wrote:
> Nate Lawson <nate at root.org> 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 <bsd.prog.mk>
> 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 <sys/cdefs.h>
> __FBSDID("$FreeBSD: src/usr.sbin/powerd/powerd.c,v 1.16 2005/10/24 18:34:54 njl Exp $");
>
> -#include <sys/types.h>
> #include <sys/param.h>
> #include <sys/ioctl.h>
> #include <sys/sysctl.h>
> #include <sys/resource.h>
> #include <sys/socket.h>
> +#include <sys/time.h>
> #include <sys/un.h>
>
> #include <err.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <libutil.h>
> -#include <pthread.h>
> #include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> @@ -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
More information about the freebsd-current
mailing list