All of lore.kernel.org
 help / color / mirror / Atom feed
* [lm-sensors]  Memory usage of sysfs files
@ 2006-03-08 22:36 Hartmut Rick
  2006-03-09 19:41 ` Jean Delvare
                   ` (9 more replies)
  0 siblings, 10 replies; 11+ messages in thread
From: Hartmut Rick @ 2006-03-08 22:36 UTC (permalink / raw)
  To: lm-sensors



Hi everybody,

On Wed, 8 Mar 2006, Jean Delvare wrote:

> The only data I am missing now is the memory used by each additional
> sysfs file we create. We need to know, as Hans objected that too many
> sysfs files could have a negative impact on memory consumption. I dug

I've just created a couple sysfs files in order to find out, and the 
conclusion is that each additional sysfs file reduces the remaining free 
memory by something on the order of 90-95 bytes.
It turns out that the memory reported by 'free' varies quite a bit between 
calls, which disturbs somewhat the precision of this measurement. I had to 
create 10000 sysfs files in order to get a statistically significant 
result. Now this result is correct on the 10% level, which I think is good 
enough for our discussion.
For this experiment, I used the same callback function for all sysfs 
files, so each additional file has just one SENSOR_DEVICE_ATTR struct 
definition and a device_create_file() call, but no extra supporting code.
This is the typical situation for the individual alarm files, I guess.

With current DRAM prices, 100 extra bytes correspond to extra costs of 
around 1e-5 EUR or USD for each additional file, which I think is 
tolerable.

Best regards,
  Hartmut


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [lm-sensors] Memory usage of sysfs files
  2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
@ 2006-03-09 19:41 ` Jean Delvare
  2006-03-09 20:05 ` Hans de Goede
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2006-03-09 19:41 UTC (permalink / raw)
  To: lm-sensors

Hi Hartmut,

> I've just created a couple sysfs files in order to find out, and the 
> conclusion is that each additional sysfs file reduces the remaining free 
> memory by something on the order of 90-95 bytes.
> It turns out that the memory reported by 'free' varies quite a bit between 
> calls, which disturbs somewhat the precision of this measurement. I had to 
> create 10000 sysfs files in order to get a statistically significant 
> result. Now this result is correct on the 10% level, which I think is good 
> enough for our discussion.
> For this experiment, I used the same callback function for all sysfs 
> files, so each additional file has just one SENSOR_DEVICE_ATTR struct 
> definition and a device_create_file() call, but no extra supporting code.
> This is the typical situation for the individual alarm files, I guess.

Yes, you tested the right thing, thanks for reporting your results.

I have been conducting a similar investigation but on the theoretical
front. It seems that the amount of memory required by each additional
sysfs file (not counting the callbacks and creation) is the size of
stuct sysfs_dirent, plus 4 bytes (not sure why, most probably an
internal slab implementation detail.) This is 40 + 4 = 44 bytes on x86,
72 + 4 = 76 bytes on x86_64.

I don't know why your measurement suggests larger allocations. But even
with your value of 90-95 bytes it's still a rather small value.

> With current DRAM prices, 100 extra bytes correspond to extra costs of 
> around 1e-5 EUR or USD for each additional file, which I think is 
> tolerable.

I don't think this is a matter of price, but more of a matter of
consumed memory on already designed/deployed systems, especially on
embedded systems. Other systems typically have over 3000 sysfs files,
so a dozen more or less really doesn't change anything.

If we consider the w83627hf driver as an example (being one of the
drivers with the higher number of alarm+beep files in my individual
files model) the additional memory consumption is (on x86) 44 * 33 1452 bytes. This really isn't much, even for an embedded system. Such
systems will typically have between 600 and 1600 sysfs files already,
33 more or less (worse case) still aren't that much.

Last, here are the figures I promised some days ago:

Before my individual alarm files patches:

   text    data     bss     dec     hex filename
   6101    1344       4    7449    1d19 drivers/hwmon/f71805f.o
   4848    1052      16    5916    171c drivers/hwmon/lm63.o
   5861    1892      36    7789    1e6d drivers/hwmon/lm90.o
  15024    1356      16   16396    400c drivers/hwmon/w83627hf.o

