Re: [UDEV, PATCH] Add profiling support to Makefile

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

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

On Tue, 2005-02-15 at 09:45 +0100, Michael Buesch wrote:
>Quoting Greg KH <greg@kroah.com>:
>> On Sun, Feb 13, 2005 at 05:16:51PM +0100, Michael Buesch wrote:
>> > Hi,
>> > 
>> > This patch adds an option to the Makefile to enable
>> > profiling. Profiling is only enabled, when not building
>> > against klibc, as klibc lacks support for mcount().
>> > 
>> > Please apply. Thanks.
>> 
>> You forgot to update the documentation about this option, so I'll hold
>> off till you redo that :)
>
>Oops. :O Here it is.
>
>> Also, what has profiling information helped you out with in udev?  Found
>> anything interesting?  Or are you just using it for coverage analysis
>> (which is a flawed thing, but that's another topic...)
>
>I did not do much testing, yet, because of missing time.
>But I will do that.
>My first testrun showed that the namedev matchrule function and the udevdb
>are the slowest parts of the whole executable. So maybe there's some
>optimization possible. But I'll look closer at it later.

The biggest part of it seems libsysfs. A replacement of the attribute
reading functions with a simple open() gives ~20% speed increase for
udevstart on my box.

Kay 

[-- Attachment #2: udev-no-libsysfs-attr-read-01.patch --]
[-- Type: text/x-patch, Size: 8963 bytes --]

===== namedev.c 1.180 vs edited =====
--- 1.180/namedev.c	2005-02-13 22:03:06 +01:00
+++ edited/namedev.c	2005-02-15 10:02:15 +01:00
@@ -36,13 +36,16 @@
 #include "libsysfs/sysfs/libsysfs.h"
 #include "list.h"
 #include "udev.h"
+#include "udev_sysfs.h"
 #include "udev_utils.h"
 #include "udev_version.h"
 #include "logging.h"
 #include "namedev.h"
 #include "udev_db.h"
 
-static struct sysfs_attribute *find_sysfs_attribute(struct sysfs_class_device *class_dev, struct sysfs_device *sysfs_device, char *attr);
+static int find_sysfs_attribute(struct sysfs_class_device *class_dev,
+				struct sysfs_device *sysfs_device, const char *attr,
+				char *value, size_t vlen);
 
 /* compare string with pattern (supports * ? [0-9] [!A-Z]) */
 static int strcmp_pattern(const char *p, const char *s)
@@ -182,7 +185,6 @@
 	char *spos;
 	char *rest;
 	int slen;
-	struct sysfs_attribute *tmpattr;
 	unsigned int next_free_number;
 	struct sysfs_class_device *class_dev_parent;
 
@@ -265,27 +267,26 @@
 			break;
 		case 's':
 			if (attr != NULL) {
-				tmpattr = find_sysfs_attribute(class_dev, sysfs_device, attr);
-				if (tmpattr == NULL) {
-					dbg("sysfa attribute '%s' not found", attr);
+				char value[VALUE_SIZE];
+
+				if (find_sysfs_attribute(class_dev, sysfs_device, attr, value, sizeof(value)) != 0) {
+					dbg("sysfs attribute '%s' not found", attr);
 					break;
 				}
 				/* strip trailing whitespace of matching value */
-				if (isspace(tmpattr->value[strlen(tmpattr->value)-1])) {
-					i = len = strlen(tmpattr->value);
-					while (i > 0 &&  isspace(tmpattr->value[i-1]))
+				if (isspace(value[strlen(value)-1])) {
+					i = len = strlen(value);
+					while (i > 0 &&  isspace(value[i-1]))
 						i--;
 					if (i < len) {
-						tmpattr->value[i] = '\0';
-						dbg("remove %i trailing whitespace chars from '%s'",
-							 len - i, tmpattr->value);
+						value[i] = '\0';
+						dbg("remove %i trailing whitespace chars from '%s'", len - i, value);
 					}
 				}
-				strfieldcatmax(string, tmpattr->value, maxsize);
-				dbg("substitute sysfs value '%s'", tmpattr->value);
-			} else {
+				strfieldcatmax(string, value, maxsize);
+				dbg("substitute sysfs value '%s'", value);
+			} else
 				dbg("missing attribute");
-			}
 			break;
 		case '%':
 			strfieldcatmax(string, "%", maxsize);
@@ -448,67 +449,64 @@
 	return retval;
 }
 
-static struct sysfs_attribute *find_sysfs_attribute(struct sysfs_class_device *class_dev, struct sysfs_device *sysfs_device, char *attr)
+static int find_sysfs_attribute(struct sysfs_class_device *class_dev,
+				struct sysfs_device *sysfs_device, const char *attr,
+				char *value, size_t vlen)
 {
-	struct sysfs_attribute *tmpattr = NULL;
 	char *c;
 
-	dbg("look for device attribute '%s'", attr);
+	dbg("look for class device attribute '%s'", attr);
 	/* try to find the attribute in the class device directory */
-	tmpattr = sysfs_get_classdev_attr(class_dev, attr);
-	if (tmpattr)
+	if (sysfs_read_attr(class_dev->path, attr, value, vlen) == 0)
 		goto attr_found;
 
 	/* look in the class device directory if present */
 	if (sysfs_device) {
-		tmpattr = sysfs_get_device_attr(sysfs_device, attr);
-		if (tmpattr)
+		dbg("look for devices device attribute '%s'", attr);
+		if (sysfs_read_attr(sysfs_device->path, attr, value, vlen) == 0)
 			goto attr_found;
 	}
 
-	return NULL;
+	return -1;
 
 attr_found:
-	c = strchr(tmpattr->value, '\n');
+	c = strchr(value, '\n');
 	if (c != NULL)
 		c[0] = '\0';
 
-	dbg("found attribute '%s'", tmpattr->path);
-	return tmpattr;
+	dbg("found attribute '%s'", attr);
+	return 0;
 }
 
 static int compare_sysfs_attribute(struct sysfs_class_device *class_dev, struct sysfs_device *sysfs_device, struct sysfs_pair *pair)
 {
-	struct sysfs_attribute *tmpattr;
+	char value[VALUE_SIZE];
 	int i;
 	int len;
 
-	if ((pair == NULL) || (pair->file[0] == '\0') || (pair->value == '\0'))
+	if ((pair == NULL) || (pair->file[0] == '\0') || (pair->value[0] == '\0'))
 		return -ENODEV;
 
-	tmpattr = find_sysfs_attribute(class_dev, sysfs_device, pair->file);
-	if (tmpattr == NULL)
+	if (find_sysfs_attribute(class_dev, sysfs_device, pair->file, value, sizeof(value)) != 0)
 		return -ENODEV;
 
 	/* strip trailing whitespace of value, if not asked to match for it */
 	if (! isspace(pair->value[strlen(pair->value)-1])) {
-		i = len = strlen(tmpattr->value);
-		while (i > 0 &&  isspace(tmpattr->value[i-1]))
+		i = len = strlen(value);
+		while (i > 0 &&  isspace(value[i-1]))
 			i--;
 		if (i < len) {
-			tmpattr->value[i] = '\0';
+			value[i] = '\0';
 			dbg("remove %i trailing whitespace chars from '%s'",
-			    len - i, tmpattr->value);
+			    len - i, value);
 		}
 	}
 
-	dbg("compare attribute '%s' value '%s' with '%s'",
-		  pair->file, tmpattr->value, pair->value);
-	if (strcmp_pattern(pair->value, tmpattr->value) != 0)
+	dbg("compare attribute '%s' value '%s' with '%s'", pair->file, value, pair->value);
+	if (strcmp_pattern(pair->value, value) != 0)
 		return -ENODEV;
 
-	dbg("found matching attribute '%s' with value '%s'",
-	    pair->file, pair->value);
+	dbg("found matching attribute '%s' with value '%s'", pair->file, pair->value);
 	return 0;
 }
 
===== udev_add.c 1.93 vs edited =====
--- 1.93/udev_add.c	2005-02-13 22:03:06 +01:00
+++ edited/udev_add.c	2005-02-15 10:02:15 +01:00
@@ -38,6 +38,7 @@
 
 #include "libsysfs/sysfs/libsysfs.h"
 #include "udev.h"
+#include "udev_sysfs.h"
 #include "udev_utils.h"
 #include "udev_version.h"
 #include "logging.h"
@@ -51,14 +52,13 @@
  */
 static int get_major_minor(struct sysfs_class_device *class_dev, struct udevice *udev)
 {
-	struct sysfs_attribute *attr = NULL;
+	char value[VALUE_SIZE];
 
-	attr = sysfs_get_classdev_attr(class_dev, "dev");
-	if (attr == NULL)
+	if (sysfs_read_attr(class_dev->path, "dev", value, sizeof(value)) != 0)
 		goto error;
-	dbg("dev='%s'", attr->value);
+	dbg("dev='%s'", value);
 
-	if (sscanf(attr->value, "%u:%u", &udev->major, &udev->minor) != 2)
+	if (sscanf(value, "%u:%u", &udev->major, &udev->minor) != 2)
 		goto error;
 	dbg("found major=%d, minor=%d", udev->major, udev->minor);
 
@@ -195,13 +195,12 @@
 
 	/* create all_partitions if requested */
 	if (udev->partitions) {
-		struct sysfs_attribute *attr;
+		char value[VALUE_SIZE];
 		int range;
 
 		/* take the maximum registered minor range */
-		attr = sysfs_get_classdev_attr(class_dev, "range");
-		if (attr) {
-			range = atoi(attr->value);
+		if (sysfs_read_attr(class_dev->path, "range", value, sizeof(value)) == 0) {
+			range = atoi(value);
 			if (range > 1)
 				udev->partitions = range-1;
 		}
===== udev_sysfs.c 1.22 vs edited =====
--- 1.22/udev_sysfs.c	2005-02-04 18:38:55 +01:00
+++ edited/udev_sysfs.c	2005-02-15 10:02:15 +01:00
@@ -26,6 +26,7 @@
 #include <string.h>
 #include <ctype.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <sys/stat.h>
 
 #include "libsysfs/sysfs/libsysfs.h"
@@ -33,6 +34,27 @@
 #include "udev_sysfs.h"
 #include "udev_utils.h"
 #include "logging.h"
+
+int sysfs_read_attr(const char *path, const char *attr, char *value, size_t vlen)
+{
+	char file[SYSFS_PATH_MAX];
+	int  fd;
+	ssize_t len;
+
+	snprintf(file, SYSFS_PATH_MAX, "%s/%s", path, attr);
+	file[SYSFS_PATH_MAX-1] = '\0';
+
+	fd = open(file, O_RDONLY);
+	if (fd <0)
+		return -1;
+
+	len = read(fd, value, vlen);
+	if (len <= 0)
+		return -1;
+	value[len-1] = '\0';
+
+	return 0;
+}
 
 /* list of subsystem specific files, NULL if there is no file to wait for */
 static const struct subsystem_file {
===== udev_sysfs.h 1.2 vs edited =====
--- 1.2/udev_sysfs.h	2004-11-13 04:36:46 +01:00
+++ edited/udev_sysfs.h	2005-02-15 10:02:16 +01:00
@@ -27,6 +27,9 @@
 #define WAIT_MAX_SECONDS		5
 #define WAIT_LOOP_PER_SECOND		20
 
+/* libsysfs overkill replacement */
+extern int sysfs_read_attr(const char *path, const char *attr, char *value, size_t vlen);
+
 extern int subsystem_expect_no_dev(const char *subsystem);
 
 /* /sys/class /sys/block devices */
===== libsysfs/sysfs_utils.c 1.14 vs edited =====
--- 1.14/libsysfs/sysfs_utils.c	2004-10-20 05:15:26 +02:00
+++ edited/libsysfs/sysfs_utils.c	2005-02-15 10:02:16 +01:00
@@ -109,20 +109,25 @@
  */
 int sysfs_get_mnt_path(char *mnt_path, size_t len)
 {
-	char *sysfs_path = NULL;
+	static char path[SYSFS_PATH_MAX] = "";
+	char *sysfs_path;
 	int ret = 0;
 
 	if (mnt_path == NULL || len == 0) {
 		errno = EINVAL;
 		return -1;
 	}
-	sysfs_path = getenv(SYSFS_PATH_ENV);
-	if (sysfs_path != NULL) {
-		safestrcpymax(mnt_path, sysfs_path, len);
-		if ((sysfs_remove_trailing_slash(mnt_path)) != 0)
-			return 1;
-	} else
-		ret = sysfs_get_fs_mnt_path(SYSFS_FSTYPE_NAME, mnt_path, len);
+
+	if (path[0] == '\0') {
+		sysfs_path = getenv(SYSFS_PATH_ENV);
+		if (sysfs_path != NULL) {
+			safestrcpy(path, sysfs_path);
+			if ((sysfs_remove_trailing_slash(path)) != 0)
+				return 1;
+		} else
+			ret = sysfs_get_fs_mnt_path(SYSFS_FSTYPE_NAME, path, sizeof(path));
+	}
+	safestrcpymax(mnt_path, path, len);
 
 	return ret;
 }