[PATCH] udevd: fix WAIT_FOR_SYSFS

[PATCH] udevd: fix WAIT_FOR_SYSFS

[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.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

diff --git a/udev/udev-rules.c b/udev/udev-rules.c
index 55e27b7..168867b 100644
--- a/udev/udev-rules.c
+++ b/udev/udev-rules.c
@@ -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;

Re: [PATCH] udevd: fix WAIT_FOR_SYSFS

[Kay Sievers]

On Thu, Oct 23, 2008 at 20:37, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> 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

Re: [PATCH] udevd: fix WAIT_FOR_SYSFS

[Kay Sievers]

On Thu, Oct 23, 2008 at 21:03, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Thu, Oct 23, 2008 at 20:37, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>> 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

Re: [PATCH] udevd: fix WAIT_FOR_SYSFS

[Alan Jenkins]

Kay Sievers wrote:
> On Thu, Oct 23, 2008 at 21:03, Kay Sievers <kay.sievers@vrfy.org> wrote:
>   
>> On Thu, Oct 23, 2008 at 20:37, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>>     
>>> 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
>   

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

Re: [PATCH] udevd: fix WAIT_FOR_SYSFS

[Kay Sievers]

On Thu, Oct 23, 2008 at 21:56, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> Kay Sievers wrote:
>> On Thu, Oct 23, 2008 at 21:03, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>> On Thu, Oct 23, 2008 at 20:37, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>>>
>>>> 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?

> 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

Re: [PATCH] udevd: fix WAIT_FOR_SYSFS

[Alan Jenkins]

Kay Sievers wrote:
> On Thu, Oct 23, 2008 at 21:56, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>   
>> Kay Sievers wrote:
>>     
>>> On Thu, Oct 23, 2008 at 21:03, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>>       
>>>> On Thu, Oct 23, 2008 at 20:37, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>>>>
>>>>         
>>>>> 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?
>>>       
>
>   
>> 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.
>   

Nice.

> 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

Re: [PATCH] udevd: fix WAIT_FOR_SYSFS

[Alan Jenkins]

Alan Jenkins wrote:
> Kay Sievers wrote:
>   
>> On Thu, Oct 23, 2008 at 21:56, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>>   
>>     
>>> Kay Sievers wrote:
>>>     
>>>       
>>>> On Thu, Oct 23, 2008 at 21:03, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>>>       
>>>>         
>>>>> On Thu, Oct 23, 2008 at 20:37, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>>>>>
>>>>>         
>>>>>           
>>>>>> 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?
>>>>       
>>>>         
>>   
>>     
>>> 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.
>>   
>>     
>
> Nice.
>
>   
>> 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?
>   

And for bonus points...

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

Alan

Re: [PATCH] udevd: fix WAIT_FOR_SYSFS

[Kay Sievers]

On Sat, Oct 25, 2008 at 18:19, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> Kay Sievers wrote:

>> 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

Re: [PATCH] udevd: fix WAIT_FOR_SYSFS

[Kay Sievers]

On Sat, Oct 25, 2008 at 19:19, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> Alan Jenkins wrote:
>> 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

Re: [PATCH] udevd: fix WAIT_FOR_SYSFS

[Alan Jenkins]

Kay Sievers wrote:
> On Sat, Oct 25, 2008 at 18:19, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>   
>> Kay Sievers wrote:
>>     
>
>   
>>> 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
>   

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