-rw-r--r--  1 khali users 24129 2006-03-09 18:52 drivers/hwmon/f71805f.c
-rw-r--r--  1 khali users 18909 2006-03-09 15:23 drivers/hwmon/lm63.c
-rw-r--r--  1 khali users 22806 2006-03-09 15:23 drivers/hwmon/lm90.c
-rw-r--r--  1 khali users 45891 2006-03-09 15:23 drivers/hwmon/w83627hf.c

After the patches:

   text    data     bss     dec     hex filename
   6357    1696       4    8057    1f79 drivers/hwmon/f71805f.o
   5122    1196      16    6334    18be drivers/hwmon/lm63.o
   6185    2060      36    8281    2059 drivers/hwmon/lm90.o
  16126    2128      16   18270    475e drivers/hwmon/w83627hf.o

-rw-r--r--  1 khali users 25316 2006-03-09 19:04 drivers/hwmon/f71805f.c
-rw-r--r--  1 khali users 20174 2006-03-09 19:04 drivers/hwmon/lm63.c
-rw-r--r--  1 khali users 24235 2006-03-09 19:04 drivers/hwmon/lm90.c
-rw-r--r--  1 khali users 50068 2006-03-09 19:18 drivers/hwmon/w83627hf.c

Summary:

f71805f:   +608 binary ( +8.1%), +1187 source ( +4.9%)
lm63:      +418 binary ( +7.0%), +1265 source ( +6.6%)
lm90:      +492 binary ( +6.3%), +1429 source ( +6.2%)
w83627hf: +1874 binary (+11.4%), +4177 source ( +9.1%)

So the size increase is admittedly not neglectable and greater than I
would like, but it isn't (IMHO at least) unaccpeptable either,
especially when hardware monitoring drivers are not amongst the largest
drivers in the first place. Also note that the number of individual
alarm files depends on the number of channels and/or limits the device
has, so this approach won't suddenly turn a small driver into a large
one. The size increment had to be somehow proportional to the original
driver size.

If anyone has similar figures for Hans de Goede's proposal, for
comparison, I'll be happy to take a look.

