[lm-sensors] TODO: "dynamic" sysfs callbacks

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Jim Cromie]

once again with the email-harvesting / communication/coordination.

<quoting Jean, I think>

BTW, do you have any plan to convert the asb100 driver to use the
"dynamic" sysfs callbacks? This would kill some more macros, and shrink
the driver size even more.


The approx list of 'opportunities' here is:

[jimc at harpo ac-2]$ grep -rl '##' drivers/hwmon/*.c
drivers/hwmon/abituguru.c
drivers/hwmon/adm1021.c
drivers/hwmon/adm1025.c
drivers/hwmon/adm1026.c	 - patch on hold, needs tester.
drivers/hwmon/adm1031.c
drivers/hwmon/adm9240.c
drivers/hwmon/asb100.c
drivers/hwmon/ds1621.c
drivers/hwmon/fscher.c
drivers/hwmon/fscpos.c
drivers/hwmon/gl518sm.c
drivers/hwmon/gl520sm.c
drivers/hwmon/it87.c
drivers/hwmon/lm75.c
drivers/hwmon/lm77.c
drivers/hwmon/lm78.c
drivers/hwmon/lm80.c
drivers/hwmon/lm85.c
drivers/hwmon/lm87.c
drivers/hwmon/lm92.c
drivers/hwmon/max1619.c
drivers/hwmon/sis5595.c
drivers/hwmon/smsc47b397.c
drivers/hwmon/smsc47m192.c
drivers/hwmon/smsc47m1.c
drivers/hwmon/via686a.c
drivers/hwmon/vt8231.c
drivers/hwmon/w83627ehf.c
drivers/hwmon/w83627hf.c
drivers/hwmon/w83781d.c
drivers/hwmon/w83791d.c
drivers/hwmon/w83792d.c



"dynamic" sysfs callbacks is :


current code base has lots of macros which are essentially
static inline functions, but which are 'parameterized' by
'inserting' an ##offset## into the function name, and 'invoking'
the macro repeatedly for multiple instances of a sensor.

an example should clarify. (from w83781d.c, chosen arbitrarily)

    341 static ssize_t store_regs_in_##reg##offset (struct device *dev, struct device_attribute *attr, const
    341 char *buf, size_t count) \
    342 { \
    343         return store_in_##reg (dev, buf, count, offset); \
    344 } \
    345 static DEVICE_ATTR(in##offset##_##reg, S_IRUGO| S_IWUSR, show_regs_in_##reg##offset, store_regs_in_##reg##offset);
    346
    347 #define sysfs_in_offsets(offset) \
    348 sysfs_in_offset(offset); \
    349 sysfs_in_reg_offset(min, offset); \
    350 sysfs_in_reg_offset(max, offset);
    351
    352 sysfs_in_offsets(0);
    353 sysfs_in_offsets(1);
    354 sysfs_in_offsets(2);
    355 sysfs_in_offsets(3);
    356 sysfs_in_offsets(4);
    357 sysfs_in_offsets(5);
    358 sysfs_in_offsets(6);
    359 sysfs_in_offsets(7);
    360 sysfs_in_offsets(8);

Once these macros are expanded and compiled, we get a multitude
of nearly identical routines.

[jimc at harpo hwmon]$ nm  -S w83781d.o | grep store_regs_
00001461 00000011 t store_regs_beep_enable
00001472 00000011 t store_regs_beep_mask
00002971 00000011 t store_regs_fan_div_1
00002982 00000011 t store_regs_fan_div_2
00002993 00000011 t store_regs_fan_div_3
00000589 00000011 t store_regs_fan_min1
0000059a 00000011 t store_regs_fan_min2
000005ab 00000011 t store_regs_fan_min3
00000428 00000011 t store_regs_in_max0
00000439 00000011 t store_regs_in_max1
0000044a 00000011 t store_regs_in_max2
0000045b 00000011 t store_regs_in_max3
0000046c 00000011 t store_regs_in_max4
0000047d 00000011 t store_regs_in_max5
0000048e 00000011 t store_regs_in_max6
0000049f 00000011 t store_regs_in_max7
000004b0 00000011 t store_regs_in_max8
000002f6 00000011 t store_regs_in_min0
00000307 00000011 t store_regs_in_min1
00000318 00000011 t store_regs_in_min2
00000329 00000011 t store_regs_in_min3
0000033a 00000011 t store_regs_in_min4
0000034b 00000011 t store_regs_in_min5
0000035c 00000011 t store_regs_in_min6
0000036d 00000011 t store_regs_in_min7
0000037e 00000011 t store_regs_in_min8


The macros can be converted to proper static inlines,
and parameterized properly so that 1 func does the job for all 8
inputs above.

static ssize_t show_fan_min(struct device *dev, struct device_attribute *devattr, char *buf)
{

	/*THIS IS THE KEY*/
        struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
	int index = attr->index;

        struct pc87360_data *data = pc87360_update_device(dev);
        return sprintf(buf, "%u\n", FAN_FROM_REG(data->fan_min[index],
                       FAN_DIV_FROM_REG(data->fan_status[index])));
}

static struct sensor_device_attribute fan_min[] = {
        SENSOR_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 0),
        SENSOR_ATTR(fan2_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 1),
        SENSOR_ATTR(fan3_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 2),
};


There are 2 things going on here.

1 - the function back-casts from a base-type to another that 
aggregates another parameter (index), which is then available
to distinguish which of many (fan) sensors is being requested.

2 - the SENSOR_ATTR initializes the index field (0,1,2 above)
So when the function is called, it can use that index.




*Deeper 'dynamic's are possible*

The above folds multiple callbacks on 1 dimension (index)
to reduce code footprint, but more are possible.



following 2 steps above:

1. function now relys on being passed a struct sensor_device_attribute_2
which has 2 additional parameters; index, and nr.  This example uses
2nd parameter to distinguish which attribute of the sensor is being
requested (index still chooses 1 of N sensors)
 
static ssize_t show_fan(struct device *dev, struct device_attribute *devattr, char *buf)
{
        struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
        int idx = attr->index;
        int func = attr->nr;
        struct pc87360_data *data = pc87360_update_device(dev);
        unsigned res = -1;

        switch(func) {
        case FN_FAN_INPUT:
                res = FAN_FROM_REG(data->fan[idx],
                                   FAN_DIV_FROM_REG(data->fan_status[idx]));
                break;
        case FN_FAN_MIN:
                res = FAN_FROM_REG(data->fan_min[idx],
                                   FAN_DIV_FROM_REG(data->fan_status[idx]));
                break;
        case FN_FAN_STATUS:
                res = FAN_STATUS_FROM_REG(data->fan_status[idx]);
                break;
        case FN_FAN_DIV:
                res = FAN_DIV_FROM_REG(data->fan_status[idx]);
                break;
        default:
                printk(KERN_ERR "unknown attr fetch\n");
        }
        return sprintf(buf, "%u\n", res);
}


2.  declare attributes like so.
Note that show_fan is the callback for both fan_input[]s, and fan_status[]s,
as well as the others (DIV,MIN) above.

static struct sensor_device_attribute_2 fan_input[] = {
        SENSOR_ATTR_2(fan1_input, S_IRUGO, show_fan, NULL, FN_FAN_INPUT, 0),
        SENSOR_ATTR_2(fan2_input, S_IRUGO, show_fan, NULL, FN_FAN_INPUT, 1),
        SENSOR_ATTR_2(fan3_input, S_IRUGO, show_fan, NULL, FN_FAN_INPUT, 2),
};
static struct sensor_device_attribute_2 fan_status[] = {
        SENSOR_ATTR_2(fan1_status, S_IRUGO, show_fan, NULL, FN_FAN_STATUS, 0),
        SENSOR_ATTR_2(fan2_status, S_IRUGO, show_fan, NULL, FN_FAN_STATUS, 1),
        SENSOR_ATTR_2(fan3_status, S_IRUGO, show_fan, NULL, FN_FAN_STATUS, 2),
};


NB - show, set routines can never be combined, due to different fn-signatures.
NB - technique saves ~ 9% object code on 1 driver.


This "2-D dynamic callback" approach is not widely used,
currently only done in:
	w83792d.c, abituguru.c.
patches which use it:
	vt1211.c - going in soon I believe (congrats Juerg)
	pc87360.c - 
	- unreviewed, patch 3/3 has some annoying screwups in the attr mapping,
	- as visible above (DIV vs fan_status, will recheck/redo)
	case FN_FAN_DIV:
                res = FAN_DIV_FROM_REG(data->fan_status[idx]);

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Jean Delvare]

Hi Jim,

> once again with the email-harvesting / communication/coordination.
> 
> <quoting Jean, I think>
> 
> BTW, do you have any plan to convert the asb100 driver to use the
> "dynamic" sysfs callbacks? This would kill some more macros, and shrink
> the driver size even more.
> 
> 
> The approx list of 'opportunities' here is:
> 
> [jimc at harpo ac-2]$ grep -rl '##' drivers/hwmon/*.c
> drivers/hwmon/abituguru.c
> drivers/hwmon/adm1021.c
> drivers/hwmon/adm1025.c
> drivers/hwmon/adm1026.c	 - patch on hold, needs tester.

I had to discard it from my stack, it no more applies after Mark M.
Hoffman's patch fixing unchecked return values.

> drivers/hwmon/adm1031.c
> drivers/hwmon/adm9240.c
> drivers/hwmon/asb100.c
> drivers/hwmon/ds1621.c
> drivers/hwmon/fscher.c
> drivers/hwmon/fscpos.c
> drivers/hwmon/gl518sm.c
> drivers/hwmon/gl520sm.c
> drivers/hwmon/it87.c
> drivers/hwmon/lm75.c
> drivers/hwmon/lm77.c
> drivers/hwmon/lm78.c
> drivers/hwmon/lm80.c
> drivers/hwmon/lm85.c
> drivers/hwmon/lm87.c
> drivers/hwmon/lm92.c
> drivers/hwmon/max1619.c
> drivers/hwmon/sis5595.c
> drivers/hwmon/smsc47b397.c
> drivers/hwmon/smsc47m192.c
> drivers/hwmon/smsc47m1.c
> drivers/hwmon/via686a.c
> drivers/hwmon/vt8231.c
> drivers/hwmon/w83627ehf.c
> drivers/hwmon/w83627hf.c
> drivers/hwmon/w83781d.c
> drivers/hwmon/w83791d.c
> drivers/hwmon/w83792d.c

I do not consider the dynamic sysfs callbacks conversion as "must be
done". Of course drivers are nicer and smaller when done, but this is a
significant work, and it needs complete testing, contrary to the
unchecked return values fixes.

So I wouldn't bother converting a driver unless you can test it. See
what happened with the adm1026 driver, you converted it long ago,
nobody ever tested it, and now the patch is discarded as it conflicts
with other changes.

> *Deeper 'dynamic's are possible*
> 
> The above folds multiple callbacks on 1 dimension (index)
> to reduce code footprint, but more are possible.
> 
> 
> 
> following 2 steps above:
> 
> 1. function now relys on being passed a struct sensor_device_attribute_2
> which has 2 additional parameters; index, and nr.  This example uses
> 2nd parameter to distinguish which attribute of the sensor is being
> requested (index still chooses 1 of N sensors)
>  
> static ssize_t show_fan(struct device *dev, struct device_attribute *devattr, char *buf)
> {
>         struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>         int idx = attr->index;
>         int func = attr->nr;
>         struct pc87360_data *data = pc87360_update_device(dev);
>         unsigned res = -1;
> 
>         switch(func) {
>         case FN_FAN_INPUT:
>                 res = FAN_FROM_REG(data->fan[idx],
>                                    FAN_DIV_FROM_REG(data->fan_status[idx]));
>                 break;
>         case FN_FAN_MIN:
>                 res = FAN_FROM_REG(data->fan_min[idx],
>                                    FAN_DIV_FROM_REG(data->fan_status[idx]));
>                 break;
>         case FN_FAN_STATUS:
>                 res = FAN_STATUS_FROM_REG(data->fan_status[idx]);
>                 break;
>         case FN_FAN_DIV:
>                 res = FAN_DIV_FROM_REG(data->fan_status[idx]);
>                 break;
>         default:
>                 printk(KERN_ERR "unknown attr fetch\n");
>         }
>         return sprintf(buf, "%u\n", res);
> }

As I mentioned earlier during the vt1211 review, I'm no big fan of this
approach. You end up aggregating many features which don't share much
code. It's still reasonable in your example above, but I think it has
gone to an extreme in one case in the vt1211 driver. Let's not jump
from one extreme to the other.

I do agree that having a single callback for displaying fanN_input and
fanN_min makes full sense, and this suggests a second level of
indexing. But beyond that point I think we have to be reasonable. I
don't mind for new drivers, authors should stay relatively free,
however I strongly object to converting all our perfectly working
drivers to use that approach.

Beyond the readability of the code, there are some performance issues
to consider. For example I wonder how the code above interacts with the
CPU cache, compared to 1-level-indexed callbacks, in the typical
"sensors" scenario. I don't really have the time to investigate this,
unfortunately. Switch/case is usually not recommended in performance
terms, even though I'd expect gcc to optimize it relatively nicely if
the "func" values are chosen wisely.

> NB - technique saves ~ 9% object code on 1 driver.

It really depends on where you come from for a given driver. But yes
there's definitely a size reduction.

> This "2-D dynamic callback" approach is not widely used,
> currently only done in:
> 	w83792d.c, abituguru.c.
> patches which use it:
> 	vt1211.c - going in soon I believe (congrats Juerg)
> 	pc87360.c - 
> 	- unreviewed, patch 3/3 has some annoying screwups in the attr mapping,
> 	- as visible above (DIV vs fan_status, will recheck/redo)
> 	case FN_FAN_DIV:
>                 res = FAN_DIV_FROM_REG(data->fan_status[idx]);

The 2-index variants may be a problem because not all callbacks need 2
parameters and mixing 0, 1 and 2-indexes attributes can become a mess.
Using sysfs_create_group may help in that respect, because it
manipulates pointers rather than the attributes directly.

I have been thinking of a "standard hwmon attribute" which would
combine our current sensor and sensor_2 attributes. I'll think again
about it after we are done with the unchecked return values.

-- 
Jean Delvare

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Greg KH]

On Fri, Sep 01, 2006 at 02:33:42PM +0200, Jean Delvare wrote:
> Beyond the readability of the code, there are some performance issues
> to consider. For example I wonder how the code above interacts with the
> CPU cache, compared to 1-level-indexed callbacks, in the typical
> "sensors" scenario. I don't really have the time to investigate this,
> unfortunately. Switch/case is usually not recommended in performance
> terms, even though I'd expect gcc to optimize it relatively nicely if
> the "func" values are chosen wisely.

I don't really think it maters at all.  This code is not cache "hot" by
any means.  It's doing something pretty infrequently, and the i2c
protocol is _so_ slow it's not funny.  These are not high-performance
things we are dealing with at all.

So please, don't worry about things like this in the i2c drivers, it's
not an issue.

Also, please note that cache issues are _very_ CPU specific.  Any tuning
you do for a desktop will be pretty useless for an embedded ARM board.
Embedded systems running Linux in a tiny amount of memory with small
batteries (like in phones), need to worry about these things, but even
then, it's sometimes better to make the code big, and the variables
small, recomputing things all the time.  It all depends on the
situation.

thanks,

greg k-h

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Jean Delvare]

Hi Greg,

> On Fri, Sep 01, 2006 at 02:33:42PM +0200, Jean Delvare wrote:
> > Beyond the readability of the code, there are some performance issues
> > to consider. For example I wonder how the code above interacts with the
> > CPU cache, compared to 1-level-indexed callbacks, in the typical
> > "sensors" scenario. I don't really have the time to investigate this,
> > unfortunately. Switch/case is usually not recommended in performance
> > terms, even though I'd expect gcc to optimize it relatively nicely if
> > the "func" values are chosen wisely.
> 
> I don't really think it maters at all.  This code is not cache "hot" by
> any means.  It's doing something pretty infrequently,

Could happen once every second or every other second, if the user has a
GUI with sensors data (gkrellm, ksensors, xsensors...) I think it
qualifies as "frequent".

>                                                       and the i2c
> protocol is _so_ slow it's not funny.  These are not high-performance
> things we are dealing with at all.

Not all hardware monitoring chips are i2c-based. I agree that all hwmon
drivers do quite a lot of I/O though, be it on the SMBus or ISA bus. But
it's hardly a reason not to make the rest of the driver code smaller
and faster if we can.

> So please, don't worry about things like this in the i2c drivers, it's
> not an issue.

OK.

-- 
Jean Delvare

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Greg KH]

On Sat, Sep 02, 2006 at 08:52:47AM +0200, Jean Delvare wrote:
> Hi Greg,
> 
> > On Fri, Sep 01, 2006 at 02:33:42PM +0200, Jean Delvare wrote:
> > > Beyond the readability of the code, there are some performance issues
> > > to consider. For example I wonder how the code above interacts with the
> > > CPU cache, compared to 1-level-indexed callbacks, in the typical
> > > "sensors" scenario. I don't really have the time to investigate this,
> > > unfortunately. Switch/case is usually not recommended in performance
> > > terms, even though I'd expect gcc to optimize it relatively nicely if
> > > the "func" values are chosen wisely.
> > 
> > I don't really think it maters at all.  This code is not cache "hot" by
> > any means.  It's doing something pretty infrequently,
> 
> Could happen once every second or every other second, if the user has a
> GUI with sensors data (gkrellm, ksensors, xsensors...) I think it
> qualifies as "frequent".

No, that's not "frequent" at all for cache related things.  A processor
can do an lot in a second or two :)

> >                                                       and the i2c
> > protocol is _so_ slow it's not funny.  These are not high-performance
> > things we are dealing with at all.
> 
> Not all hardware monitoring chips are i2c-based. I agree that all hwmon
> drivers do quite a lot of I/O though, be it on the SMBus or ISA bus. But
> it's hardly a reason not to make the rest of the driver code smaller
> and faster if we can.

Sure, I'm not saying that at all.  It's just that trying to optimize for
things that you can not even measure, isn't a good thing.

Size of the module, yes, that's fine, but look at percentages.  Shaving
a few bytes here and there at the expense of readablity and
maintainablity is not worth it.

Cache usage of driver code that is run every other second?  Very hard to
measure, it will be lost in the noise...

thanks,

greg k-h

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Jean Delvare]

Hi Greg,

On Sat, 2 Sep 2006 00:08:09 -0700, while he should have been sleeping,
Greg KH wrote:
> On Sat, Sep 02, 2006 at 08:52:47AM +0200, Jean Delvare wrote:
> > Could happen once every second or every other second, if the user has a
> > GUI with sensors data (gkrellm, ksensors, xsensors...) I think it
> > qualifies as "frequent".
> 
> No, that's not "frequent" at all for cache related things.  A processor
> can do an lot in a second or two :)

Hm, in fact I failed to describe the situation clearly. The "once every
second" was intended to underline that it's worth optimizing if
possible and reasonable. The fact that the cache may matter is related
to another implementation detail of the hardware drivers. Let me
explain it better.

Each hardware monitoring driver presents a user-space interface in
sysfs. Large drivers export many files (easily over 50) and hardware
monitoring applications will read from most of them on every refresh.
These sysfs files may of may not share their callbacks. The older
drivers didn't, the newer do. So, depending on the driver
implementation, the same callback function may be called 10 times in a
row.

This is where I suspect cache could matter. I am in no way a hardware
architecture expert though, so I was merely waving my hands, I admit.

> > Not all hardware monitoring chips are i2c-based. I agree that all hwmon
> > drivers do quite a lot of I/O though, be it on the SMBus or ISA bus. But
> > it's hardly a reason not to make the rest of the driver code smaller
> > and faster if we can.
> 
> Sure, I'm not saying that at all.  It's just that trying to optimize for
> things that you can not even measure, isn't a good thing.

Agreed, actually it was quite my original point: let's not change the
code if there is no benefit.

> Size of the module, yes, that's fine, but look at percentages.  Shaving
> a few bytes here and there at the expense of readablity and
> maintainablity is not worth it.

Agreed again; here the idea was to get rid of ugly macros in the older
drivers, gaining both in readability and size (i.e. smaller driver),
and, I suspect, compilation time. The only point of disagreement (I
think) between Jim and me was, how far we wanted to go in that
direction. I am admittedly a conservative person ;)

> Cache usage of driver code that is run every other second?  Very hard to
> measure, it will be lost in the noise...

I don't have the time to even try measuring it. I just thought I would
mention performance in the conversation, after I noticed with a user
some days ago that logging sensors data using "sensors" every other
second would put 15% of load on the system, while I expected it
to be unnoticeable. I'm pretty certain there is room for optimization
almost all the way down from the chip to the output of "sensors". Some
hardware monitoring drivers can be made faster/lighter. Most SMBus
controller drivers could be converted to be interrupt-driven rather than
poll-based. libsensors could be way faster (Mark M. Hoffman is working
on this) and smaller. "sensors" should be way smaller as well, if
libsensors had been designed differently.

-- 
Jean Delvare

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Greg KH]

On Sat, Sep 02, 2006 at 04:24:01PM +0200, Jean Delvare wrote:
> I don't have the time to even try measuring it. I just thought I would
> mention performance in the conversation, after I noticed with a user
> some days ago that logging sensors data using "sensors" every other
> second would put 15% of load on the system, while I expected it
> to be unnoticeable.

This is probably just due to an interruptable sleep happening, which
artificially increases the load average.  I would be supprised if the
CPU usage really goes up by that much.

thanks,

greg k-h

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Jean Delvare]

> On Sat, Sep 02, 2006 at 04:24:01PM +0200, Jean Delvare wrote:
> > I don't have the time to even try measuring it. I just thought I would
> > mention performance in the conversation, after I noticed with a user
> > some days ago that logging sensors data using "sensors" every other
> > second would put 15% of load on the system, while I expected it
> > to be unnoticeable.
> 
> This is probably just due to an interruptable sleep happening, which
> artificially increases the load average.  I would be supprised if the
> CPU usage really goes up by that much.

The bus driver msleep()s, so it's rather an uninterruptible sleep? Not
that I know what different it makes in that context. And there's the
userspace part probably taking a great deal of CPU as well, but I did
not bother analyzing in detail where the time was spent back then.

-- 
Jean Delvare

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Greg KH]

On Sat, Sep 02, 2006 at 10:57:03PM +0200, Jean Delvare wrote:
> > On Sat, Sep 02, 2006 at 04:24:01PM +0200, Jean Delvare wrote:
> > > I don't have the time to even try measuring it. I just thought I would
> > > mention performance in the conversation, after I noticed with a user
> > > some days ago that logging sensors data using "sensors" every other
> > > second would put 15% of load on the system, while I expected it
> > > to be unnoticeable.
> > 
> > This is probably just due to an interruptable sleep happening, which
> > artificially increases the load average.  I would be supprised if the
> > CPU usage really goes up by that much.
> 
> The bus driver msleep()s, so it's rather an uninterruptible sleep?

Yes it is.

> Not that I know what different it makes in that context.

When you sleep in the kernel, in an uninterruptable state, it increases
the load average spit out by the kernel by 1.  Now this really doesn't
make that much sense, as the code is sleeping and not doing anything at
all, but it plays havoc on tools that look at the load average of the
machine to see what is going on.

That might be why users think the driver is taking up more cpu time than
it really is, but it all depends on how they were measuring it.

thanks,

greg k-h

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Jean Delvare]

Greg KH wrote:
> On Sat, Sep 02, 2006 at 10:57:03PM +0200, Jean Delvare wrote:
> > The bus driver msleep()s, so it's rather an uninterruptible sleep?
> 
> Yes it is.
> 
> > Not that I know what different it makes in that context.
> 
> When you sleep in the kernel, in an uninterruptable state, it increases
> the load average spit out by the kernel by 1.  Now this really doesn't
> make that much sense, as the code is sleeping and not doing anything at
> all, but it plays havoc on tools that look at the load average of the
> machine to see what is going on.
> 
> That might be why users think the driver is taking up more cpu time than
> it really is, but it all depends on how they were measuring it.

OK, Thanks for the explanation. Isn't it possible to change the kernel
to not count sleeping processes in the load? That'd make people happier,
and the value more meaningful.

We still should convert our drivers to be interrupt-driven anyway, as
the SMBus transactions would be much faster that way. If it makes the
average load _look_ lower, it can't hurt from a marketing point of
view ;)

-- 
Jean Delvare

[lm-sensors] TODO: "dynamic" sysfs callbacks

[Greg KH]

On Sun, Sep 03, 2006 at 08:08:02AM +0200, Jean Delvare wrote:
> Greg KH wrote:
> > On Sat, Sep 02, 2006 at 10:57:03PM +0200, Jean Delvare wrote:
> > > The bus driver msleep()s, so it's rather an uninterruptible sleep?
> > 
> > Yes it is.
> > 
> > > Not that I know what different it makes in that context.
> > 
> > When you sleep in the kernel, in an uninterruptable state, it increases
> > the load average spit out by the kernel by 1.  Now this really doesn't
> > make that much sense, as the code is sleeping and not doing anything at
> > all, but it plays havoc on tools that look at the load average of the
> > machine to see what is going on.
> > 
> > That might be why users think the driver is taking up more cpu time than
> > it really is, but it all depends on how they were measuring it.
> 
> OK, Thanks for the explanation. Isn't it possible to change the kernel
> to not count sleeping processes in the load? That'd make people happier,
> and the value more meaningful.

From what I remember, no, it is not simple to do this.  But feel free to
poke at it, I really don't know how that code works :)

good luck,

greg k-h