Re: [PATCH] i2c-isch: Decrease delay in the loop checking the BUSY state of the bus

[Olivier Sobrie <olivier-Ui3EtX6WB9GzQB+pC5nmwQ@public.gmane.org>]

On Tue, Jan 24, 2012 at 10:58:38AM +0100, Jean Delvare wrote:
> OK, changed. I have queued your patch for kernel 3.4, it will be in
> linux-next until then. If adjustments are needed, feel free to send an
> update.
> 
> http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/i2c-isch-decrease-delay-in-command-completion-check-loop.patch

Ok thanks!

> > (...)
> > I didn't check the CPU load. But I assume there will be no difference
> > in my case as the timer is generally fired only one time.
> 
> In my own tests, the CPU load seems to increase slightly, most probably
> because I use i2c_smbus_read_byte_data() which is a longer transaction
> than yours so the timer is fired several times (2 or 3) per
> transaction. If I change the range from (100, 200) to (250, 500) then
> I'm almost back to the original CPU load figures, modulo measurement
> noise, with almost the same speed benefit.
> 
> As the optimal range depends on the SMBus frequency and the transaction,
> it seems a little difficult to optimize. We can settle on a per-driver
> range, and anyone not happy with it will have to work on interrupt
> support ;)
> 
> > For info, I tested this change with a touchscreen device for which I've
> > to perform a lot of i2c_smbus_read_byte() to read touch data.
> > I'll have a look at the CPU load. By the way if you've a good idea how
> > to have relevant measures I'm interested in.
> 
> I am using the i2c-dev driver + i2cdump (from the i2c-tools package) on
> an arbitrary SMBus slave on my SMBus. I'm measuring the time it takes
> to dump all the register space:
> 
> # modprobe i2c-dev
> # time for i in `seq 1 10` ; do i2cdump -y 8 0x2f b > /dev/null ; done
> 
> real	0m5.139s
> user	0m0.016s
> sys	0m0.118s
> 
> This was with the original i2c-i801 driver. "real" tells me how fast
> register reads actually are (2560 reads in 5.139 s -> 2 ms/read on
> average), and "sys" how much they cost in terms of CPU. "user" can be
> ignored. With usleep_range(100, 200) I get:
> 
> real	0m1.448s
> user	0m0.006s
> sys	0m0.150s
> 
> So you can see it's much faster (0.57 ms/read) but costs more CPU. With
> usleep_range(250, 500) I get:
> 
> real	0m1.587s
> user	0m0.003s
> sys	0m0.124s
> 
> That's 0.62 ms/read. And finally with usleep_range(400, 700) I get:
> 
> real	0m2.043s
> user	0m0.007s
> sys	0m0.118s
> 
> The speed/CPU tradeoff is visible, and I think I'll go with
> usleep_range(250, 500).
> 
> Of course if you want more accurate measurements you want to do more
> iterations and probably use better statistical analysis than the sum I
> did.

I performed the same test you did on my system and I observed this:

* msleep(1)
   real    0m 51.20s
   user    0m 0.29s
   sys     0m 0.00s

* usleep_range(100, 200)
   real    0m 1.46s
   user    0m 0.10s
   sys     0m 0.10s

* usleep_range(250, 500)
   real    0m 2.01s
   user    0m 0.05s
   sys     0m 0.25s

* usleep_range(50, 150)
   real    0m 1.43s
   user    0m 0.07s
   sys     0m 0.23s

I think usleep_range(100, 200) is the best compromise.

> > Concerning the system without hrtimers support, I just did a test and
> > the performances decrease! It introduces again a long delay... which is
> > not the case if I do a udelay(100)...
> 
> But udelay() is busy-waiting so it would have an unacceptable CPU cost
> especially on large transactions. Question is, is usleep_range(100,
> 200) without hrtimers support slower than msleep(1)? If not then we're
> fine. If it is slower then that would be a bug in the implementation of
> usleep_range(), as it really shouldn't be each driver's job to check
> for this individually.

I agree udelay() is not a good solution!
I did the test without hrtimers using usleep_range(100, 200) and got:
    real    0m 25.60s
    user    0m 0.30s
    sys     0m 0.00s
So that's not slower than msleep(1) in the case of no hrtimers.

-- 
Olivier Sobrie