Thanks,
-- 
Jean Delvare


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [lm-sensors] Memory usage of sysfs files
  2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
  2006-03-09 19:41 ` Jean Delvare
@ 2006-03-09 20:05 ` Hans de Goede
  2006-03-09 20:06 ` Hans de Goede
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Hans de Goede @ 2006-03-09 20:05 UTC (permalink / raw)
  To: lm-sensors

Jean Delvare wrote:
> Hi Hartmut,
> 
>> I've just created a couple sysfs files in order to find out, and the 
>> conclusion is that each additional sysfs file reduces the remaining free 
>> memory by something on the order of 90-95 bytes.
>> It turns out that the memory reported by 'free' varies quite a bit between 
>> calls, which disturbs somewhat the precision of this measurement. I had to 
>> create 10000 sysfs files in order to get a statistically significant 
>> result. Now this result is correct on the 10% level, which I think is good 
>> enough for our discussion.
>> For this experiment, I used the same callback function for all sysfs 
>> files, so each additional file has just one SENSOR_DEVICE_ATTR struct 
>> definition and a device_create_file() call, but no extra supporting code.
>> This is the typical situation for the individual alarm files, I guess.
> 
> Yes, you tested the right thing, thanks for reporting your results.
> 
> I have been conducting a similar investigation but on the theoretical
> front. It seems that the amount of memory required by each additional
> sysfs file (not counting the callbacks and creation) is the size of
> stuct sysfs_dirent, plus 4 bytes (not sure why, most probably an
> internal slab implementation detail.) This is 40 + 4 = 44 bytes on x86,
> 72 + 4 = 76 bytes on x86_64.
> 
 > I don't know why your measurement suggests larger allocations. But even
 > with your value of 90-95 bytes it's still a rather small value.
 >

AFAIK each sysfs file has a inode behind it which has been fixated in 
the inode-cache. This is also one of the more worry some parts IMHO, if 
we completly fill the inode-cache (which can be configured to be quite 
small on embedded systems) this will cause a performace impact. This is 
all AFAIK / IMHO.

> Also note that the number of individual
> alarm files depends on the number of channels and/or limits the device
> has, so this approach won't suddenly turn a small driver into a large
> one. The size increment had to be somehow proportional to the original
> driver size.
> 

Well, the uguru driver would have 100 alarm (mask/beep/shutdown) files 
on my mb instead of the current 14!
(this number could vary depending on the exact config of the uguru on a mb)


> If anyone has similar figures for Hans de Goede's proposal, for
> comparison, I'll be happy to take a look.
> 


To get an idea of the extra code size for the mask creation I've removed 
the loops doing the masking in the uguru driver and replaced them with a 
simple function which gets/sets values directly from the data using 
attr->nr and/or attr->index .

Results:
With loops:
[hans at shalem uguru]$ ls -l abituguru.o abituguru.ko
-rw-rw-r-- 1 hans hans 232871 Mar  8 15:14 abituguru.ko
-rw-rw-r-- 1 hans hans 154472 Mar  8 15:14 abituguru.o

Without loops:
[hans at shalem uguru]$ ls -l abituguru.o abituguru.ko
-rw-rw-r-- 1 hans hans 231191 Mar  9 21:12 abituguru.ko
-rw-rw-r-- 1 hans hans 152816 Mar  9 21:11 abituguru.o

This would also spare one array with the sensor masks in show to user 
order a 16 bytes which is malloced.

So this saves around 1.8k code and would cost around 9k malloced (+ 
possible fragmented page size loss) data.



Also think about the number of extra file handles and / or open/closes a 
userspace program would need to have / do with this interface.

Regards,

Hans



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [lm-sensors] Memory usage of sysfs files
  2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
  2006-03-09 19:41 ` Jean Delvare
  2006-03-09 20:05 ` Hans de Goede
@ 2006-03-09 20:06 ` Hans de Goede
  2006-03-09 21:05 ` Jean Delvare
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Hans de Goede @ 2006-03-09 20:06 UTC (permalink / raw)
  To: lm-sensors

p.s.

I'm on x86_64 so the code would be even smaller on i386

Regards,

Hans




^ permalink raw reply	[flat|nested] 11+ messages in thread

* [lm-sensors] Memory usage of sysfs files
  2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
                   ` (2 preceding siblings ...)
  2006-03-09 20:06 ` Hans de Goede
@ 2006-03-09 21:05 ` Jean Delvare
  2006-03-09 21:41 ` Hans de Goede
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2006-03-09 21:05 UTC (permalink / raw)
  To: lm-sensors

Hi Hans,

> AFAIK each sysfs file has a inode behind it which has been fixated in 
> the inode-cache. This is also one of the more worry some parts IMHO, if 
> we completly fill the inode-cache (which can be configured to be quite 
> small on embedded systems) this will cause a performace impact. This is 
> all AFAIK / IMHO.

Well, if a given embedded system wants to support hardware monitoring,
it gets to be configured with such an inode-cache size that it'll work
properly. If not this is a configuration issue, and I don't think we
can take this into account in our design decision.

> > Also note that the number of individual
> > alarm files depends on the number of channels and/or limits the device
> > has, so this approach won't suddenly turn a small driver into a large
> > one. The size increment had to be somehow proportional to the original
> > driver size.
> 
> Well, the uguru driver would have 100 alarm (mask/beep/shutdown) files 
> on my mb instead of the current 14!
> (this number could vary depending on the exact config of the uguru on a mb)

Ah, I guess this explains your reluctance to individual files ;) But the
uguru really is more of an exception than the rule here. For the other
drivers, I'd expect no more than 50 files for the largest ones, and
rather half that value for the vast majority of our "complete hardware
monitoring" drivers. Temperature-only drivers have even less, of course.

How many sysfs files does the uguru driver have, not counting the
alarm/beep/shutdown entries?

> To get an idea of the extra code size for the mask creation I've removed 
> the loops doing the masking in the uguru driver and replaced them with a 
> simple function which gets/sets values directly from the data using 
> attr->nr and/or attr->index .
> 
> Results:
> With loops:
> [hans at shalem uguru]$ ls -l abituguru.o abituguru.ko
> -rw-rw-r-- 1 hans hans 232871 Mar  8 15:14 abituguru.ko
> -rw-rw-r-- 1 hans hans 154472 Mar  8 15:14 abituguru.o
> 
> Without loops:
> [hans at shalem uguru]$ ls -l abituguru.o abituguru.ko
> -rw-rw-r-- 1 hans hans 231191 Mar  9 21:12 abituguru.ko
> -rw-rw-r-- 1 hans hans 152816 Mar  9 21:11 abituguru.o

These numbers are really too high, you must have some debugging option
enabled. Please provide the same numbers without debugging so that the
comparisons make more sense.

> This would also spare one array with the sensor masks in show to user 
> order a 16 bytes which is malloced.

Hm?

> So this saves around 1.8k code and would cost around 9k malloced (+ 
> possible fragmented page size loss) data.

Nothing to be afraid of, if you want my opinion.

> Also think about the number of extra file handles and / or open/closes a 
> userspace program would need to have / do with this interface.

That's right, this is an interesting point, maybe the more serious in
your series of objections. That being said, I wouldn't expect a
user-space program to use several file handles, that's really the kind
of process where you serialize the reads. I'm also not sure how costly
an open/close on a ram-based fs is. Given that there is no seek time
involved, I'd expect it to be quite fast. But if you have some figures
to throw in the battle, they'll be welcome.

I'd really like to see other numbers for your propsal on a driver
different from the uguru, as this one really isn't representative of
the rest of the drivers. If you could pick one (or more) on which I did
my own tests, even better (these are f71805f, lm63, lm90, w83627hf).

Lastly, I have been thinking of another advantage of my proposal over
yours: it makes it possible for user-space to know exactly which
features a given chip has. With your proposal, there is no way for the
user-space to know if a given channel supports alarm or beep until this
alarm or beep is activated; it basically assumes that all channels
support alarm or beep, or none does.

A concrete example can be found in the new smsc47m192 driver: temp2 and
temp3 have a diode fault bit, but temp1 doesn't. Same for temp1 vs
temp2 in the lm63 and lm90 drivers. I guess we can find similar
examples in other drivers. But I would agree that this probably is a
rare case, so it doesn't justify my proposal on its own - it's just
another item on the scales.

Thanks,
-- 
Jean Delvare


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [lm-sensors] Memory usage of sysfs files
  2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
                   ` (3 preceding siblings ...)
  2006-03-09 21:05 ` Jean Delvare
@ 2006-03-09 21:41 ` Hans de Goede
  2006-03-09 23:16 ` Hans de Goede
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Hans de Goede @ 2006-03-09 21:41 UTC (permalink / raw)
  To: lm-sensors

Jean Delvare wrote:
> Hi Hans,
> 
>> Well, the uguru driver would have 100 alarm (mask/beep/shutdown) files 
>> on my mb instead of the current 14!
>> (this number could vary depending on the exact config of the uguru on a mb)
> 
> Ah, I guess this explains your reluctance to individual files ;) But the
> uguru really is more of an exception than the rule here. For the other
> drivers, I'd expect no more than 50 files for the largest ones, and
> rather half that value for the vast majority of our "complete hardware
> monitoring" drivers. Temperature-only drivers have even less, of course.
> 
> How many sysfs files does the uguru driver have, not counting the
> alarm/beep/shutdown entries?
> 
72 without pwm 54

>> To get an idea of the extra code size for the mask creation I've removed 
>> the loops doing the masking in the uguru driver and replaced them with a 
>> simple function which gets/sets values directly from the data using 
>> attr->nr and/or attr->index .
>>
>> Results:
>> With loops:
>> [hans at shalem uguru]$ ls -l abituguru.o abituguru.ko
>> -rw-rw-r-- 1 hans hans 232871 Mar  8 15:14 abituguru.ko
>> -rw-rw-r-- 1 hans hans 154472 Mar  8 15:14 abituguru.o
>>
>> Without loops:
>> [hans at shalem uguru]$ ls -l abituguru.o abituguru.ko
>> -rw-rw-r-- 1 hans hans 231191 Mar  9 21:12 abituguru.ko
>> -rw-rw-r-- 1 hans hans 152816 Mar  9 21:11 abituguru.o
> 
> These numbers are really too high, you must have some debugging option
> enabled. Please provide the same numbers without debugging so that the
> comparisons make more sense.
> 

Erm, how do I do that? Also they might be correct this is x86_64 which 
comes with almost twice as big code and: "wc -l abituguru.c" gives 1473.

Remeber currently I'm building this against a kernel as provided by 
Fedora using the out of tree module building tech 2.6 has (which btw is 
great).

>> This would also spare one array with the sensor masks in show to user 
>> order a 16 bytes which is malloced.
> 
> Hm?
> 

Currently I need an array wich maps a sensor id to an alarm-reg-mask for 
that sensor, this array is 16 bytes big.

>> Also think about the number of extra file handles and / or open/closes a 
>> userspace program would need to have / do with this interface.
> 
> That's right, this is an interesting point, maybe the more serious in
> your series of objections. That being said, I wouldn't expect a
> user-space program to use several file handles, that's really the kind
> of process where you serialize the reads. I'm also not sure how costly
> an open/close on a ram-based fs is. Given that there is no seek time
> involved, I'd expect it to be quite fast. But if you have some figures
> to throw in the battle, they'll be welcome.
> 

Sorry no figures, it just doesn't feel right todo all this opens / close 
if we can pass it all in one file. Also be aware that each open and each 
read and each close will cause a userspace <-> kernel space context 
switch syscalls are expensive. Doing this the lots of files way just 
doesn't feel right for me (I've learned programming on a 8051, so 9k 
additional data is _huge_ to me).

> I'd really like to see other numbers for your propsal on a driver
> different from the uguru, as this one really isn't representative of
> the rest of the drivers. If you could pick one (or more) on which I did
> my own tests, even better (these are f71805f, lm63, lm90, w83627hf).
> 
I'll see what I can do. I've just finised making all the requested 
changes to the uguru driver, but untill this is sorted submitting it 
doesn't make much sense.

> Lastly, I have been thinking of another advantage of my proposal over
> yours: it makes it possible for user-space to know exactly which
> features a given chip has. With your proposal, there is no way for the
> user-space to know if a given channel supports alarm or beep until this
> alarm or beep is activated; it basically assumes that all channels
> support alarm or beep, or none does.
> 
> A concrete example can be found in the new smsc47m192 driver: temp2 and
> temp3 have a diode fault bit, but temp1 doesn't. Same for temp1 vs
> temp2 in the lm63 and lm90 drivers. I guess we can find similar
> examples in other drivers. But I would agree that this probably is a
> rare case, so it doesn't justify my proposal on its own - it's just
> another item on the scales.
> 


Yes, that is a real argument in favor of the one file method.

Regards,

Hans



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [lm-sensors] Memory usage of sysfs files
  2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
                   ` (4 preceding siblings ...)
  2006-03-09 21:41 ` Hans de Goede
@ 2006-03-09 23:16 ` Hans de Goede
  2006-03-10 11:11 ` Hans de Goede
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Hans de Goede @ 2006-03-09 23:16 UTC (permalink / raw)
  To: lm-sensors

Hi,

I've given this some more thought and I've decided against implementing 
my way for a couple of other drivers to be able to really measure the 
difference. I believe we agree my way is somewhat more efficient, but 
that the differences aren't big enough to swing the scales to my way 
only on the efficiency reasons.

Instead of spending more time coding test-cases I'd rather just see a 
decision soon and (if nescessarry) I'll update the uguru driver to match .

There are 2 arguments I would still like to make in favor of my way though:

1) you initially did it this way too for atleast one driver. Remember, 
usually your first hunch is a good one.

2) I've also written support for libsensors and the sensors application 
when doing things my way and that fits nicely. I'm afraid that the many 
files way will be a problem with the current libsensors. I don't want to 
now wath the lib and the progs chips.* will look like. 100+ additional 
entries for the uguru alone! And I don't think the chip specific reading 
and report code in the sensors application will get any pretter too.

Now I know that libsensors is destined to be replaced, but it will be 
around for a while I think.

I've attached a patch adding support for the uguru to both lib and prog 
with alarm reporting to give you an idea how this will look like when 
doing things my way. Please note that this patch needs updating for
2.10 / cvs and thus won't apply.

Regards,

Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lm_sensors-2.9.1-uguru.patch
Type: text/x-patch
Size: 15184 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060310/73c52b90/lm_sensors-2.9.1-uguru-0001.bin

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [lm-sensors] Memory usage of sysfs files
  2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
                   ` (5 preceding siblings ...)
  2006-03-09 23:16 ` Hans de Goede
@ 2006-03-10 11:11 ` Hans de Goede
  2006-03-10 13:46 ` Jean Delvare
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Hans de Goede @ 2006-03-10 11:11 UTC (permalink / raw)
  To: lm-sensors

Jean,

Can you take a decision on this please and then write a bit of 
specification? Then I can make the uGuru driver implement the 
specification and submit it for merging.

What I need for specification if you decide for the one file per sensor 
is the names to use for:
-alarm
-alarm enable/disable
-beep on alarm enable/disable
-shutdown on alarm enable/disable

And also the names for alarm and alarm enable/disable for sensors with 
more then 1 alarm, specifically with a volt low and a volt high alarm.

Thanks & Regards,

Hans


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [lm-sensors] Memory usage of sysfs files
  2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
                   ` (6 preceding siblings ...)
  2006-03-10 11:11 ` Hans de Goede
@ 2006-03-10 13:46 ` Jean Delvare
  2006-03-10 14:09 ` Jean Delvare
  2006-03-10 15:28 ` Jim Cromie
  9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2006-03-10 13:46 UTC (permalink / raw)
  To: lm-sensors

