[lm-sensors] [Patch] libsensors: Ignore directories and symlinks in

[lm-sensors] [Patch] libsensors: Ignore directories and symlinks in

[Andre Prendel]

Sysfs directory of the hwmon devices contains directories and symlinks.

Here is the output on my Thinkpad:

  andre@ubuntu:/sys/class/hwmon/hwmon0/device$ ls -l
  insgesamt 0
  lrwxrwxrwx 1 root root    0 2009-02-04 19:44 bus -> ../../../bus/platform
  lrwxrwxrwx 1 root root    0 2009-02-04 19:42 driver ->
  ../../../bus/platform/drivers/thinkpad_hwmon 
  -r--r--r-- 1 root root 4096 2009-02-04 19:44 fan1_input
  lrwxrwxrwx 1 root root    0 2009-02-04 19:44 hwmon:hwmon0 ->
  ../../../class/hwmon/hwmon0 
  -r--r--r-- 1 root root 4096 2009-02-04 19:44 modalias
  -r--r--r-- 1 root root 4096 2009-02-04 19:44 name
  drwxr-xr-x 2 root root    0 2009-02-04 19:44 power
  -rw-r--r-- 1 root root 4096 2009-02-04 19:44 pwm1
  -rw-r--r-- 1 root root 4096 2009-02-04 19:44 pwm1_enable
  lrwxrwxrwx 1 root root    0 2009-02-04 19:42 subsystem ->
  ../../../bus/platform 
  -r--r--r-- 1 root root 4096 2009-02-04 19:44 temp10_input
  -r--r--r-- 1 root root 4096 2009-02-04 19:44 temp11_input
  -r--r--r-- 1 root root 4096 2009-02-04 19:44 temp12_input
  [...]

IMO we can ignore them looking for subfeatures (in
sensors_read_dynamic_chip()), can't we?

I assume there are no hidden files in the directory, right? That's the
reason why I have removed this from condition.

Any comments or improvements?

---
--- lm-sensors-dev/lib/sysfs.c	2009-01-26 17:43:43.000000000 +0100
+++ my-sensors-dev/lib/sysfs.c	2009-02-04 21:59:44.000000000 +0100
@@ -360,7 +360,8 @@ static int sensors_read_dynamic_chip(sen
 		char *name = ent->d_name;
 		int nr;
 
-		if (ent->d_name[0] = '.')
+		/* Skip directories and symlinks.  */
+		if (ent->d_type = DT_DIR || ent->d_type = DT_LNK)
 			continue;
 
 		sftype = sensors_subfeature_get_type(name, &nr);

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch] libsensors: Ignore directories and

[Jean Delvare]

Hi Andre,

On Thu, 5 Feb 2009 10:03:31 +0100, Andre Prendel wrote:
> Sysfs directory of the hwmon devices contains directories and symlinks.
> 
> Here is the output on my Thinkpad:
> 
>   andre@ubuntu:/sys/class/hwmon/hwmon0/device$ ls -l
>   insgesamt 0
>   lrwxrwxrwx 1 root root    0 2009-02-04 19:44 bus -> ../../../bus/platform
>   lrwxrwxrwx 1 root root    0 2009-02-04 19:42 driver ->
>   ../../../bus/platform/drivers/thinkpad_hwmon 
>   -r--r--r-- 1 root root 4096 2009-02-04 19:44 fan1_input
>   lrwxrwxrwx 1 root root    0 2009-02-04 19:44 hwmon:hwmon0 ->
>   ../../../class/hwmon/hwmon0 
>   -r--r--r-- 1 root root 4096 2009-02-04 19:44 modalias
>   -r--r--r-- 1 root root 4096 2009-02-04 19:44 name
>   drwxr-xr-x 2 root root    0 2009-02-04 19:44 power
>   -rw-r--r-- 1 root root 4096 2009-02-04 19:44 pwm1
>   -rw-r--r-- 1 root root 4096 2009-02-04 19:44 pwm1_enable
>   lrwxrwxrwx 1 root root    0 2009-02-04 19:42 subsystem ->
>   ../../../bus/platform 
>   -r--r--r-- 1 root root 4096 2009-02-04 19:44 temp10_input
>   -r--r--r-- 1 root root 4096 2009-02-04 19:44 temp11_input
>   -r--r--r-- 1 root root 4096 2009-02-04 19:44 temp12_input
>   [...]
> 
> IMO we can ignore them looking for subfeatures (in
> sensors_read_dynamic_chip()), can't we?

