Re: [PATCH[ udevd race conditions and performance, assorted cleanups

[Kay Sievers <kay.sievers@vrfy.org>]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

On Fri, Mar 26, 2004 at 11:48:15PM -0500, Chris Friesen wrote:
> 
> This patch covers a number of areas:
> 
> 1) sysfs.h is fixed up to use the common dbg() macro.  This fixes the 
> case where DEBUG is defined but USE_LOG isn't.
> 
> 2) udevstart.c is modified to include the proper headers, rather than 
> getting them indirectly which can break depending on Makefile flags
> 
> 3) udevd.c gets some major changes:
> a) I added a pipe from the signal handler.  This fixes the race 
> conditions that I mentioned earlier.  Basically, the point of the pipe 
> is to force the select() call to return immediately if a signal handler 
> fired before we actually started the select() call.  This then lets us 
> run the appropriate code based on flags set in the signal handler proper.
> b) I added a number of flags to coalesce calls to common routines.  This 
> should make things slightly more efficient.
> c) since most calls will tend to come in with a sequence number larger 
> than what has been received, I switched msg_queue_insert() to scan the 
> msg_list backwards to improve performance.


Chris,
here is a patch on top of your nice improvements.
I fixed the whitespace and it hopefully fixes the stupid timestamp bug in
udevd. Some stupid OS sets the hwclock to localtime and linux changes it
to UTC while starting. If any events are pending they may be delayed by
the users time distance from UTC :) So we use the uptime seconds now.

thanks,
Kay