Hi Hans,

> > > With loops:
> > > [hans at shalem uguru]$ ls -l abituguru.o abituguru.ko
> > > -rw-rw-r-- 1 hans hans 232871 Mar  8 15:14 abituguru.ko
> > > -rw-rw-r-- 1 hans hans 154472 Mar  8 15:14 abituguru.o
> > >
> > > Without loops:
> > > [hans at shalem uguru]$ ls -l abituguru.o abituguru.ko
> > > -rw-rw-r-- 1 hans hans 231191 Mar  9 21:12 abituguru.ko
> > > -rw-rw-r-- 1 hans hans 152816 Mar  9 21:11 abituguru.o
> > 
> > These numbers are really too high, you must have some debugging option
> > enabled. Please provide the same numbers without debugging so that the
> > comparisons make more sense.
> 
> Erm, how do I do that? Also they might be correct this is x86_64 which 
> comes with almost twice as big code and: "wc -l abituguru.c" gives 1473.

Granted, drivers are a bit larger on x86_64 but not to that extent. I
have w83627hf.o around 35 kB on x86 and 51 kB on x86_64, for example.

Please check in your kernel configuration if you have some kernel-wide
debugging option set. These options are under Kernel hacking > Kernel
debugging (most of them are named CONFIG_DEBUG_*, but not all).

