udevd: fix WAIT_FOR_SYSFS

PATCH In review Alan Jenkins 2 participants 10 messages Oct 23, 2008 – Oct 25, 2008

lore

Share & tools

lore

Alan Jenkins

author

2008-10-23 18:37 UTC #0 lore raw

PATCH 1/1 udevd: fix WAIT_FOR_SYSFS

The wait should be ordered after matching KERNEL, ENV, etc. but before ATTR.

Without this, WAIT_FOR_SYSFS rules will be applied unconditionally to all events.

Signed-off-by Alan Jenkins <[email protected]>

udev/udev-rules.c +16 ~~-16~~

index 55e27b7..168867b 100644
@@ -52,7 +52,6 @@ enum token_type {
 	TK_UNDEF,
 	TK_RULE,
 
-	TK_M_WAITFOR,			/* val */
 	TK_M_ACTION,			/* val */
 	TK_M_DEVPATH,			/* val */
 	TK_M_KERNEL,			/* val */

@@ -61,6 +60,7 @@ enum token_type {
 	TK_M_ENV,			/* val, attr */
 	TK_M_SUBSYSTEM,			/* val */
 	TK_M_DRIVER,			/* val */
+	TK_M_WAITFOR,			/* val */
 	TK_M_ATTR,			/* val, attr */
 
 	TK_M_KERNELS,			/* val */

@@ -104,7 +104,6 @@ static const char *token_str[] = {
 	[TK_UNDEF] =			"UNDEF",
 	[TK_RULE] =			"RULE",
 
-	[TK_M_WAITFOR] =		"M WAITFOR",
 	[TK_M_ACTION] =			"M ACTION",
 	[TK_M_DEVPATH] =		"M DEVPATH",
 	[TK_M_KERNEL] =			"M KERNEL",

@@ -113,6 +112,7 @@ static const char *token_str[] = {
 	[TK_M_ENV] =			"M ENV",
 	[TK_M_SUBSYSTEM] =		"M SUBSYSTEM",
 	[TK_M_DRIVER] =			"M DRIVER",
+	[TK_M_WAITFOR] =		"M WAITFOR",
 	[TK_M_ATTR] =			"M ATTR",
 
 	[TK_M_KERNELS] =		"M KERNELS",

@@ -591,12 +591,12 @@ static int rule_add_token(struct rule_tmp *rule_tmp, enum token_type type,
 	mode_t mode = 0000;
 
 	switch (type) {
-	case TK_M_WAITFOR:
 	case TK_M_ACTION:
 	case TK_M_DEVPATH:
 	case TK_M_KERNEL:
 	case TK_M_SUBSYSTEM:
 	case TK_M_DRIVER:
+	case TK_M_WAITFOR:
 	case TK_M_DEVLINK:
 	case TK_M_NAME:
 	case TK_M_KERNELS:

@@ -696,12 +696,12 @@ static void dump_token(struct udev_rules *rules, struct token *token)
 			    &rules->buf[token->rule.label_off]);
 			break;
 		}
-	case TK_M_WAITFOR:
 	case TK_M_ACTION:
 	case TK_M_DEVPATH:
 	case TK_M_KERNEL:
 	case TK_M_SUBSYSTEM:
 	case TK_M_DRIVER:
+	case TK_M_WAITFOR:
 	case TK_M_DEVLINK:
 	case TK_M_NAME:
 	case TK_M_KERNELS:

@@ -1612,18 +1612,6 @@ int udev_rules_apply_to_event(struct udev_rules *rules, struct udev_event *event
 			rule = cur;
 			esc = ESCAPE_UNSET;
 			break;
-		case TK_M_WAITFOR:
-			{
-				char filename[UTIL_PATH_SIZE];
-				int found;
-
-				util_strlcpy(filename, &rules->buf[cur->key.value_off], sizeof(filename));
-				udev_event_apply_format(event, filename, sizeof(filename));
-				found = (wait_for_file(event->dev, filename, 10) = 0);
-				if (!found && (cur->key.op != KEY_OP_NOMATCH))
-					goto nomatch;
-				break;
-			}
 		case TK_M_ACTION:
 			if (match_key(rules, cur, udev_device_get_action(event->dev)) != 0)
 				goto nomatch;

@@ -1684,6 +1672,18 @@ int udev_rules_apply_to_event(struct udev_rules *rules, struct udev_event *event
 			if (match_key(rules, cur, udev_device_get_driver(event->dev)) != 0)
 				goto nomatch;
 			break;
+		case TK_M_WAITFOR:
+			{
+				char filename[UTIL_PATH_SIZE];
+				int found;
+
+				util_strlcpy(filename, &rules->buf[cur->key.value_off], sizeof(filename));
+				udev_event_apply_format(event, filename, sizeof(filename));
+				found = (wait_for_file(event->dev, filename, 10) = 0);
+				if (!found && (cur->key.op != KEY_OP_NOMATCH))
+					goto nomatch;
+				break;
+			}
 		case TK_M_ATTR:
 			if (match_attr(rules, event->dev, event, cur) != 0)
 				goto nomatch;

Kay Sievers

2008-10-23 19:03 UTC #1 lore raw

Quoted 5 lines · Alan Jenkins

The wait should be ordered after matching KERNEL, ENV, etc. but before ATTR.
Without this, WAIT_FOR_SYSFS rules will be applied unconditionally to all events.

Ah, nice! Thanks for finding that out. I do not have any of theses rules left, as there are currently no know issues with recent kernels. :) Applied.

Kay

Kay Sievers

2008-10-23 19:12 UTC #2 lore raw

In reply to Alan Jenkins

Quoted 10 lines · Kay Sievers

Quoted 5 lines · Alan Jenkins
The wait should be ordered after matching KERNEL, ENV, etc. but before ATTR.
Without this, WAIT_FOR_SYSFS rules will be applied unconditionally to all events.
Ah, nice! Thanks for finding that out. I do not have any of theses rules left, as there are currently no know issues with recent kernels. :) Applied.

Oh, now that I expect it works for you without the long delay. :) How does is compare? Is it still slower?

Thanks,
Kay

Alan Jenkins

author

2008-10-23 19:56 UTC #3 lore raw

In reply to Alan Jenkins

Quoted 15 lines · Kay Sievers

Quoted 5 lines · Kay Sievers
Quoted 3 lines · Alan Jenkins
The wait should be ordered after matching KERNEL, ENV, etc. but before ATTR.
Quoted 7 lines
Quoted 3 lines
Without this, WAIT_FOR_SYSFS rules will be applied unconditionally to all events.
Ah, nice! Thanks for finding that out. I do not have any of theses rules left, as there are currently no know issues with recent kernels. :) Applied.

Quoted 6 lines

Oh, now that I expect it works for you without the long delay. :) How does is compare? Is it still slower?
Thanks,
Kay

No, that fixed it. The network interface rename problem seems to have gone as well (!).

I just tested it on my desktop. oprofile says it now _reduces_ cpu cycles in udevd by 10%. Not big enough to show up in time(1), but not bad! And it should help more on the EeePC, which has a less lavish cpu cache.

Alan

Kay Sievers

2008-10-25 13:33 UTC #4 lore raw

In reply to Alan Jenkins

Quoted 17 lines · Alan Jenkins

Quoted 9 lines · Kay Sievers
Quoted 8 lines · Kay Sievers
Quoted 6 lines · Alan Jenkins
The wait should be ordered after matching KERNEL, ENV, etc. but before ATTR.
Without this, WAIT_FOR_SYSFS rules will be applied unconditionally to all events.
Quoted 6 lines
Quoted 3 lines
Ah, nice! Thanks for finding that out. I do not have any of theses rules left, as there are currently no know issues with recent kernels. :) Applied.
Oh, now that I expect it works for you without the long delay. :) How does is compare? Is it still slower?