[-- Attachment #2: 01-udevd.patch --]
[-- Type: text/plain, Size: 8958 bytes --]

===== Makefile 1.148 vs edited =====
--- 1.148/Makefile	Sat Mar 27 02:23:20 2004
+++ edited/Makefile	Sun Mar 28 01:36:10 2004
@@ -220,6 +220,7 @@
 
 ifeq ($(strip $(USE_KLIBC)),true)
 	OBJS += klibc_fixups.o
+	KLIBC_FIXUP = klibc_fixups.o
 endif
 
 # header files automatically generated
@@ -266,8 +267,8 @@
 	$(LD) $(LDFLAGS) -o $@ $(CRT0) udevinfo.o udev_lib.o udev_config.o udevdb.o $(SYSFS) $(TDB) $(LIB_OBJS) $(ARCH_LIB_OBJS)
 	$(STRIPCMD) $@
 
-$(DAEMON): $(DAEMON).o udevd.h $(LIBC)
-	$(LD) $(LDFLAGS) -o $@ $(CRT0) udevd.o $(LIB_OBJS) $(ARCH_LIB_OBJS)
+$(DAEMON): $(DAEMON).o $(OBJS) udevd.h $(LIBC)
+	$(LD) $(LDFLAGS) -o $@ $(CRT0) udevd.o udev_lib.o $(KLIBC_FIXUP) $(LIB_OBJS) $(ARCH_LIB_OBJS)
 	$(STRIPCMD) $@
 
 $(SENDER): $(SENDER).o udevd.h $(LIBC)
===== klibc_fixups.c 1.8 vs edited =====
--- 1.8/klibc_fixups.c	Wed Mar 17 23:40:12 2004
+++ edited/klibc_fixups.c	Sun Mar 28 00:55:06 2004
@@ -30,12 +30,14 @@
 
 #include "udev.h"
 #include "klibc_fixups.h"
+#include "udev_lib.h"
 #include "logging.h"
 
 #define PW_FILE		"/etc/passwd"
 #define GR_FILE		"/etc/group"
 #define UTMP_FILE	"/var/run/utmp"
 
+_syscall1(int, sysinfo, struct sysinfo *, info);
 
 /* return the id of a passwd style line, selected by the users name */
 static unsigned long get_id_by_name(const char *uname, const char *dbfile)
===== klibc_fixups.h 1.6 vs edited =====
--- 1.6/klibc_fixups.h	Sun Feb 29 05:51:47 2004
+++ edited/klibc_fixups.h	Sun Mar 28 01:05:19 2004
@@ -3,6 +3,11 @@
 #ifndef KLIBC_FIXUPS_H
 #define KLIBC_FIXUPS_H
 
+#include <linux/kernel.h>
+#include <linux/unistd.h>
+
+int sysinfo(struct sysinfo *info);
+
 struct passwd {
 	char	*pw_name;	/* user name */
 	char	*pw_passwd;	/* user password */
===== udevd.c 1.26 vs edited =====
--- 1.26/udevd.c	Sat Mar 27 23:32:48 2004
+++ edited/udevd.c	Sun Mar 28 01:48:23 2004
@@ -28,12 +28,15 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <time.h>
+#include <sys/time.h>
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <sys/un.h>
-#include <sys/time.h>
 #include <fcntl.h>
+#include "klibc_fixups.h"
+#ifndef __KLIBC__
+#include <sys/sysinfo.h>
+#endif
 
 #include "list.h"
 #include "udev.h"
@@ -106,6 +109,7 @@
 static void msg_queue_insert(struct hotplug_msg *msg)
 {
 	struct hotplug_msg *loop_msg;
+	struct sysinfo info;
 
 	/* sort message by sequence number into list. events
 	 * will tend to come in order, so scan the list backwards
@@ -113,11 +117,13 @@
 	list_for_each_entry_reverse(loop_msg, &msg_list, list)
 		if (loop_msg->seqnum < msg->seqnum)
 			break;
-	list_add(&msg->list, &loop_msg->list);
-	dbg("queued message seq %d", msg->seqnum);
 
 	/* store timestamp of queuing */
-	msg->queue_time = time(NULL);
+	sysinfo(&info);
+	msg->queue_time = info.uptime;
+
+	list_add(&msg->list, &loop_msg->list);
+	dbg("queued message seq %d", msg->seqnum);
 
 	/* run msg queue manager */
 	run_msg_q = 1;
@@ -203,7 +209,8 @@
 {
 	struct hotplug_msg *loop_msg;
 	struct hotplug_msg *tmp_msg;
-	time_t msg_age = 0;
+	struct sysinfo info;
+	long msg_age = 0;
 
 	dbg("msg queue manager, next expected is %d", expected_seqnum);
 recheck:
@@ -215,7 +222,9 @@
 		}
 
 		/* move event with expired timeout to the exec list */
-		msg_age = time(NULL) - loop_msg->queue_time;
+		sysinfo(&info);
+		msg_age = info.uptime - loop_msg->queue_time;
+		dbg("seq %d is %li seconds old", loop_msg->seqnum, msg_age);
 		if (msg_age > EVENT_TIMEOUT_SEC-1) {
 			msg_move_exec(loop_msg);
 			goto recheck;
@@ -226,11 +235,10 @@
 
 	msg_dump_queue();
 
+	/* set timeout for remaining queued events */
 	if (list_empty(&msg_list) == 0) {
-		/* set timeout for remaining queued events */
 		struct itimerval itv = {{0, 0}, {EVENT_TIMEOUT_SEC - msg_age, 0}};
-		dbg("next event expires in %li seconds",
-		    EVENT_TIMEOUT_SEC - msg_age);
+		dbg("next event expires in %li seconds", EVENT_TIMEOUT_SEC - msg_age);
 		setitimer(ITIMER_REAL, &itv, 0);
 	}
 }
@@ -399,25 +407,25 @@
 		dbg("need to be root, exit");
 		exit(1);
 	}
-	
+
 	/* setup signal handler pipe */
-   retval = pipe(pipefds);
-   if (retval < 0) {
-      dbg("error getting pipes: %s", strerror(errno));
-      exit(1);
-   }
-	
-   retval = fcntl(pipefds[0], F_SETFL, O_NONBLOCK);
-   if (retval < 0) {
-      dbg("fcntl on read pipe: %s", strerror(errno));
-      exit(1);
-   }
-   
-   retval = fcntl(pipefds[1], F_SETFL, O_NONBLOCK);
-   if (retval < 0) {
-      dbg("fcntl on write pipe: %s", strerror(errno));
-      exit(1);
-   }
+	retval = pipe(pipefds);
+	if (retval < 0) {
+		dbg("error getting pipes: %s", strerror(errno));
+		exit(1);
+	}
+
+	retval = fcntl(pipefds[0], F_SETFL, O_NONBLOCK);
+		if (retval < 0) {
+		dbg("error fcntl on read pipe: %s", strerror(errno));
+		exit(1);
+	}
+
+	retval = fcntl(pipefds[1], F_SETFL, O_NONBLOCK);
+	if (retval < 0) {
+		dbg("error fcntl on write pipe: %s", strerror(errno));
+		exit(1);
+	}
 
 	/* set signal handlers */
 	act.sa_handler = sig_handler;
@@ -450,43 +458,42 @@
 	/* enable receiving of the sender credentials */
 	setsockopt(ssock, SOL_SOCKET, SO_PASSCRED, &on, sizeof(on));
 
-   FD_ZERO(&readfds);
-   FD_SET(ssock, &readfds);
-   FD_SET(pipefds[0], &readfds);
+	FD_ZERO(&readfds);
+	FD_SET(ssock, &readfds);
+	FD_SET(pipefds[0], &readfds);
 	maxsockplus = ssock+1;
 	while (1) {
 		fd_set workreadfds = readfds;
 		retval = select(maxsockplus, &workreadfds, NULL, NULL, NULL);
-		
+
 		if (retval < 0) {
-			dbg("error in select: %s", strerror(errno));
+			if (errno != EINTR)
+				dbg("error in select: %s", strerror(errno));
 			continue;
 		}
-		
+
 		if (FD_ISSET(ssock, &workreadfds))
 			handle_msg(ssock);
-		
+
 		if (FD_ISSET(pipefds[0], &workreadfds))
 			user_sighandler();
-		
+
 		if (children_waiting) {
 			children_waiting = 0;
 			reap_kids();
 		}
-		
+
 		if (run_msg_q) {
 			run_msg_q = 0;
 			msg_queue_manager();
 		}
-		
+
 		if (run_exec_q) {
-			
 			/* this is tricky.  exec_queue_manager() loops over exec_list, and
 			 * calls running_with_devpath(), which loops over running_list. This gives
 			 * O(N*M), which can get *nasty*.  Clean up running_list before
 			 * calling exec_queue_manager().
 			 */
-			
 			if (children_waiting) {
 				children_waiting = 0;
 				reap_kids();
===== udevd.h 1.8 vs edited =====
--- 1.8/udevd.h	Thu Feb 26 04:09:18 2004
+++ edited/udevd.h	Sat Mar 27 23:40:46 2004
@@ -34,7 +34,7 @@
 	struct list_head list;
 	pid_t pid;
 	int seqnum;
-	time_t queue_time;
+	long queue_time;
 	char action[ACTION_SIZE];
 	char devpath[DEVPATH_SIZE];
 	char subsystem[SUBSYSTEM_SIZE];
===== udevsend.c 1.28 vs edited =====
--- 1.28/udevsend.c	Wed Mar 17 23:40:12 2004
+++ edited/udevsend.c	Sun Mar 28 00:08:58 2004
@@ -26,13 +26,13 @@
 #include <sys/socket.h>
 #include <sys/wait.h>
 #include <sys/un.h>
+#include <time.h>
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stddef.h>
 #include <string.h>
 #include <unistd.h>
-#include <time.h>
 #include <linux/stddef.h>
 
 #include "udev.h"
@@ -53,16 +53,15 @@
 }
 #endif
 
-static int build_hotplugmsg(struct hotplug_msg *msg, char *action,
-			    char *devpath, char *subsystem, int seqnum)
+static void build_hotplugmsg(struct hotplug_msg *msg, char *action,
+			     char *devpath, char *subsystem, int seqnum)
 {
-	memset(msg, 0x00, sizeof(*msg));
+	memset(msg, 0x00, sizeof(struct hotplug_msg));
 	strfieldcpy(msg->magic, UDEV_MAGIC);
 	msg->seqnum = seqnum;
 	strfieldcpy(msg->action, action);
 	strfieldcpy(msg->devpath, devpath);
 	strfieldcpy(msg->subsystem, subsystem);
-	return sizeof(struct hotplug_msg);
 }
 
 static int start_daemon(void)
@@ -108,7 +107,6 @@
 	char *seqnum;
 	int seq;
 	int retval = 1;
-	int size;
 	int loop;
 	struct timespec tspec;
 	int sock;
@@ -155,18 +153,19 @@
 		goto exit;
 	}
 
-	memset(&saddr, 0x00, sizeof(saddr));
+	memset(&saddr, 0x00, sizeof(struct sockaddr_un));
 	saddr.sun_family = AF_LOCAL;
 	/* use abstract namespace for socket path */
 	strcpy(&saddr.sun_path[1], UDEVD_SOCK_PATH);
 	addrlen = offsetof(struct sockaddr_un, sun_path) + strlen(saddr.sun_path+1) + 1;
 
-	size = build_hotplugmsg(&msg, action, devpath, subsystem, seq);
+	build_hotplugmsg(&msg, action, devpath, subsystem, seq);
 
 	/* If we can't send, try to start daemon and resend message */
 	loop = UDEVSEND_CONNECT_RETRY;
 	while (loop--) {
-		retval = sendto(sock, &msg, size, 0, (struct sockaddr *)&saddr, addrlen);
+		retval = sendto(sock, &msg, sizeof(struct hotplug_msg), 0,
+				(struct sockaddr *)&saddr, addrlen);
 		if (retval != -1) {
 			retval = 0;
 			goto close_and_exit;
===== libsysfs/sysfs.h 1.8 vs edited =====
--- 1.8/libsysfs/sysfs.h	Sat Mar 27 23:32:48 2004
+++ edited/libsysfs/sysfs.h	Sat Mar 27 23:33:06 2004
@@ -36,8 +36,7 @@
 /* Debugging */
 #ifdef DEBUG
 #include "../logging.h" 
-#define dprintf(format, arg...)								\
-	dbg(format, ##arg)
+#define dprintf(format, arg...) dbg(format, ##arg)
 #else
 #define dprintf(format, arg...) do { } while (0)
 #endif