* [PATCH] libudev: Allocate udev_device->envp lazily
@ 2008-10-21 10:11 Alan Jenkins
2008-10-21 11:07 ` Kay Sievers
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Alan Jenkins @ 2008-10-21 10:11 UTC (permalink / raw)
To: linux-hotplug
Allocate udev_device->envp lazily
It's a pity to allocate 128 words when we don't need them.
Measured 2% _user_ cpu time reduction on EeePC coldplug.
Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
diff --git a/udev/lib/libudev-device.c b/udev/lib/libudev-device.c
index b54c727..69ab1f7 100644
--- a/udev/lib/libudev-device.c
+++ b/udev/lib/libudev-device.c
@@ -31,6 +31,8 @@
#include "libudev.h"
#include "libudev-private.h"
+#define ENVP_SIZE 128
+
struct udev_device {
int refcount;
struct udev *udev;
@@ -46,7 +48,7 @@ struct udev_device {
struct udev_list_node devlinks_list;
int devlinks_uptodate;
struct udev_list_node properties_list;
- char *envp[128];
+ char **envp;
int envp_uptodate;
char *driver;
int driver_set;
@@ -611,8 +613,11 @@ void udev_device_unref(struct udev_device *udev_device)
free(udev_device->devpath_old);
free(udev_device->physdevpath);
udev_list_cleanup_entries(udev_device->udev, &udev_device->sysattr_list);
- for (i = 0; i < ARRAY_SIZE(udev_device->envp) && udev_device->envp[i] != NULL; i++)
- free(udev_device->envp[i]);
+ if (udev_device->envp != NULL) {
+ for (i = 0; i < ENVP_SIZE && udev_device->envp[i] != NULL; i++)
+ free(udev_device->envp[i]);
+ free(udev_device->envp);
+ }
info(udev_device->udev, "udev_device: %p released\n", udev_device);
free(udev_device);
}
@@ -1014,14 +1019,18 @@ char **udev_device_get_properties_envp(struct udev_device *udev_device)
unsigned int i;
struct udev_list_entry *list_entry;
- for (i = 0; i < ARRAY_SIZE(udev_device->envp) && udev_device->envp[i] != NULL; i++)
- free(udev_device->envp[i]);
+ if (udev_device->envp) {
+ for (i = 0; i < 128 && udev_device->envp[i] != NULL; i++)
+ free(udev_device->envp[i]);
+ } else
+ udev_device->envp = malloc(sizeof(char *) * ENVP_SIZE);
+
i = 0;
udev_list_entry_foreach(list_entry, udev_device_get_properties_list_entry(udev_device)) {
asprintf(&udev_device->envp[i++], "%s=%s",
udev_list_entry_get_name(list_entry),
udev_list_entry_get_value(list_entry));
- if (i+1 >= ARRAY_SIZE(udev_device->envp))
+ if (i+1 >= ENVP_SIZE)
break;
}
udev_device->envp[i] = NULL;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] libudev: Allocate udev_device->envp lazily
2008-10-21 10:11 [PATCH] libudev: Allocate udev_device->envp lazily Alan Jenkins
@ 2008-10-21 11:07 ` Kay Sievers
2008-10-21 11:48 ` Alan Jenkins
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Kay Sievers @ 2008-10-21 11:07 UTC (permalink / raw)
To: linux-hotplug
On Tue, Oct 21, 2008 at 12:11, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> Allocate udev_device->envp lazily
>
> It's a pity to allocate 128 words when we don't need them.
> Measured 2% _user_ cpu time reduction on EeePC coldplug.
Any idea what might take 2% here? We don't do anything if the envp
isn't used. We do almost the same thing with or without the patch
besides that "struct udev_device" is 1016 bytes smaller now, and we
malloc() it.
Kay
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libudev: Allocate udev_device->envp lazily
2008-10-21 10:11 [PATCH] libudev: Allocate udev_device->envp lazily Alan Jenkins
2008-10-21 11:07 ` Kay Sievers
@ 2008-10-21 11:48 ` Alan Jenkins
2008-10-21 12:00 ` Kay Sievers
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Alan Jenkins @ 2008-10-21 11:48 UTC (permalink / raw)
To: linux-hotplug
Kay Sievers wrote:
> On Tue, Oct 21, 2008 at 12:11, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>
>> Allocate udev_device->envp lazily
>>
>> It's a pity to allocate 128 words when we don't need them.
>> Measured 2% _user_ cpu time reduction on EeePC coldplug.
>>
>
> Any idea what might take 2% here? We don't do anything if the envp
> isn't used. We do almost the same thing with or without the patch
> besides that "struct udev_device" is 1016 bytes smaller now, and we
> malloc() it.
>
> Kay
>
oprofile said malloc() went down by about that much. I measured on top
of threaded udevd, so I might be unlucky and chasing overheads you don't
see in the single-threaded implementation :).
By rights it should reduce memset() (or calloc now). And it might help
with caching, because the array effectively splits udev_device in two.
Using memory more compactly can reduce page faults, but I don't think
that was significant.
Alan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libudev: Allocate udev_device->envp lazily
2008-10-21 10:11 [PATCH] libudev: Allocate udev_device->envp lazily Alan Jenkins
2008-10-21 11:07 ` Kay Sievers
2008-10-21 11:48 ` Alan Jenkins
@ 2008-10-21 12:00 ` Kay Sievers
2008-10-21 13:00 ` Bob Beers
2008-10-21 13:01 ` Alan Jenkins
4 siblings, 0 replies; 6+ messages in thread
From: Kay Sievers @ 2008-10-21 12:00 UTC (permalink / raw)
To: linux-hotplug
On Tue, Oct 21, 2008 at 13:48, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> Kay Sievers wrote:
>> On Tue, Oct 21, 2008 at 12:11, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>>
>>> Allocate udev_device->envp lazily
>>>
>>> It's a pity to allocate 128 words when we don't need them.
>>> Measured 2% _user_ cpu time reduction on EeePC coldplug.
>>
>> Any idea what might take 2% here? We don't do anything if the envp
>> isn't used. We do almost the same thing with or without the patch
>> besides that "struct udev_device" is 1016 bytes smaller now, and we
>> malloc() it.
> oprofile said malloc() went down by about that much. I measured on top
> of threaded udevd, so I might be unlucky and chasing overheads you don't
> see in the single-threaded implementation :).
>
> By rights it should reduce memset() (or calloc now). And it might help
> with caching, because the array effectively splits udev_device in two.
Applied.
Thanks,
Kay
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libudev: Allocate udev_device->envp lazily
2008-10-21 10:11 [PATCH] libudev: Allocate udev_device->envp lazily Alan Jenkins
` (2 preceding siblings ...)
2008-10-21 12:00 ` Kay Sievers
@ 2008-10-21 13:00 ` Bob Beers
2008-10-21 13:01 ` Alan Jenkins
4 siblings, 0 replies; 6+ messages in thread
From: Bob Beers @ 2008-10-21 13:00 UTC (permalink / raw)
To: linux-hotplug
one small question ...
On Tue, Oct 21, 2008 at 6:11 AM, Alan Jenkins
<alan-jenkins@tuffmail.co.uk> wrote:
> Allocate udev_device->envp lazily
>
> It's a pity to allocate 128 words when we don't need them.
> Measured 2% _user_ cpu time reduction on EeePC coldplug.
>
> Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
>
> diff --git a/udev/lib/libudev-device.c b/udev/lib/libudev-device.c
> index b54c727..69ab1f7 100644
> --- a/udev/lib/libudev-device.c
> +++ b/udev/lib/libudev-device.c
> @@ -31,6 +31,8 @@
> #include "libudev.h"
> #include "libudev-private.h"
>
> +#define ENVP_SIZE 128
> +
> struct udev_device {
> int refcount;
> struct udev *udev;
> @@ -46,7 +48,7 @@ struct udev_device {
> struct udev_list_node devlinks_list;
> int devlinks_uptodate;
> struct udev_list_node properties_list;
> - char *envp[128];
> + char **envp;
> int envp_uptodate;
> char *driver;
> int driver_set;
> @@ -611,8 +613,11 @@ void udev_device_unref(struct udev_device *udev_device)
> free(udev_device->devpath_old);
> free(udev_device->physdevpath);
> udev_list_cleanup_entries(udev_device->udev, &udev_device->sysattr_list);
> - for (i = 0; i < ARRAY_SIZE(udev_device->envp) && udev_device->envp[i] != NULL; i++)
> - free(udev_device->envp[i]);
> + if (udev_device->envp != NULL) {
> + for (i = 0; i < ENVP_SIZE && udev_device->envp[i] != NULL; i++)
> + free(udev_device->envp[i]);
> + free(udev_device->envp);
> + }
> info(udev_device->udev, "udev_device: %p released\n", udev_device);
> free(udev_device);
> }
> @@ -1014,14 +1019,18 @@ char **udev_device_get_properties_envp(struct udev_device *udev_device)
> unsigned int i;
> struct udev_list_entry *list_entry;
>
> - for (i = 0; i < ARRAY_SIZE(udev_device->envp) && udev_device->envp[i] != NULL; i++)
> - free(udev_device->envp[i]);
> + if (udev_device->envp) {
> + for (i = 0; i < 128 && udev_device->envp[i] != NULL; i++)
should this 128 also be ENVP_SIZE?
> + free(udev_device->envp[i]);
> + } else
> + udev_device->envp = malloc(sizeof(char *) * ENVP_SIZE);
> +
> i = 0;
> udev_list_entry_foreach(list_entry, udev_device_get_properties_list_entry(udev_device)) {
> asprintf(&udev_device->envp[i++], "%s=%s",
> udev_list_entry_get_name(list_entry),
> udev_list_entry_get_value(list_entry));
> - if (i+1 >= ARRAY_SIZE(udev_device->envp))
> + if (i+1 >= ENVP_SIZE)
> break;
> }
> udev_device->envp[i] = NULL;
>
>
-Bob
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libudev: Allocate udev_device->envp lazily
2008-10-21 10:11 [PATCH] libudev: Allocate udev_device->envp lazily Alan Jenkins
` (3 preceding siblings ...)
2008-10-21 13:00 ` Bob Beers
@ 2008-10-21 13:01 ` Alan Jenkins
4 siblings, 0 replies; 6+ messages in thread
From: Alan Jenkins @ 2008-10-21 13:01 UTC (permalink / raw)
To: linux-hotplug
Bob Beers wrote:
> one small question ...
>
> On Tue, Oct 21, 2008 at 6:11 AM, Alan Jenkins
> <alan-jenkins@tuffmail.co.uk> wrote:
>
>> Allocate udev_device->envp lazily
>>
>> It's a pity to allocate 128 words when we don't need them.
>> Measured 2% _user_ cpu time reduction on EeePC coldplug.
>>
>> Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
>>
>> diff --git a/udev/lib/libudev-device.c b/udev/lib/libudev-device.c
>> index b54c727..69ab1f7 100644
>> --- a/udev/lib/libudev-device.c
>> +++ b/udev/lib/libudev-device.c
>> @@ -31,6 +31,8 @@
>> #include "libudev.h"
>> #include "libudev-private.h"
>>
>> +#define ENVP_SIZE 128
>> +
>> struct udev_device {
>> int refcount;
>> struct udev *udev;
>> @@ -46,7 +48,7 @@ struct udev_device {
>> struct udev_list_node devlinks_list;
>> int devlinks_uptodate;
>> struct udev_list_node properties_list;
>> - char *envp[128];
>> + char **envp;
>> int envp_uptodate;
>> char *driver;
>> int driver_set;
>> @@ -611,8 +613,11 @@ void udev_device_unref(struct udev_device *udev_device)
>> free(udev_device->devpath_old);
>> free(udev_device->physdevpath);
>> udev_list_cleanup_entries(udev_device->udev, &udev_device->sysattr_list);
>> - for (i = 0; i < ARRAY_SIZE(udev_device->envp) && udev_device->envp[i] != NULL; i++)
>> - free(udev_device->envp[i]);
>> + if (udev_device->envp != NULL) {
>> + for (i = 0; i < ENVP_SIZE && udev_device->envp[i] != NULL; i++)
>> + free(udev_device->envp[i]);
>> + free(udev_device->envp);
>> + }
>> info(udev_device->udev, "udev_device: %p released\n", udev_device);
>> free(udev_device);
>> }
>> @@ -1014,14 +1019,18 @@ char **udev_device_get_properties_envp(struct udev_device *udev_device)
>> unsigned int i;
>> struct udev_list_entry *list_entry;
>>
>> - for (i = 0; i < ARRAY_SIZE(udev_device->envp) && udev_device->envp[i] != NULL; i++)
>> - free(udev_device->envp[i]);
>> + if (udev_device->envp) {
>> + for (i = 0; i < 128 && udev_device->envp[i] != NULL; i++)
>>
>
> should this 128 also be ENVP_SIZE?
>
Yes, sorry. I see Kay fixed it already though :)
Alan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-10-21 13:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-10-21 10:11 [PATCH] libudev: Allocate udev_device->envp lazily Alan Jenkins
2008-10-21 11:07 ` Kay Sievers
2008-10-21 11:48 ` Alan Jenkins
2008-10-21 12:00 ` Kay Sievers
2008-10-21 13:00 ` Bob Beers
2008-10-21 13:01 ` Alan Jenkins
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).