Quoted 7 lines

No, that fixed it. The network interface rename problem seems to have gone as well (!).
I just tested it on my desktop. oprofile says it now _reduces_ cpu cycles in udevd by 10%. Not big enough to show up in time(1), but not bad! And it should help more on the EeePC, which has a less lavish cpu cache.

Sounds good. I've changed a few other things, which might make things faster:

We cache the results of getpwnam/getgrnam() during rules parse time, some of the rules files I have here have ~700 rules with GROUP="..." keys ...

We check at parse time, if we need to call fnmatch() for a key, or can just go with the much cheaper strcmp(). That seems to make a real difference with the large rules files I have.

Also the snprintf() that was used compose the buffer to pass the event to a socket was _very_ expensive. We just use strlcpy() now and cache the result for the next RUN+="socket:" call.

Thanks,
Kay

Alan Jenkins

author

2008-10-25 16:19 UTC #5 lore raw

In reply to Alan Jenkins

Quoted 36 lines · Kay Sievers

Quoted 23 lines · Alan Jenkins
Quoted 12 lines · Kay Sievers
Quoted 10 lines · Kay Sievers
Quoted 7 lines · Alan Jenkins
The wait should be ordered after matching KERNEL, ENV, etc. but before ATTR.
Without this, WAIT_FOR_SYSFS rules will be applied unconditionally to all events.
Quoted 7 lines
Quoted 4 lines
Ah, nice! Thanks for finding that out. I do not have any of theses rules left, as there are currently no know issues with recent kernels. :) Applied.
Oh, now that I expect it works for you without the long delay. :) How does is compare? Is it still slower?
Quoted 8 lines
No, that fixed it. The network interface rename problem seems to have gone as well (!).
I just tested it on my desktop. oprofile says it now _reduces_ cpu cycles in udevd by 10%. Not big enough to show up in time(1), but not bad! And it should help more on the EeePC, which has a less lavish cpu cache.
Sounds good. I've changed a few other things, which might make things faster:

Quoted 8 lines

We cache the results of getpwnam/getgrnam() during rules parse time, some of the rules files I have here have ~700 rules with GROUP="..." keys ...
We check at parse time, if we need to call fnmatch() for a key, or can just go with the much cheaper strcmp(). That seems to make a real difference with the large rules files I have.

Nice.

Quoted 4 lines

Also the snprintf() that was used compose the buffer to pass the event to a socket was _very_ expensive. We just use strlcpy() now and cache the result for the next RUN+="socket:" call.

I might adapt that for udev-exec.

But I think it needs a flag like envp_uptodate, no?

Regards
Alan

Alan Jenkins

author

2008-10-25 17:19 UTC #6 lore raw

In reply to Alan Jenkins

Quoted 64 lines · Alan Jenkins

Quoted 43 lines · Kay Sievers
Quoted 29 lines · Alan Jenkins
Quoted 15 lines · Kay Sievers
Quoted 12 lines · Kay Sievers
Quoted 8 lines · Alan Jenkins
The wait should be ordered after matching KERNEL, ENV, etc. but before ATTR.
Without this, WAIT_FOR_SYSFS rules will be applied unconditionally to all events.
Quoted 9 lines
Quoted 5 lines
Ah, nice! Thanks for finding that out. I do not have any of theses rules left, as there are currently no know issues with recent kernels. :) Applied.
Oh, now that I expect it works for you without the long delay. :) How does is compare? Is it still slower?
Quoted 9 lines
No, that fixed it. The network interface rename problem seems to have gone as well (!).
I just tested it on my desktop. oprofile says it now _reduces_ cpu cycles in udevd by 10%. Not big enough to show up in time(1), but not bad! And it should help more on the EeePC, which has a less lavish cpu cache.
Sounds good. I've changed a few other things, which might make things faster:
Quoted 9 lines
We cache the results of getpwnam/getgrnam() during rules parse time, some of the rules files I have here have ~700 rules with GROUP="..." keys ...
We check at parse time, if we need to call fnmatch() for a key, or can just go with the much cheaper strcmp(). That seems to make a real difference with the large rules files I have.
Nice.
Quoted 5 lines
Also the snprintf() that was used compose the buffer to pass the event to a socket was _very_ expensive. We just use strlcpy() now and cache the result for the next RUN+="socket:" call.

Quoted 4 lines

I might adapt that for udev-exec.
But I think it needs a flag like envp_uptodate, no?

And for bonus points...

udev_device_get_properties_envp() could return pointers into this buffer, instead of allocating individual strings on the heap.

Alan

Kay Sievers

2008-10-25 17:46 UTC #7 lore raw

In reply to Alan Jenkins

Quoted 9 lines · Alan Jenkins

Quoted 3 lines · Kay Sievers
Also the snprintf() that was used compose the buffer to pass the event to a socket was _very_ expensive. We just use strlcpy() now and cache the result for the next RUN+="socket:" call.
I might adapt that for udev-exec.
But I think it needs a flag like envp_uptodate, no?

It uses monitor_buf_len, which is reset to 0, if a property gets changed. Is that what you mean?

Kay

Kay Sievers

2008-10-25 17:48 UTC #8 lore raw

In reply to Alan Jenkins

Quoted 7 lines · Alan Jenkins

Quoted 1 line · Alan Jenkins
Kay Sievers wrote:
And for bonus points...
udev_device_get_properties_envp() could return pointers into this buffer, instead of allocating individual strings on the heap.

Yeah, we can do this, sounds like a nice idea, to compose the buffer and envp at the same time. Will look into it next, if you don't beat me. :)

Kay

Alan Jenkins

author

2008-10-25 17:57 UTC #9 lore raw

In reply to Alan Jenkins

Quoted 12 lines · Kay Sievers

Quoted 10 lines · Alan Jenkins
Quoted 4 lines · Kay Sievers
Also the snprintf() that was used compose the buffer to pass the event to a socket was _very_ expensive. We just use strlcpy() now and cache the result for the next RUN+="socket:" call.
I might adapt that for udev-exec.

Quoted 8 lines

Quoted 2 lines
But I think it needs a flag like envp_uptodate, no?
It uses monitor_buf_len, which is reset to 0, if a property gets changed. Is that what you mean?
Kay

Sneaky. Yes, that's what I was looking for.