I remember that Jim Cromie reported similarly incredible sizes when he
was working on the pc87360 driver. I don't remember what the cause was
(if I ever knew it).

> Sorry no figures, it just doesn't feel right todo all this opens / close 
> if we can pass it all in one file. Also be aware that each open and each 
> read and each close will cause a userspace <-> kernel space context 
> switch syscalls are expensive. Doing this the lots of files way just 
> doesn't feel right for me (I've learned programming on a 8051, so 9k 
> additional data is _huge_ to me).

Well we're not talking about 8051 here (whatever it is) so we have to
think in terms of hardware which actually runs Linux, and 9 additional
kB really isn't much in that respect (or at least not enough to
invalidate my proposed interface.)

However, your point about context switching is interesting. I have to
admit I did not think about it at all until now. Greg, can you please
comment on this point? Imagine that we have 200 sysfs files total for a
driver, and an application which is polling half of them every two
seconds, this means an average 50 such context switches per second.
Does it sound bad?

(As a side question, I am wondering if inotify could be used instead of
polling here? Does inotify work on sysfs? If so maybe this addresses
Hans' objection altogether.)

Greg, you pointed our that there were large amounts of sysfs files in
other areas of the kernel, however do these other areas repeatedly poll
the values found in these sysfs files, like we do to the hwmon drivers?

Thanks,
-- 
Jean Delvare


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [lm-sensors] Memory usage of sysfs files
  2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
                   ` (7 preceding siblings ...)
  2006-03-10 13:46 ` Jean Delvare
@ 2006-03-10 14:09 ` Jean Delvare
  2006-03-10 15:28 ` Jim Cromie
  9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2006-03-10 14:09 UTC (permalink / raw)
  To: lm-sensors

Hi Hans,

> I've given this some more thought and I've decided against implementing 
> my way for a couple of other drivers to be able to really measure the 
> difference. I believe we agree my way is somewhat more efficient, but 
> that the differences aren't big enough to swing the scales to my way 
> only on the efficiency reasons.
> 
> Instead of spending more time coding test-cases I'd rather just see a 
> decision soon and (if nescessarry) I'll update the uguru driver to match .
> 
> There are 2 arguments I would still like to make in favor of my way though:
> 
> 1) you initially did it this way too for atleast one driver. Remember, 
> usually your first hunch is a good one.

Doesn't work for me, it seems. See my automatic fan speed control
interface :(

If I did it that way for the f71805f, this was mostly suggested by the
design of the chip itself. If you look at the code you'll see that it's
really simple. This one chip was almost meant for your proposal. But
for many other chips it wouldn't be that nice at all.

Also, I did this without really thinking of all the other features
(separate high/low alarms, input faults, beeps, etc...) that other
drivers would need, and ideas cannot always be generalized.

> 2) I've also written support for libsensors and the sensors application 
> when doing things my way and that fits nicely. I'm afraid that the many 
> files way will be a problem with the current libsensors. I don't want to 
> now wath the lib and the progs chips.* will look like. 100+ additional 
> entries for the uguru alone! And I don't think the chip specific reading 
> and report code in the sensors application will get any pretter too.

This will look fat and ugly, no matter which alarms interface we go
with. This is due to the broken design of libsensors, there's not much
we can do until then. And this isn't specific to the uguru either.

Having seperate alarm files would simply cause you to pass feature IDs
for alarms instead of pre-computed booleans to print_abituguru_in and
friends. It really doesn't change much in terms of number of lines of
code, just try it.

> Now I know that libsensors is destined to be replaced, but it will be 
> around for a while I think.

True, but the new interface is clearly meant for the future library, so
I really don't care if it looks less than optimal for the old library.

> I've attached a patch adding support for the uguru to both lib and prog 
> with alarm reporting to give you an idea how this will look like when 
> doing things my way. Please note that this patch needs updating for
> 2.10 / cvs and thus won't apply.

You may spare some lines in lib/chips.h by defining feature IDs as
parametrized macros rather than constants. Given that this is how you
end up using these values, this would be cleaner too. That's what I
did for the f71805f, if you want to take a look.

Also, I invite you to leave some room after each set of values. Having
SENSORS_ABITUGURU_IN10 = 11 and SENSORS_ABITUGURU_IN0_MIN = 12 doesn't
look good at all - what will you do if the next version of the uguru
has a 12th voltage input?

Thanks,
-- 
Jean Delvare


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [lm-sensors] Memory usage of sysfs files
  2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
                   ` (8 preceding siblings ...)
  2006-03-10 14:09 ` Jean Delvare
@ 2006-03-10 15:28 ` Jim Cromie
  9 siblings, 0 replies; 11+ messages in thread
From: Jim Cromie @ 2006-03-10 15:28 UTC (permalink / raw)
  To: lm-sensors

Jean Delvare wrote:
>
> Please check in your kernel configuration if you have some kernel-wide
> debugging option set. These options are under Kernel hacking > Kernel
> debugging (most of them are named CONFIG_DEBUG_*, but not all).
>
> I remember that Jim Cromie reported similarly incredible sizes when he
> was working on the pc87360 driver. I don't remember what the cause was
> (if I ever knew it).
>   

It never came up that they were 'incredible', but you did comment on 
DEBUG and
the validity of size comparisons.  Ive got various kernels lying around, 
the sizes of
the .ko files is either ~30kb or ~135kb.

Various DEBUG flags are strong determining factors, but Ive not turned 
the flags off,
as they dont interfere with checking sizes before & after other patches.

FWIW, /usr/bin/size gives more info than wc or ls -l, and it seems to 
eliminate
some of the 'noise' in the results, esp those due to CONFIG diffs

soekris:/lib/modules# size `find . -name pc87360.ko `
   text    data     bss     dec     hex filename
  14893    3400      16   18309    4785 
./2.6.16-rc5-mm2/kernel/drivers/hwmon/pc87360.ko
  13370    2780      16   16166    3f26 
./2.6.15-sk/kernel/drivers/hwmon/pc87360.ko
  12029    2972      16   15017    3aa9 
./2.6.15.1-ipipe-103-sony/kernel/drivers/hwmon/pc87360.ko 
  13876    3085      16   16977    4251 
./2.6.14-1.1656_FC4/kernel/drivers/hwmon/pc87360.ko
  12029    2972      16   15017    3aa9 
./2.6.15-ipipe-103-sony/kernel/drivers/hwmon/pc87360.ko
  13800    3085      16   16901    4205 
./2.6.15-1.1831_FC4/kernel/drivers/hwmon/pc87360.ko
  13806    2740      16   16562    40b2 
./2.6.16-rc5-sk/kernel/drivers/hwmon/pc87360.ko
  11894    3276      16   15186    3b52 
./2.6.16-rc1-sony/kernel/drivers/hwmon/pc87360.ko
  13800    3085      16   16901    4205 
./2.6.15-1.1830_FC4/kernel/drivers/hwmon/pc87360.ko
  13869    3368      16   17253    4365 
./2.6.15-mm4-sony/kernel/drivers/hwmon/pc87360.ko
  13117    2780      16   15913    3e29 
./2.6.15.6-sk/kernel/drivers/hwmon/pc87360.ko
  13117    2780      16   15913    3e29 
./2.6.15.6-ipipe-121-sk/kernel/drivers/hwmon/pc87360.ko
  14572    3176      16   17764    4564 
./2.6.16-rc5-mm2-sk/kernel/drivers/hwmon/pc87360.ko


BTW, I'll take this (on-topic) opportunity to remind you of  2 patches I 
sent on 1/26:

1. hwmon-pc87360-use-sensor-attr-2.patch

This converts SENSOR_ATTRs to SENSOR_ATTR_2s,
and add a bunch of #defines for the new member's values.

2. hwmon-pc87360-sysfs-combo-callbacks.patch

This combines individual  (show|set)_Sensor_Attr callbacks into
(show|set)_Sensor callbacks that handle all that Sensor's Attrs.

this results in a non-trivial size reduction (b4, after)
14588    3224      16   17828    45a4 A-2/drivers/hwmon/pc87360.ko
13124    3224      16   16364    3fec A-3/drivers/hwmon/pc87360.ko
ie about 10%

Combining all show_X (for all X) together is possible, but would 
compromise clarity,
and would save only ~ 1/5 th additional space.


hth ;-)
-jimc


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2006-03-10 15:28 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-03-08 22:36 [lm-sensors] Memory usage of sysfs files Hartmut Rick
2006-03-09 19:41 ` Jean Delvare
2006-03-09 20:05 ` Hans de Goede
2006-03-09 20:06 ` Hans de Goede
2006-03-09 21:05 ` Jean Delvare
2006-03-09 21:41 ` Hans de Goede
2006-03-09 23:16 ` Hans de Goede
2006-03-10 11:11 ` Hans de Goede
2006-03-10 13:46 ` Jean Delvare
2006-03-10 14:09 ` Jean Delvare
2006-03-10 15:28 ` Jim Cromie

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.