What problem are you trying to solve?

> I assume there are no hidden files in the directory, right? That's the
> reason why I have removed this from condition.

I've never seen hidden files in sysfs except for sections
in /sys/module. But the purpose of the original code was merely to skip
"." and ".." anyway, not hidden files.

> 
> Any comments or improvements?
> 
> ---
> --- lm-sensors-dev/lib/sysfs.c	2009-01-26 17:43:43.000000000 +0100
> +++ my-sensors-dev/lib/sysfs.c	2009-02-04 21:59:44.000000000 +0100
> @@ -360,7 +360,8 @@ static int sensors_read_dynamic_chip(sen
>  		char *name = ent->d_name;
>  		int nr;
>  
> -		if (ent->d_name[0] = '.')
> +		/* Skip directories and symlinks.  */
> +		if (ent->d_type = DT_DIR || ent->d_type = DT_LNK)
>  			continue;
>  
>  		sftype = sensors_subfeature_get_type(name, &nr);

Links must always be handled with care, sometimes sysfs turns
directories into links or vice-versa and it has broken user-space code
in the past. But I guess we can safely assume that device attributes
will never be links?

If you really care about performance (and I guess that's the sole
purpose of your patch) then it's faster to check for ent->d_type !DT_REG. You can also move "name = ent->d_name" after this test to make
the fast path even faster.

Out of curiosity, did you actually measure any performance improvement
with your patch?

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch] libsensors: Ignore directories and

[Andre Prendel]

On Thu, Feb 05, 2009 at 10:53:34AM +0100, Jean Delvare wrote:
> Hi Andre,

Hello Jean,
 
> On Thu, 5 Feb 2009 10:03:31 +0100, Andre Prendel wrote:
> > Sysfs directory of the hwmon devices contains directories and symlinks.
> > 
> > Here is the output on my Thinkpad:
> > 
> >   andre@ubuntu:/sys/class/hwmon/hwmon0/device$ ls -l
> >   insgesamt 0
> >   lrwxrwxrwx 1 root root    0 2009-02-04 19:44 bus -> ../../../bus/platform
> >   lrwxrwxrwx 1 root root    0 2009-02-04 19:42 driver ->
> >   ../../../bus/platform/drivers/thinkpad_hwmon 
> >   -r--r--r-- 1 root root 4096 2009-02-04 19:44 fan1_input
> >   lrwxrwxrwx 1 root root    0 2009-02-04 19:44 hwmon:hwmon0 ->
> >   ../../../class/hwmon/hwmon0 
> >   -r--r--r-- 1 root root 4096 2009-02-04 19:44 modalias
> >   -r--r--r-- 1 root root 4096 2009-02-04 19:44 name
> >   drwxr-xr-x 2 root root    0 2009-02-04 19:44 power
> >   -rw-r--r-- 1 root root 4096 2009-02-04 19:44 pwm1
> >   -rw-r--r-- 1 root root 4096 2009-02-04 19:44 pwm1_enable
> >   lrwxrwxrwx 1 root root    0 2009-02-04 19:42 subsystem ->
> >   ../../../bus/platform 
> >   -r--r--r-- 1 root root 4096 2009-02-04 19:44 temp10_input
> >   -r--r--r-- 1 root root 4096 2009-02-04 19:44 temp11_input
> >   -r--r--r-- 1 root root 4096 2009-02-04 19:44 temp12_input
> >   [...]
> > 
> > IMO we can ignore them looking for subfeatures (in
> > sensors_read_dynamic_chip()), can't we?
> 
> What problem are you trying to solve?

Not a problem. It's just a little improvement.
> 
> > I assume there are no hidden files in the directory, right? That's the
> > reason why I have removed this from condition.
> 
> I've never seen hidden files in sysfs except for sections
> in /sys/module. But the purpose of the original code was merely to skip
> "." and ".." anyway, not hidden files.

At another place (sysfs_foreach_classdev()) there is a comment
explaining the condition.

[...]
      if (ent->d_name[0] = '.')      /* skip hidden entries */
                        continue;
[...]

> > 
> > Any comments or improvements?
> > 
> > ---
> > --- lm-sensors-dev/lib/sysfs.c	2009-01-26 17:43:43.000000000 +0100
> > +++ my-sensors-dev/lib/sysfs.c	2009-02-04 21:59:44.000000000 +0100
> > @@ -360,7 +360,8 @@ static int sensors_read_dynamic_chip(sen
> >  		char *name = ent->d_name;
> >  		int nr;
> >  
> > -		if (ent->d_name[0] = '.')
> > +		/* Skip directories and symlinks.  */
> > +		if (ent->d_type = DT_DIR || ent->d_type = DT_LNK)
> >  			continue;
> >  
> >  		sftype = sensors_subfeature_get_type(name, &nr);
> 
> Links must always be handled with care, sometimes sysfs turns
> directories into links or vice-versa and it has broken user-space code
> in the past. But I guess we can safely assume that device attributes
> will never be links?
> 
> If you really care about performance (and I guess that's the sole
> purpose of your patch) then it's faster to check for ent->d_type !> DT_REG.

Indeed. That's even better.

> You can also move "name = ent->d_name" after this test to make
> the fast path even faster.

This should just avoid needless work and maybe potential errors in
parsing unexpected directory names.

> Out of curiosity, did you actually measure any performance improvement
> with your patch?

I didn't do a measurement. That wasn't the goal.

P.S. To become familiar with the whole code and getting a better
understanding how things work, I walk through the code. And if I found
something that I would do in a different way, I send a patch to you :)

Andre
> -- 
> Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch] libsensors: Ignore directories and

[Jean Delvare]

Hi Andre,

On Thu, 5 Feb 2009 11:45:36 +0100, Andre Prendel wrote:
> On Thu, Feb 05, 2009 at 10:53:34AM +0100, Jean Delvare wrote:
> > On Thu, 5 Feb 2009 10:03:31 +0100, Andre Prendel wrote:
> > > I assume there are no hidden files in the directory, right? That's the
> > > reason why I have removed this from condition.
> > 
> > I've never seen hidden files in sysfs except for sections
> > in /sys/module. But the purpose of the original code was merely to skip
> > "." and ".." anyway, not hidden files.
> 
> At another place (sysfs_foreach_classdev()) there is a comment
> explaining the condition.
> 
> [...]
>       if (ent->d_name[0] = '.')      /* skip hidden entries */
>                         continue;
> [...]

Indeed, but as I recall, the hidden entries we were worried about
back then were just "." and "..". I don't expect it to make any
difference anyway, as I doubt we'll ever have other hidden files or
directories in class directories, nor do I expect non-directory (or
link to directory) entries there.

> > (...)
> > If you really care about performance (and I guess that's the sole
> > purpose of your patch) then it's faster to check for ent->d_type !> > DT_REG.
> 
> Indeed. That's even better.
> 
> > You can also move "name = ent->d_name" after this test to make
> > the fast path even faster.
> 
> This should just avoid needless work and maybe potential errors in
> parsing unexpected directory names.

I get the idea, and second it.

> > Out of curiosity, did you actually measure any performance improvement
> > with your patch?
> 
> I didn't do a measurement. That wasn't the goal.

You might want to ask valgrind about it (I certainly will, being
curious.)

> P.S. To become familiar with the whole code and getting a better
> understanding how things work, I walk through the code. And if I found
> something that I would do in a different way, I send a patch to you :)

Sounds like a good plan, don't stop :)

Care to send an updated patch? I'll be happy to apply it.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch] libsensors: Ignore directories and

[Andre Prendel]

On Thu, Feb 05, 2009 at 02:31:40PM +0100, Jean Delvare wrote:
> Hi Andre,
> 
> On Thu, 5 Feb 2009 11:45:36 +0100, Andre Prendel wrote:
> > On Thu, Feb 05, 2009 at 10:53:34AM +0100, Jean Delvare wrote:
> > > On Thu, 5 Feb 2009 10:03:31 +0100, Andre Prendel wrote:
> > > > I assume there are no hidden files in the directory, right? That's the
> > > > reason why I have removed this from condition.
> > > 
> > > I've never seen hidden files in sysfs except for sections
> > > in /sys/module. But the purpose of the original code was merely to skip
> > > "." and ".." anyway, not hidden files.
> > 
> > At another place (sysfs_foreach_classdev()) there is a comment
> > explaining the condition.
> > 
> > [...]
> >       if (ent->d_name[0] = '.')      /* skip hidden entries */
> >                         continue;
> > [...]
> 
> Indeed, but as I recall, the hidden entries we were worried about
> back then were just "." and "..". I don't expect it to make any
> difference anyway, as I doubt we'll ever have other hidden files or
> directories in class directories, nor do I expect non-directory (or
> link to directory) entries there.
> 
> > > (...)
> > > If you really care about performance (and I guess that's the sole
> > > purpose of your patch) then it's faster to check for ent->d_type !> > > DT_REG.
> > 
> > Indeed. That's even better.
> > 
> > > You can also move "name = ent->d_name" after this test to make
> > > the fast path even faster.
> > 
> > This should just avoid needless work and maybe potential errors in
> > parsing unexpected directory names.
> 
> I get the idea, and second it.
> 
> > > Out of curiosity, did you actually measure any performance improvement
> > > with your patch?
> > 
> > I didn't do a measurement. That wasn't the goal.
> 
> You might want to ask valgrind about it (I certainly will, being
> curious.)

So I will take a closer look at valgrind. I have used only the
memcheck tool so far. After that you will get an updated patch.
> 
> > P.S. To become familiar with the whole code and getting a better
> > understanding how things work, I walk through the code. And if I found
> > something that I would do in a different way, I send a patch to you :)
> 
> Sounds like a good plan, don't stop :)

I won't stop. :)

> Care to send an updated patch? I'll be happy to apply it.

Thanks
Andre

> -- 
> Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch] libsensors: Ignore directories and

[Jean Delvare]

On Thu, 5 Feb 2009 15:20:17 +0100, Andre Prendel wrote:
> So I will take a closer look at valgrind. I have used only the
> memcheck tool so far. After that you will get an updated patch.

The memcheck tool is very nice indeed. But in this case the tool I had
in mind is callgrind (with kcachegrind to look at the results.)

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch] libsensors: Ignore directories and

[Andre Prendel]

On Thu, Feb 05, 2009 at 03:32:22PM +0100, Jean Delvare wrote:
> On Thu, 5 Feb 2009 15:20:17 +0100, Andre Prendel wrote:
> > So I will take a closer look at valgrind. I have used only the
> > memcheck tool so far. After that you will get an updated patch.
> 
> The memcheck tool is very nice indeed. But in this case the tool I had
> in mind is callgrind (with kcachegrind to look at the results.)

Hey,

running sensors under callgrind with (1.) and without (2.) the patch
below, gives me the following result (callgrind_annotate).

1.)

 [...]
 42,709  ???:sensors_read_one_sysfs_chip [/usr/local/lib/libsensors.so.4.0.2]
 ...
 1,925  ???:vsscanf [/lib/tls/i686/cmov/libc-2.8.90.so]
 935  ???:sscanf [/lib/tls/i686/cmov/libc-2.8.90.so]
 [...]

2.)

 [...]
 43,272  ???:sensors_read_one_sysfs_chip [/usr/local/lib/libsensors.so.4.0.2]
 ...
 3,080  ???:vsscanf [/lib/tls/i686/cmov/libc-2.8.90.so]
 1,496  ???:sscanf [/lib/tls/i686/cmov/libc-2.8.90.so]
 [...]

I don't know what the numbers exactly mean. Could you explain please?

In real life of course you can't feel any speed
improvements. Nevertheless, IMO doing useless work (parsing directories
and symlinks) doesn't make sense.

Andre

> -- 
> Jean Delvare

---
--- lm-sensors-dev/lib/sysfs.c	2009-02-05 22:02:25.000000000 +0100
+++ my-sensors-dev/lib/sysfs.c	2009-02-05 22:21:09.000000000 +0100
@@ -357,12 +357,15 @@ static int sensors_read_dynamic_chip(sen
 		sensors_fatal_error(__func__, "Out of memory");
 
 	while ((ent = readdir(dir))) {
-		char *name = ent->d_name;
+		char *name;
 		int nr;
 
-		if (ent->d_name[0] = '.')
+		/* Skip directories and symlinks.  */
+		if (ent->d_type != DT_REG)
 			continue;
 
+		name = ent->d_name;
+
 		sftype = sensors_subfeature_get_type(name, &nr);
 		if (sftype = SENSORS_SUBFEATURE_UNKNOWN)
 			continue;

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [Patch] libsensors: Ignore directories and

[Jean Delvare]

Hi Andre,

On Fri, 6 Feb 2009 09:43:35 +0100, Andre Prendel wrote:
> On Thu, Feb 05, 2009 at 03:32:22PM +0100, Jean Delvare wrote:
> > On Thu, 5 Feb 2009 15:20:17 +0100, Andre Prendel wrote:
> > > So I will take a closer look at valgrind. I have used only the
> > > memcheck tool so far. After that you will get an updated patch.
> > 
> > The memcheck tool is very nice indeed. But in this case the tool I had
> > in mind is callgrind (with kcachegrind to look at the results.)
> 
> running sensors under callgrind with (1.) and without (2.) the patch
> below, gives me the following result (callgrind_annotate).

I didn't know about callgrind_annotate, thanks for the info!

> 1.)
> 
>  [...]
>  42,709  ???:sensors_read_one_sysfs_chip [/usr/local/lib/libsensors.so.4.0.2]
>  ...
>  1,925  ???:vsscanf [/lib/tls/i686/cmov/libc-2.8.90.so]
>  935  ???:sscanf [/lib/tls/i686/cmov/libc-2.8.90.so]
>  [...]
> 
> 2.)
> 
>  [...]
>  43,272  ???:sensors_read_one_sysfs_chip [/usr/local/lib/libsensors.so.4.0.2]
>  ...
>  3,080  ???:vsscanf [/lib/tls/i686/cmov/libc-2.8.90.so]
>  1,496  ???:sscanf [/lib/tls/i686/cmov/libc-2.8.90.so]
>  [...]
> 
> I don't know what the numbers exactly mean. Could you explain please?

As far as I know this is an (estimated?) number of CPU cycles spent in
these functions. To get a better overview you might want to try the
--inclusive=yes option of callgrind_annotate. For me it results in:

-  381,917  ???:sensors_read_one_sysfs_chip [/usr/local/lib/libsensors.so.4.0.2]
+  371,463  ???:sensors_read_one_sysfs_chip [/usr/local/lib/libsensors.so.4.0.2]

That's for the typical case, with a hwmon device which has only 4
non-file entries to be skipped (driver, hwmon, power and subsystem).

This is a 2.7% speedup for this function, which is pretty good given
how cheap it was to implement.

> In real life of course you can't feel any speed
> improvements. Nevertheless, IMO doing useless work (parsing directories
> and symlinks) doesn't make sense.

I agree. Most speedups are not visible, but accumulated speedups, first
at the library or application level, then at the system level, can make
a big difference in the end, so they are worth the work.

> --- lm-sensors-dev/lib/sysfs.c	2009-02-05 22:02:25.000000000 +0100
> +++ my-sensors-dev/lib/sysfs.c	2009-02-05 22:21:09.000000000 +0100
> @@ -357,12 +357,15 @@ static int sensors_read_dynamic_chip(sen
>  		sensors_fatal_error(__func__, "Out of memory");
>  
>  	while ((ent = readdir(dir))) {
> -		char *name = ent->d_name;
> +		char *name;
>  		int nr;
>  
> -		if (ent->d_name[0] = '.')
> +		/* Skip directories and symlinks.  */
> +		if (ent->d_type != DT_REG)
>  			continue;
>  
> +		name = ent->d_name;
> +
>  		sftype = sensors_subfeature_get_type(name, &nr);
>  		if (sftype = SENSORS_SUBFEATURE_UNKNOWN)
>  			continue;

I'll apply this right now, thanks.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors