[PATCH] cleanup udevstart

[PATCH] cleanup udevstart

[Kay Sievers]

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

I just wanted to terminate the snprintf() strings, cause I can see a
overflow with closed eyes after all the audit :)

But then I changed a bit more to bring it in line with the style of the
other files. I replaced the exec_udev() function with the one from
udevd, cause we don't need to read the stdout from udev.

Please have a look if it still works for you too and not
only for usernames with 3 characters :)

thanks,
Kay

[-- Attachment #2: 01-cleanup-undevstart.patch --]
[-- Type: text/plain, Size: 7425 bytes --]

===== udevstart.c 1.1 vs edited =====
--- 1.1/udevstart.c	Tue Mar  2 02:30:38 2004
+++ edited/udevstart.c	Tue Mar  2 22:31:16 2004
@@ -27,7 +27,6 @@
 #include <stdio.h>
 #include <errno.h>
 #include <ctype.h>
-#include <signal.h>
 #include <dirent.h>
 #include <sys/wait.h>
 
@@ -47,133 +46,86 @@
 #endif
 
 
-#define MAX_PATHLEN	1024
-#define SYSBLOCK	"/sys/block"
-#define SYSCLASS	"/sys/class"
+#define MAX_PATHLEN		1024
+#define SYSBLOCK		"/sys/block"
+#define SYSCLASS		"/sys/class"
+#define UDEV_BIN		"/sbin/udev"
 
-static int execute_udev(char *path, char *value, int len)
+static void udev_exec(const char *path, const char* subsystem)
 {
-	int retval;
-	int res;
-	int status;
-	int fds[2];
 	pid_t pid;
-	int value_set = 0;
-	char buffer[255];
-	char *pos;
-
-	retval = pipe(fds);
-	if (retval != 0) {
-		dbg("pipe failed");
-		return -1;
-	}
+	char action[] = "ACTION=add";
+	char devpath[MAX_PATHLEN];
+	char nosleep[] = "UDEV_NO_SLEEP=1";
+	char *env[] = { action, devpath, nosleep, NULL };
+
+	snprintf(devpath, MAX_PATHLEN, "DEVPATH=%s", path);
+	devpath[MAX_PATHLEN-1] = '\0';
+
 	pid = fork();
-	switch(pid) {
+	switch (pid) {
 	case 0:
 		/* child */
-		close(STDOUT_FILENO);
-
-		/* dup write side of pipe to STDOUT */
-		dup(fds[1]);
-
-		dbg("executing /sbin/udev '%s'", path);
-		retval = execl("/sbin/udev", "/sbin/udev", path, NULL);
-
-		info("execution of '%s' failed", path);
+		execle(UDEV_BIN, "udev", subsystem, NULL, env);
+		dbg("exec of child failed");
 		exit(1);
+		break;
 	case -1:
-		dbg("fork failed");
-		return -1;
+		dbg("fork of child failed");
+		break;
 	default:
-		/* parent reads from fds[0] */
-		close(fds[1]);
-		retval = 0;
-		while (1) {
-			res = read(fds[0], buffer, sizeof(buffer) - 1);
-			if (res <= 0)
-				break;
-			buffer[res] = '\0';
-			if (res > len) {
-				dbg("result len %d too short", len);
-				retval = -1;
-			}
-			if (value_set) {
-				dbg("result value already set");
-				retval = -1;
-			} else {
-				value_set = 1;
-				strncpy(value, buffer, len);
-				pos = value + strlen(value)-1;
-				if (pos[0] == '\n')
-					pos[0] = '\0';
-				dbg("result is '%s'", value);
-			}
-		}
-		close(fds[0]);
-		res = wait(&status);
-		if (res < 0) {
-			dbg("wait failed result %d", res);
-			retval = -1;
-		}
-
-		if (!WIFEXITED(status) || (WEXITSTATUS(status) != 0)) {
-			dbg("exec program status 0x%x", status);
-			retval = -1;
-		}
+		wait(NULL);
 	}
-	return retval;
 }
 
 static int udev_scan(void)
 {
-	char           *devpath;
-	DIR            *dir;
-	struct dirent  *dent;
-	int             retval = -EINVAL;
-	char scratch[200];
+	char *devpath;
+	DIR *dir;
+	struct dirent *dent;
+	int retval = -EINVAL;
 
 	devpath = "block";
 	dir = opendir(SYSBLOCK);
-	if (dir) {
-		for (dent = readdir(dir); dent; dent = readdir(dir)) {
-			char            dirname[MAX_PATHLEN];
-			DIR            *dir2;
-			struct dirent  *dent2;
-			if ((strcmp(dent->d_name, ".") == 0)
-			    || (strcmp(dent->d_name, "..") == 0))
+	if (dir != NULL) {
+		for (dent = readdir(dir); dent != NULL; dent = readdir(dir)) {
+			char dirname[MAX_PATHLEN];
+			DIR *dir2;
+			struct dirent *dent2;
+
+			if ((strcmp(dent->d_name, ".") == 0) ||
+			    (strcmp(dent->d_name, "..") == 0))
 				continue;
 
 			snprintf(dirname, MAX_PATHLEN, "/block/%s", dent->d_name);
-
-			setenv("DEVPATH", dirname, 1);
-			dbg("udev block, 'DEVPATH' = '%s'", dirname);
-			execute_udev("block", scratch, sizeof(scratch));
+			dirname[MAX_PATHLEN-1] = '\0';
+			udev_exec(dirname, "block");
 
 			snprintf(dirname, MAX_PATHLEN, "%s/%s", SYSBLOCK, dent->d_name);
-
 			dir2 = opendir(dirname);
-			if (dir2) {
-				for (dent2 = readdir(dir2); dent2; dent2 = readdir(dir2)) {
-					char            dirname2[MAX_PATHLEN];
-					DIR            *dir3;
-					struct dirent  *dent3;
+			if (dir2 != NULL) {
+				for (dent2 = readdir(dir2); dent2 != NULL; dent2 = readdir(dir2)) {
+					char dirname2[MAX_PATHLEN];
+					DIR *dir3;
+					struct dirent *dent3;
 
 					if ((strcmp(dent2->d_name, ".") == 0) ||
 					    (strcmp(dent2->d_name, "..") == 0))
 						continue;
 
 					snprintf(dirname2, MAX_PATHLEN, "%s/%s", dirname, dent2->d_name);
+					dirname2[MAX_PATHLEN-1] = '\0';
 
 					dir3 = opendir(dirname2);
-					if (dir3) {
-						for (dent3 = readdir(dir3); dent3; dent3 = readdir(dir3)) {
+					if (dir3 != NULL) {
+						for (dent3 = readdir(dir3); dent3 != NULL; dent3 = readdir(dir3)) {
 							char filename[MAX_PATHLEN];
 
 							if (strcmp(dent3->d_name, "dev") == 0) {
-								snprintf(filename, MAX_PATHLEN, "/block/%s/%s", dent->d_name, dent2->d_name);
-								setenv("DEVPATH", filename, 1);
-								dbg("udev block, 'DEVPATH' = '%s'", filename);
-								execute_udev("block", scratch, sizeof(scratch));
+								snprintf(filename, MAX_PATHLEN, "/block/%s/%s",
+									 dent->d_name, dent2->d_name);
+								filename[MAX_PATHLEN-1] = '\0';
+								udev_exec(filename, "block");
 							}
 						}
 					}
@@ -184,43 +136,42 @@
 
 	devpath = "class";
 	dir = opendir(SYSCLASS);
-	if (dir) {
-		for (dent = readdir(dir); dent; dent = readdir(dir)) {
-			char            dirname[MAX_PATHLEN];
-			DIR            *dir2;
-			struct dirent  *dent2;
-			if ((strcmp(dent->d_name, ".") == 0)
-			    || (strcmp(dent->d_name, "..") == 0))
+	if (dir != NULL) {
+		for (dent = readdir(dir); dent != NULL; dent = readdir(dir)) {
+			char dirname[MAX_PATHLEN];
+			DIR *dir2;
+			struct dirent *dent2;
+
+			if ((strcmp(dent->d_name, ".") == 0) ||
+			    (strcmp(dent->d_name, "..") == 0))
 				continue;
 
 			snprintf(dirname, MAX_PATHLEN, "%s/%s", SYSCLASS, dent->d_name);
-
+			dirname[MAX_PATHLEN] = '\0';
 			dir2 = opendir(dirname);
-			if (dir2) {
-				for (dent2 = readdir(dir2); dent2; dent2 = readdir(dir2)) {
-					char            dirname2[MAX_PATHLEN];
-					DIR            *dir3;
-					struct dirent  *dent3;
+			if (dir2 != NULL) {
+				for (dent2 = readdir(dir2); dent2 != NULL; dent2 = readdir(dir2)) {
+					char dirname2[MAX_PATHLEN-1];
+					DIR *dir3;
+					struct dirent *dent3;
 
-					if ((strcmp(dent2->d_name, ".") == 0) || (strcmp(dent2->d_name, "..") == 0))
+					if ((strcmp(dent2->d_name, ".") == 0) ||
+					    (strcmp(dent2->d_name, "..") == 0))
 						continue;
 
 					snprintf(dirname2, MAX_PATHLEN, "%s/%s", dirname, dent2->d_name);
+					dirname2[MAX_PATHLEN-1] = '\0';
 
 					dir3 = opendir(dirname2);
-					if (dir3) {
-						for (dent3 = readdir(dir3); dent3; dent3 = readdir(dir3)) {
-							char
-							                filename[MAX_PATHLEN];
+					if (dir3 != NULL) {
+						for (dent3 = readdir(dir3); dent3 != NULL; dent3 = readdir(dir3)) {
+							char filename[MAX_PATHLEN];
 
 							if (strcmp(dent3->d_name, "dev") == 0) {
-								snprintf
-								    (filename,
-								     MAX_PATHLEN,
-								     "/class/%s/%s", dent->d_name, dent2->d_name);
-								setenv("DEVPATH", filename, 1);
-								dbg("udev '%s', 'DEVPATH' = '%s'", dent->d_name, filename);
-								execute_udev(dent->d_name, scratch, sizeof(scratch));
+								snprintf(filename, MAX_PATHLEN, "/class/%s/%s",
+									 dent->d_name, dent2->d_name);
+								filename[MAX_PATHLEN-1] = '\0';
+								udev_exec(filename, dent->d_name);
 							}
 						}
 					}
@@ -239,9 +190,6 @@
 int main(int argc, char **argv, char **envp)
 {
 	init_logging("udevstart");
-
-	setenv("ACTION", "add", 1);
-	setenv("UDEV_NO_SLEEP", "1", 1);
 
 	return udev_scan();
 }

Re: [PATCH] cleanup udevstart

[Greg KH]

On Tue, Mar 02, 2004 at 10:55:36PM +0100, Kay Sievers wrote:
> I just wanted to terminate the snprintf() strings, cause I can see a
> overflow with closed eyes after all the audit :)
> 
> But then I changed a bit more to bring it in line with the style of the
> other files. I replaced the exec_udev() function with the one from
> udevd, cause we don't need to read the stdout from udev.
> 
> Please have a look if it still works for you too and not
> only for usernames with 3 characters :)

Ah, much nicer, thanks a lot for doing this.  It works in my testing,
and I've applied it.

greg k-h


-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id\x1356&alloc_id438&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] cleanup udevstart

[Olaf Hering]

 On Tue, Mar 02, Kay Sievers wrote:

> +#define SYSBLOCK		"/sys/block"
> +#define SYSCLASS		"/sys/class"
> +#define UDEV_BIN		"/sbin/udev"

>  	pid = fork();

> -		retval = execl("/sbin/udev", "/sbin/udev", path, NULL);

I have not looked closer at it, wouldnt it be better to feed the udevdb
directly, instead of spawning countless udev processes? Some sort of
udev binary which takes optional directories as agument to limit it to
these sysfs dirs.


-- 
USB is for mice, FireWire is for men!

sUse lINUX ag, n√úRNBERG


-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id\x1356&alloc_id438&opÃk
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] cleanup udevstart

[Greg KH]

On Wed, Mar 03, 2004 at 12:09:51AM +0100, Olaf Hering wrote:
>  On Tue, Mar 02, Kay Sievers wrote:
> 
> > +#define SYSBLOCK		"/sys/block"
> > +#define SYSCLASS		"/sys/class"
> > +#define UDEV_BIN		"/sbin/udev"
> 
> >  	pid = fork();
> 
> > -		retval = execl("/sbin/udev", "/sbin/udev", path, NULL);
> 
> I have not looked closer at it, wouldnt it be better to feed the udevdb
> directly, instead of spawning countless udev processes? Some sort of
> udev binary which takes optional directories as agument to limit it to
> these sysfs dirs.

Nah, it's simpler this way.  We spawn and then wait, so there isn't a
zillion udev processes all running at once :)

thanks,

greg k-h


-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id\x1356&alloc_id438&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] cleanup udevstart

[Olaf Hering]

 On Tue, Mar 02, Greg KH wrote:

> On Wed, Mar 03, 2004 at 12:09:51AM +0100, Olaf Hering wrote:
> >  On Tue, Mar 02, Kay Sievers wrote:
> > 
> > > +#define SYSBLOCK		"/sys/block"
> > > +#define SYSCLASS		"/sys/class"
> > > +#define UDEV_BIN		"/sbin/udev"
> > 
> > >  	pid = fork();
> > 
> > > -		retval = execl("/sbin/udev", "/sbin/udev", path, NULL);
> > 
> > I have not looked closer at it, wouldnt it be better to feed the udevdb
> > directly, instead of spawning countless udev processes? Some sort of
> > udev binary which takes optional directories as agument to limit it to
> > these sysfs dirs.
> 
> Nah, it's simpler this way.  We spawn and then wait, so there isn't a
> zillion udev processes all running at once :)

thats the point, wait for what?
I hope the whole point is to create all nodes in a jiffy, in the long run?

-- 
USB is for mice, FireWire is for men!

sUse lINUX ag, n√úRNBERG


-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id\x1356&alloc_id438&opÃk
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] cleanup udevstart

[Greg KH]

On Wed, Mar 03, 2004 at 12:23:56AM +0100, Olaf Hering wrote:
>  On Tue, Mar 02, Greg KH wrote:
> 
> > On Wed, Mar 03, 2004 at 12:09:51AM +0100, Olaf Hering wrote:
> > >  On Tue, Mar 02, Kay Sievers wrote:
> > > 
> > > > +#define SYSBLOCK		"/sys/block"
> > > > +#define SYSCLASS		"/sys/class"
> > > > +#define UDEV_BIN		"/sbin/udev"
> > > 
> > > >  	pid = fork();
> > > 
> > > > -		retval = execl("/sbin/udev", "/sbin/udev", path, NULL);
> > > 
> > > I have not looked closer at it, wouldnt it be better to feed the udevdb
> > > directly, instead of spawning countless udev processes? Some sort of
> > > udev binary which takes optional directories as agument to limit it to
> > > these sysfs dirs.
> > 
> > Nah, it's simpler this way.  We spawn and then wait, so there isn't a
> > zillion udev processes all running at once :)
> 
> thats the point, wait for what?
> I hope the whole point is to create all nodes in a jiffy, in the long run?

At startup we have to wait as usually the next thing the initscripts do
is use those device nodes.  So we have to wait until they are all
created.

And it's fast today.  Have you timed udevstart (or even the start_udev
bash script)?  Here's my box as I do a kernel build in the background at
the same time:

# /usr/bin/time /sbin/udevstart 
Command exited with non-zero status 22
0.19user 0.67system 0:01.04elapsed 83%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (2major+6065minor)pagefaults 0swaps

1 second to populate a full /dev while the system is under a very heavy
load is pretty good I think :)

thanks,

greg k-h


-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id\x1356&alloc_id438&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] cleanup udevstart

[Olaf Hering]

 On Tue, Mar 02, Greg KH wrote:

> # /usr/bin/time /sbin/udevstart 
> Command exited with non-zero status 22
> 0.19user 0.67system 0:01.04elapsed 83%CPU (0avgtext+0avgdata 0maxresident)k
> 0inputs+0outputs (2major+6065minor)pagefaults 0swaps
> 
> 1 second to populate a full /dev while the system is under a very heavy
> load is pretty good I think :)


Yeah, I was using the klibc version, that makes it slower due to the
clever fgets implementation.

-- 
USB is for mice, FireWire is for men!

sUse lINUX ag, n√úRNBERG


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id\x1470&alloc_id638&opÃk
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] cleanup udevstart

[Kay Sievers]

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

On Mon, Mar 15, 2004 at 10:22:02PM +0100, Olaf Hering wrote:
>  On Tue, Mar 02, Greg KH wrote:
> 
> > # /usr/bin/time /sbin/udevstart 
> > Command exited with non-zero status 22
> > 0.19user 0.67system 0:01.04elapsed 83%CPU (0avgtext+0avgdata 0maxresident)k
> > 0inputs+0outputs (2major+6065minor)pagefaults 0swaps
> > 
> > 1 second to populate a full /dev while the system is under a very heavy
> > load is pretty good I think :)
> 
> 
> Yeah, I was using the klibc version, that makes it slower due to the
> clever fgets implementation.

Hey, better try to fix it :)
You may try this one.

thanks,
Kay

[-- Attachment #2: 80-mmap.patch --]
[-- Type: text/plain, Size: 2189 bytes --]

===== namedev_parse.c 1.32 vs edited =====
--- 1.32/namedev_parse.c	Fri Mar 12 17:43:30 2004
+++ edited/namedev_parse.c	Tue Mar 16 02:16:35 2004
@@ -36,6 +36,7 @@
 #include <sys/stat.h>
 #include <dirent.h>
 #include <errno.h>
+#include <sys/mman.h>
 
 #include "udev.h"
 #include "logging.h"
@@ -145,6 +146,47 @@
 	return NULL;
 }
 
+static int file_map(const char *filename, char **buf, size_t *bufsize)
+{
+	struct stat stats;
+	int fd;
+
+	fd = open(filename, O_RDONLY);
+	if (fd < 0) {
+		dbg("can't open '%s'", filename);
+		return -1;
+	}
+
+	if (fstat(fd, &stats) < 0) {
+		dbg("error stating '%s'", filename);
+		return -1;
+	}
+
+	*buf = mmap(NULL, stats.st_size, PROT_READ, MAP_SHARED, fd, 0);
+	if (*buf == MAP_FAILED) {
+		dbg("error mapping file '%s'", filename);
+		return -1;
+	}
+	*bufsize = stats.st_size;
+
+	return 0;
+}
+
+static void file_unmap(char *buf, size_t bufsize)
+{
+	munmap(buf, bufsize);
+}
+
+
+static size_t buf_get_line(char *buf, size_t buflen, size_t cur)
+{
+	size_t count = 0;
+
+	for (count = cur; count < buflen && buf[count] != '\n'; count++);
+
+	return count - cur;
+}
+
 static int namedev_parse_rules(char *filename)
 {
 	char line[255];
@@ -153,27 +195,38 @@
 	char *temp2;
 	char *temp3;
 	char *attr;
-	FILE *fd;
+	char *buf;
+	size_t bufsize;
+	size_t cur;
+	size_t count;
 	int program_given = 0;
 	int retval = 0;
 	struct config_device dev;
 
-	fd = fopen(filename, "r");
-	if (fd != NULL) {
+	if (file_map(filename, &buf, &bufsize) == 0) {
 		dbg("reading '%s' as rules file", filename);
 	} else {
 		dbg("can't open '%s' as a rules file", filename);
 		return -ENODEV;
 	}
 
+	dbg("bufsize %i", bufsize);
+
 	/* loop through the whole file */
+	cur = 0;
 	lineno = 0;
 	while (1) {
-		/* get a line */
-		temp = fgets(line, sizeof(line), fd);
-		if (temp == NULL)
-			goto exit;
+		count = buf_get_line(buf, bufsize, cur);
+
+		strncpy(line, buf + cur, count);
+		line[count] = '\0';
+		temp = line;
 		lineno++;
+
+		cur += count+1;
+		if (cur > bufsize)
+			break;
+
 		dbg_parse("read '%s'", temp);
 
 		/* eat the whitespace */
@@ -312,7 +365,7 @@
 		}
 	}
 exit:
-	fclose(fd);
+	file_unmap(buf, bufsize);
 	return retval;
 }
 

Re: [PATCH] cleanup udevstart

[Kay Sievers]

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

On Tue, Mar 16, 2004 at 02:28:56AM +0100, Kay Sievers wrote:
> On Mon, Mar 15, 2004 at 10:22:02PM +0100, Olaf Hering wrote:
> >  On Tue, Mar 02, Greg KH wrote:
> > 
> > > # /usr/bin/time /sbin/udevstart 
> > > Command exited with non-zero status 22
> > > 0.19user 0.67system 0:01.04elapsed 83%CPU (0avgtext+0avgdata 0maxresident)k
> > > 0inputs+0outputs (2major+6065minor)pagefaults 0swaps
> > > 
> > > 1 second to populate a full /dev while the system is under a very heavy
> > > load is pretty good I think :)
> > 
> > 
> > Yeah, I was using the klibc version, that makes it slower due to the
> > clever fgets implementation.
> 
> Hey, better try to fix it :)
> You may try this one.

Here are all fgets() converted to mmap(), also the config file read.
I get for a very small(20 lines) rules file:

  time ./udevstart

  klibc mmap
  real    0m0.119s
  user    0m0.020s
  sys     0m0.040s

  glibc mmap
  real    0m0.232s
  user    0m0.064s
  sys     0m0.108s

  glibc fgets
  real    0m0.308s
  user    0m0.124s
  sys     0m0.123s

  klibc fgets
  real    0m0.341s
  user    0m0.086s
  sys     0m0.195s


The strace looks really short now :)

  [pid 28262] sigaction(SIGTERM, {0x80480c4, [], SA_RESTART}, NULL, 0x804e0ec) = 0
  [pid 28262] stat("/etc/udev/udev.rules", {st_mode=S_IFREG|0644, st_size=2485, ...}) = 0
  [pid 28262] open("/etc/udev/udev.rules", O_RDONLY) = 6
  [pid 28262] fstat(6, {st_mode=S_IFREG|0644, st_size=2485, ...}) = 0
  [pid 28262] mmap2(NULL, 2485, PROT_READ, MAP_SHARED, 6, 0) = 0x4001a000
  [pid 28262] munmap(0x4001a000, 2485)    = 0
  [pid 28262] stat("/etc/udev/udev.permissions", {st_mode=S_IFREG|0644, st_size=148, ...}) = 0



thanks,
Kay

[-- Attachment #2: 80-mmap.patch --]
[-- Type: text/plain, Size: 5942 bytes --]

===== klibc_fixups.c 1.7 vs edited =====
--- 1.7/klibc_fixups.c	Tue Mar  2 00:59:53 2004
+++ edited/klibc_fixups.c	Tue Mar 16 03:30:35 2004
@@ -28,6 +28,7 @@
 #include <fcntl.h>
 #include <sys/types.h>
 
+#include "udev.h"
 #include "klibc_fixups.h"
 #include "logging.h"
 
@@ -40,22 +41,35 @@
 static unsigned long get_id_by_name(const char *uname, const char *dbfile)
 {
 	unsigned long id = -1;
-	FILE *file;
-	char buf[255];
+	char line[255];
+	char *buf;
+	size_t bufsize;
+	size_t cur;
+	size_t count;
 	char *pos;
 	char *name;
 	char *idstr;
 	char *tail;
 
-	file = fopen(dbfile, "r");
-	if (file == NULL) {
-		dbg("unable to open file '%s'", dbfile);
+	if (file_map(dbfile, &buf, &bufsize) == 0) {
+		dbg("reading '%s' as db file", dbfile);
+	} else {
+		dbg("can't open '%s' as db file", dbfile);
 		return -1;
 	}
 
+	/* loop through the whole file */
+
+	cur = 0;
 	while (1) {
-		pos = fgets(buf, sizeof(buf), file);
-		if (pos == NULL)
+		count = buf_get_line(buf, bufsize, cur);
+
+		strncpy(line, buf + cur, count);
+		line[count] = '\0';
+		pos = line;
+
+		cur += count+1;
+		if (cur > bufsize)
 			break;
 
 		/* get name */
@@ -82,7 +96,7 @@
 		}
 	}
 
-	fclose(file);
+	file_unmap(buf, bufsize);
 	return id;
 }
 
===== namedev_parse.c 1.32 vs edited =====
--- 1.32/namedev_parse.c	Fri Mar 12 17:43:30 2004
+++ edited/namedev_parse.c	Tue Mar 16 03:31:03 2004
@@ -30,7 +30,6 @@
 #include <stdlib.h>
 #include <string.h>
 #include <stdio.h>
-#include <fcntl.h>
 #include <ctype.h>
 #include <unistd.h>
 #include <sys/stat.h>
@@ -153,27 +152,36 @@
 	char *temp2;
 	char *temp3;
 	char *attr;
-	FILE *fd;
+	char *buf;
+	size_t bufsize;
+	size_t cur;
+	size_t count;
 	int program_given = 0;
 	int retval = 0;
 	struct config_device dev;
 
-	fd = fopen(filename, "r");
-	if (fd != NULL) {
+	if (file_map(filename, &buf, &bufsize) == 0) {
 		dbg("reading '%s' as rules file", filename);
 	} else {
-		dbg("can't open '%s' as a rules file", filename);
-		return -ENODEV;
+		dbg("can't open '%s' as rules file", filename);
+		return -1;
 	}
 
 	/* loop through the whole file */
+	cur = 0;
 	lineno = 0;
 	while (1) {
-		/* get a line */
-		temp = fgets(line, sizeof(line), fd);
-		if (temp == NULL)
-			goto exit;
+		count = buf_get_line(buf, bufsize, cur);
+
+		strncpy(line, buf + cur, count);
+		line[count] = '\0';
+		temp = line;
 		lineno++;
+
+		cur += count+1;
+		if (cur > bufsize)
+			break;
+
 		dbg_parse("read '%s'", temp);
 
 		/* eat the whitespace */
@@ -311,8 +319,8 @@
 			    filename, lineno, temp - line);
 		}
 	}
-exit:
-	fclose(fd);
+
+	file_unmap(buf, bufsize);
 	return retval;
 }
 
@@ -321,22 +329,31 @@
 	char line[255];
 	char *temp;
 	char *temp2;
-	FILE *fd;
+	char *buf;
+	size_t bufsize;
+	size_t cur;
+	size_t count;
 	int retval = 0;
 	struct perm_device dev;
 
-	fd = fopen(filename, "r");
-	if (fd != NULL) {
+	if (file_map(filename, &buf, &bufsize) == 0) {
 		dbg("reading '%s' as permissions file", filename);
 	} else {
 		dbg("can't open '%s' as permissions file", filename);
-		return -ENODEV;
+		return -1;
 	}
 
 	/* loop through the whole file */
+	cur = 0;
 	while (1) {
-		temp = fgets(line, sizeof(line), fd);
-		if (temp == NULL)
+		count = buf_get_line(buf, bufsize, cur);
+
+		strncpy(line, buf + cur, count);
+		line[count] = '\0';
+		temp = line;
+
+		cur += count+1;
+		if (cur > bufsize)
 			break;
 
 		dbg_parse("read '%s'", temp);
@@ -394,7 +411,7 @@
 	}
 
 exit:
-	fclose(fd);
+	file_unmap(buf, bufsize);
 	return retval;
 }
 
===== udev.h 1.51 vs edited =====
--- 1.51/udev.h	Thu Mar  4 22:40:39 2004
+++ edited/udev.h	Tue Mar 16 03:30:36 2004
@@ -27,7 +27,10 @@
 #include <string.h>
 #include <sysfs/libsysfs.h>
 #include <stddef.h>
+#include <fcntl.h>
 #include <sys/param.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
 
 #define COMMENT_CHARACTER		'#'
 
@@ -140,6 +143,43 @@
 		subsystem[SUBSYSTEM_SIZE-1] = '\0';
 
 	return subsystem;
+}
+
+static inline int file_map(const char *filename, char **buf, size_t *bufsize)
+{
+	struct stat stats;
+	int fd;
+
+	fd = open(filename, O_RDONLY);
+	if (fd < 0) {
+		return -1;
+	}
+
+	if (fstat(fd, &stats) < 0) {
+		return -1;
+	}
+
+	*buf = mmap(NULL, stats.st_size, PROT_READ, MAP_SHARED, fd, 0);
+	if (*buf == MAP_FAILED) {
+		return -1;
+	}
+	*bufsize = stats.st_size;
+
+	return 0;
+}
+
+static inline void file_unmap(char *buf, size_t bufsize)
+{
+	munmap(buf, bufsize);
+}
+
+static inline size_t buf_get_line(char *buf, size_t buflen, size_t cur)
+{
+	size_t count = 0;
+
+	for (count = cur; count < buflen && buf[count] != '\n'; count++);
+
+	return count - cur;
 }
 
 extern int udev_add_device(char *path, char *subsystem, int fake);
===== udev_config.c 1.14 vs edited =====
--- 1.14/udev_config.c	Thu Mar  4 22:40:39 2004
+++ edited/udev_config.c	Tue Mar 16 03:30:37 2004
@@ -131,12 +131,14 @@
 	char *temp;
 	char *variable;
 	char *value;
-	FILE *fd;
-	int lineno = 0;
+	char *buf;
+	size_t bufsize;
+	size_t cur;
+	size_t count;
+	int lineno;
 	int retval = 0;
-	
-	fd = fopen(udev_config_filename, "r");
-	if (fd != NULL) {
+
+	if (file_map(udev_config_filename, &buf, &bufsize) == 0) {
 		dbg("reading '%s' as config file", udev_config_filename);
 	} else {
 		dbg("can't open '%s' as config file", udev_config_filename);
@@ -144,13 +146,20 @@
 	}
 
 	/* loop through the whole file */
+	lineno = 0;
+	cur = 0;
 	while (1) {
-		/* get a line */
-		temp = fgets(line, sizeof(line), fd);
-		if (temp == NULL)
-			goto exit;
+		count = buf_get_line(buf, bufsize, cur);
+
+		strncpy(line, buf + cur, count);
+		line[count] = '\0';
+		temp = line;
 		lineno++;
 
+		cur += count+1;
+		if (cur > bufsize)
+			break;
+
 		dbg_parse("read '%s'", temp);
 
 		/* eat the whitespace at the beginning of the line */
@@ -182,8 +191,8 @@
 	}
 	dbg_parse("%s:%d:%Zd: error parsing '%s'", udev_config_filename,
 		  lineno, temp - line, temp);
-exit:
-	fclose(fd);
+
+	file_unmap(buf, bufsize);
 	return retval;
 }
 

Re: [PATCH] cleanup udevstart

[Patrick Mansfield]

On Tue, Mar 16, 2004 at 03:49:51AM +0100, Kay Sievers wrote:
> 
> Here are all fgets() converted to mmap(), also the config file read.
> I get for a very small(20 lines) rules file:
> 
>   time ./udevstart

My timings using lots of scsi_debug LUN's and 1000 line udev.rules got a
about twice the speedup for klibc compared to your timings (almost 6 times
faster). None of my udev.rules match, so all 1000 rules are compared even
for 512 LUNs.

The inlines of longer functions gains nothing (and probably the other
inlines in udev.h that are more than 2 lines long and use more than once).
Not inlining file_map and buf_get_line gives almost no difference in
times, but shrinks executables a bit.

insmod was really this:

	modprobe scsi_debug num_tgts2 max_luns\x16 delay=0

Total elapsed times as follows:

			insmod		rmmod
klibc current		4:11.42		4:06.27
klibc patch		0:38.48		0:30.89
klibc noinline 		0:38.33		0:30.89
glibc current		0:40.84		0:36.20
glibc with patch	0:40.72		0:32.41

And:

# size */udev */udevtest */udevinfo
   text    data     bss     dec     hex filename
  64888     264   22512   87664   15670 noinline-udev/udev
  65032     264   22512   87808   15700 udev-bk/udev
  64044     260   22480   86784   15300 noinline-udev/udevtest
  64188     260   22480   86928   15390 udev-bk/udevtest
  68900     236   22524   91660   1660c noinline-udev/udevinfo
  53092     140   22044   75276   1260c udev-bk/udevinfo

-- Patrick Mansfield


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id\x1470&alloc_id638&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] cleanup udevstart

[Kay Sievers]

On Tue, 2004-03-16 at 18:01, Patrick Mansfield wrote:
> On Tue, Mar 16, 2004 at 03:49:51AM +0100, Kay Sievers wrote:
> > 
> > Here are all fgets() converted to mmap(), also the config file read.
> > I get for a very small(20 lines) rules file:
> > 
> >   time ./udevstart
> 
> My timings using lots of scsi_debug LUN's and 1000 line udev.rules got a
> about twice the speedup for klibc compared to your timings (almost 6 times
> faster). None of my udev.rules match, so all 1000 rules are compared even
> for 512 LUNs.

Hey, looks like a pretty nice speedup.

> The inlines of longer functions gains nothing (and probably the other
> inlines in udev.h that are more than 2 lines long and use more than once).
> Not inlining file_map and buf_get_line gives almost no difference in
> times, but shrinks executables a bit.

Yes, I know. It was just a quick hack. I tried to avoid putting "normal"
functions into a header file. If we want this to go in, we may create a
udev_lib.[hc] or something and put in all the generic stuff currently in
udev.h.

> insmod was really this:
> 
> 	modprobe scsi_debug num_tgts2 max_luns\x16 delay=0
> 
> Total elapsed times as follows:
> 
> 			insmod		rmmod
> klibc current		4:11.42		4:06.27
> klibc patch		0:38.48		0:30.89
> klibc noinline 		0:38.33		0:30.89
> glibc current		0:40.84		0:36.20
> glibc with patch	0:40.72		0:32.41

Much thanks for your measurements. It's always better to have real
numbers :)
It seeems that we need to fix it. The mmap() difference for glibc is
pretty small, so it should also be possible to fix the fgets() in klibc.
Any suggestion to go with mmap() or fix klibc instead?

thanks again,
Kay




-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id\x1470&alloc_id638&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] cleanup udevstart

[Patrick Mansfield]

On Tue, Mar 16, 2004 at 10:57:55PM +0100, Kay Sievers wrote:
> On Tue, 2004-03-16 at 18:01, Patrick Mansfield wrote:
> > On Tue, Mar 16, 2004 at 03:49:51AM +0100, Kay Sievers wrote:
> 
> Yes, I know. It was just a quick hack. I tried to avoid putting "normal"
> functions into a header file. If we want this to go in, we may create a
> udev_lib.[hc] or something and put in all the generic stuff currently in
> udev.h.

udev_lib sounds good, I created a udev-other.c.

> Much thanks for your measurements. It's always better to have real
> numbers :)

Well they are still not real disks. I assume we have even less of an issue
using real disks, as IO times increase and the hotplug and udev overhead
becomes a smaller percentage.

> It seeems that we need to fix it. The mmap() difference for glibc is
> pretty small, so it should also be possible to fix the fgets() in klibc.
> Any suggestion to go with mmap() or fix klibc instead?

IMO klibc should buffer fgets() no matter what udev uses.

So fix klibc and use its fgets, and if that can't happen soon use your
mmap patch.

-- Patrick Mansfield


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id\x1470&alloc_id638&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel