I saw flashrom process use too many cpu time when I use it to write flashchip. Then I strace the process, and found too many "clock_gettime(CLOCK_MONOTONIC...)" system call. And located it to udelay.c line 39, in function clock_usec_delay(). So I replace the code with just usleep(), as below, static void clock_usec_delay(int usecs) { usleep(usecs); } And it still worked well, and use much less cpu time, and the time spend on erase&write a chip is basicly the same as before.
Also I write image to a flashchip and read it out with the modified flashrom many times, and the read out results are always same as the original image.
So why not use usleep() to save cpu time ?
BTW: I have used flashrom many years, it helped me a lot, thanks for your work, very grateful.
On Sat, Jul 10, 2021 at 11:45:19AM +0800, Patrick Star wrote:
I saw flashrom process use too many cpu time when I use it to write flashchip. Then I strace the process, and found too many "clock_gettime(CLOCK_MONOTONIC...)" system call. And located it to udelay.c line 39, in function clock_usec_delay(). So I replace the code with just usleep(), as below, static void clock_usec_delay(int usecs) { usleep(usecs); } And it still worked well, and use much less cpu time, and the time spend on erase&write a chip is basicly the same as before.
Also I write image to a flashchip and read it out with the modified flashrom many times, and the read out results are always same as the original image.
So why not use usleep() to save cpu time ?
usleep() will sleep a whole lot longer than a few microseconds when the kernel isn't tickless; this could slow flashrom down drastically if for instance a usleep() will always sleep at least 10 milliseconds on a 100 Hz tick-based kernel.
On Sat, Jul 10, 2021 at 11:45:19AM +0800, Patrick Star wrote:
I saw flashrom process use too many cpu time when I use it to write flashchip. Then I strace the process, and found too many "clock_gettime(CLOCK_MONOTONIC...)" system call.
This is odd.
clock_gettime(CLOCK_MONOTONIC...) shouldn't normally show up in strace (since most architectures arrange implement it in the vdso there are no syscalls to trace when running this function).
And located it to udelay.c line 39, in function clock_usec_delay(). So I replace the code with just usleep(), as below, static void clock_usec_delay(int usecs) { usleep(usecs); } And it still worked well, and use much less cpu time, and the time spend on erase&write a chip is basicly the same as before.
Also I write image to a flashchip and read it out with the modified flashrom many times, and the read out results are always same as the original image.
So why not use usleep() to save cpu time ?
For the very short delays used by some programmers the cost of sleeping might simply outweigh the cost of a spinning wait.
More likely though this is there to prevent issues on some systems (notably those without hrtimers) when a short sleep results in the calling thread being asleep for *much* longer than requests. This causes very slow programmer performance in some cases. 100x or even 1000x slowdown is not implausible.
Of course the break even point will be different on different systems. flashrom's internal_delay() function is pretty conservative to allow it to run well even on systems without hrtimers. The impact will also very between different programmers since some programmers delay for much longer than others.
Daniel.
On Mon, Jul 12, 2021 at 5:53 PM Daniel Thompson daniel.thompson@linaro.org wrote:
On Sat, Jul 10, 2021 at 11:45:19AM +0800, Patrick Star wrote:
I saw flashrom process use too many cpu time when I use it to write flashchip. Then I strace the process, and found too many "clock_gettime(CLOCK_MONOTONIC...)" system call.
This is odd.
clock_gettime(CLOCK_MONOTONIC...) shouldn't normally show up in strace (since most architectures arrange implement it in the vdso there are no syscalls to trace when running this function).
On my system, only CLOCK_REALTIME_COARSE and CLOCK_MONOTONIC_COARSE not show up in strace. May this behavior has something to do with my kernel configuration(see attachment).
And located it to udelay.c line 39, in function clock_usec_delay(). So I replace the code with just usleep(), as below, static void clock_usec_delay(int usecs) { usleep(usecs); } And it still worked well, and use much less cpu time, and the time spend
on
erase&write a chip is basicly the same as before.
Also I write image to a flashchip and read it out with the modified flashrom many times, and the read out results are always same as the original image.
So why not use usleep() to save cpu time ?
For the very short delays used by some programmers the cost of sleeping might simply outweigh the cost of a spinning wait.
More likely though this is there to prevent issues on some systems (notably those without hrtimers) when a short sleep results in the calling thread being asleep for *much* longer than requests. This causes very slow programmer performance in some cases. 100x or even 1000x slowdown is not implausible.
Of course the break even point will be different on different systems. flashrom's internal_delay() function is pretty conservative to allow it to run well even on systems without hrtimers. The impact will also very between different programmers since some programmers delay for much longer than others.
Thanks for your explanation, helped me a lot. I personally will use usleep in flashrom, just to save a little energy when use battery. :-)
Daniel.
Hi,
Am 12.07.21 um 13:14 schrieb Patrick Star:
On Mon, Jul 12, 2021 at 5:53 PM Daniel Thompson <daniel.thompson@linaro.org mailto:daniel.thompson@linaro.org> wrote:
On Sat, Jul 10, 2021 at 11:45:19AM +0800, Patrick Star wrote: > I saw flashrom process use too many cpu time when I use it to write > flashchip. > Then I strace the process, and found too many > "clock_gettime(CLOCK_MONOTONIC...)" system call. This is odd. clock_gettime(CLOCK_MONOTONIC...) shouldn't normally show up in strace (since most architectures arrange implement it in the vdso there are no syscalls to trace when running this function).On my system, only CLOCK_REALTIME_COARSE and CLOCK_MONOTONIC_COARSE not show up in strace. May this behavior has something to do with my kernel configuration(see attachment).
> And located it to udelay.c line 39, in function clock_usec_delay(). > So I replace the code with just usleep(), as below, > static void clock_usec_delay(int usecs) > { > usleep(usecs); > } > And it still worked well, and use much less cpu time, and the time spend on > erase&write a chip is basicly the same as before. > > Also I write image to a flashchip and read it out with the modified > flashrom many times, and the read out results are always same as the > original image. > > So why not use usleep() to save cpu time ? For the very short delays used by some programmers the cost of sleeping might simply outweigh the cost of a spinning wait. More likely though this is there to prevent issues on some systems (notably those without hrtimers) when a short sleep results in the calling thread being asleep for *much* longer than requests. This causes very slow programmer performance in some cases. 100x or even 1000x slowdown is not implausible. Of course the break even point will be different on different systems. flashrom's internal_delay() function is pretty conservative to allow it to run well even on systems without hrtimers. The impact will also very between different programmers since some programmers delay for much longer than others.Thanks for your explanation, helped me a lot. I personally will use usleep in flashrom, just to save a little energy when use battery. :-)
Since some of that code is mine, I should probably explain.
In the ancient days of parallel and LPC/FWH flash, timing was everything. We had real-world flash chips which needed at least 1 microsecond between reads of the status register because with shorter intervals the status register would return "operation complete" instead of "erase/write in progress". We also had chips where delays between operations had to be in a time window between 10 and 100 microseconds, or the chip would discard commands and/or read/write incorrect data. With SPI flash, some timings are still fun, but nowhere near as catastrophic as in the old days. That just leaves the problem of usleep() possibly slowing down writes a bit (factor of 1000 is indeed possible), depending on which SPI flash chip you're using.
Then again, should you use the Bus Pirate as your programmer of choice, writing some flash chips can take more than 24 hours and energy saving really matters, but the timing-sensitive chips are incompatible with the Bus Pirate anyway.
Regards, Carl-Daniel
On Tue, Jul 13, 2021 at 12:42 AM Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
Hi,
Am 12.07.21 um 13:14 schrieb Patrick Star:
On Mon, Jul 12, 2021 at 5:53 PM Daniel Thompson <daniel.thompson@linaro.org mailto:daniel.thompson@linaro.org> wrote:
On Sat, Jul 10, 2021 at 11:45:19AM +0800, Patrick Star wrote: > I saw flashrom process use too many cpu time when I use it to write > flashchip. > Then I strace the process, and found too many > "clock_gettime(CLOCK_MONOTONIC...)" system call. This is odd. clock_gettime(CLOCK_MONOTONIC...) shouldn't normally show up instrace
(since most architectures arrange implement it in the vdso there are no syscalls to trace when running this function).On my system, only CLOCK_REALTIME_COARSE and CLOCK_MONOTONIC_COARSE not show up in strace. May this behavior has something to do with my kernel configuration(see attachment).
> And located it to udelay.c line 39, in function clock_usec_delay(). > So I replace the code with just usleep(), as below, > static void clock_usec_delay(int usecs) > { > usleep(usecs); > } > And it still worked well, and use much less cpu time, and the time spend on > erase&write a chip is basicly the same as before. > > Also I write image to a flashchip and read it out with the modified > flashrom many times, and the read out results are always same asthe
> original image. > > So why not use usleep() to save cpu time ? For the very short delays used by some programmers the cost of sleeping might simply outweigh the cost of a spinning wait. More likely though this is there to prevent issues on some systems (notably those without hrtimers) when a short sleep results in the calling thread being asleep for *much* longer than requests. This causes very slow programmer performance in some cases. 100x or even 1000xslowdown
is not implausible. Of course the break even point will be different on differentsystems.
flashrom's internal_delay() function is pretty conservative to allow it to run well even on systems without hrtimers. The impact will also very between different programmers since some programmers delay for much longer than others.Thanks for your explanation, helped me a lot. I personally will use usleep in flashrom, just to save a little energy when use battery. :-)
Since some of that code is mine, I should probably explain.
In the ancient days of parallel and LPC/FWH flash, timing was everything. We had real-world flash chips which needed at least 1 microsecond between reads of the status register because with shorter intervals the status register would return "operation complete" instead of "erase/write in progress". We also had chips where delays between operations had to be in a time window between 10 and 100 microseconds, or the chip would discard commands and/or read/write incorrect data. With SPI flash, some timings are still fun, but nowhere near as catastrophic as in the old days. That just leaves the problem of usleep() possibly slowing down writes a bit (factor of 1000 is indeed possible), depending on which SPI flash chip you're using.
Then again, should you use the Bus Pirate as your programmer of choice, writing some flash chips can take more than 24 hours and energy saving really matters, but the timing-sensitive chips are incompatible with the Bus Pirate anyway.
Regards, Carl-Daniel
Thanks for these knowledge. Forget about what I said about "flashrom and energy saving". Since it not running all day and every day, it doesn't matter for flashrom to save a little bit energy for most situations. Except one day I was at a factory, need to programme a 16MB flash chip, with a laptop which had only 30% battery, no outlet available. It took 12 mins to write the image to it, and 20% battery left after that. Normally it shouldn't use that much battery by only 12 mins. So after returned home, I dived into it, and found spinning wait cause it. A better laptop can fix it. :-)
BTW: I don't have Bus Pirate, instead I use a self-made programmer from https://github.com/dword1511/stm32-vserprog
On Tue, Jul 13, 2021 at 11:45:14AM +0800, Patrick Star wrote:
On Tue, Jul 13, 2021 at 12:42 AM Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
Hi,
Am 12.07.21 um 13:14 schrieb Patrick Star:
On Mon, Jul 12, 2021 at 5:53 PM Daniel Thompson <daniel.thompson@linaro.org mailto:daniel.thompson@linaro.org> wrote:
On Sat, Jul 10, 2021 at 11:45:19AM +0800, Patrick Star wrote: > I saw flashrom process use too many cpu time when I use it to write > flashchip. > Then I strace the process, and found too many > "clock_gettime(CLOCK_MONOTONIC...)" system call. This is odd. clock_gettime(CLOCK_MONOTONIC...) shouldn't normally show up instrace
(since most architectures arrange implement it in the vdso there are no syscalls to trace when running this function).On my system, only CLOCK_REALTIME_COARSE and CLOCK_MONOTONIC_COARSE not show up in strace. May this behavior has something to do with my kernel configuration(see attachment).
> And located it to udelay.c line 39, in function clock_usec_delay(). > So I replace the code with just usleep(), as below, > static void clock_usec_delay(int usecs) > { > usleep(usecs); > } > And it still worked well, and use much less cpu time, and the time spend on > erase&write a chip is basicly the same as before. > > Also I write image to a flashchip and read it out with the modified > flashrom many times, and the read out results are always same asthe
> original image. > > So why not use usleep() to save cpu time ? For the very short delays used by some programmers the cost of sleeping might simply outweigh the cost of a spinning wait. More likely though this is there to prevent issues on some systems (notably those without hrtimers) when a short sleep results in the calling thread being asleep for *much* longer than requests. This causes very slow programmer performance in some cases. 100x or even 1000xslowdown
is not implausible. Of course the break even point will be different on differentsystems.
flashrom's internal_delay() function is pretty conservative to allow it to run well even on systems without hrtimers. The impact will also very between different programmers since some programmers delay for much longer than others.Thanks for your explanation, helped me a lot. I personally will use usleep in flashrom, just to save a little energy when use battery. :-)
Since some of that code is mine, I should probably explain.
In the ancient days of parallel and LPC/FWH flash, timing was everything. We had real-world flash chips which needed at least 1 microsecond between reads of the status register because with shorter intervals the status register would return "operation complete" instead of "erase/write in progress". We also had chips where delays between operations had to be in a time window between 10 and 100 microseconds, or the chip would discard commands and/or read/write incorrect data. With SPI flash, some timings are still fun, but nowhere near as catastrophic as in the old days. That just leaves the problem of usleep() possibly slowing down writes a bit (factor of 1000 is indeed possible), depending on which SPI flash chip you're using.
Then again, should you use the Bus Pirate as your programmer of choice, writing some flash chips can take more than 24 hours and energy saving really matters, but the timing-sensitive chips are incompatible with the Bus Pirate anyway.
Regards, Carl-Daniel
Thanks for these knowledge. Forget about what I said about "flashrom and energy saving". Since it not running all day and every day, it doesn't matter for flashrom to save a little bit energy for most situations. Except one day I was at a factory, need to programme a 16MB flash chip, with a laptop which had only 30% battery, no outlet available. It took 12 mins to write the image to it, and 20% battery left after that. Normally it shouldn't use that much battery by only 12 mins. So after returned home, I dived into it, and found spinning wait cause it. A better laptop can fix it. :-)
BTW: I don't have Bus Pirate, instead I use a self-made programmer from https://github.com/dword1511/stm32-vserprog
Interesting (and duely noted in case I need it).
Assuming the dominant caller of the delay functions is via serprog_delay() then it looks like flashrom would avoid using internal_delay() entirely if you could add an implementation of S_CMD_O_DELAY to this firmware. I think the effect of that would be that STM32 would do the spinning so the host doesn't have to.
Daniel.
On Tue, Jul 13, 2021 at 2:06 PM Daniel Thompson daniel.thompson@linaro.org wrote:
On Tue, Jul 13, 2021 at 11:45:14AM +0800, Patrick Star wrote:
<snip>
BTW: I don't have Bus Pirate, instead I use a self-made programmer from https://github.com/dword1511/stm32-vserprog
Interesting (and duely noted in case I need it).
Assuming the dominant caller of the delay functions is via serprog_delay() then it looks like flashrom would avoid using internal_delay() entirely if you could add an implementation of S_CMD_O_DELAY to this firmware. I think the effect of that would be that STM32 would do the spinning so the host doesn't have to.
Because historical stuff (serprog being designed for the parallel/LPC/FWH flashes carldani talked about, not SPI :) ), you will need to also have the opbuf init and exec commands, but you do not need an actual opbuf (operation buffer), a single delay variable is enough, there is an implementation of that in libfrser: https://github.com/urjaman/libfrser/blob/master/frser.c#L401
And this is why I'd prefer serprog implementations to be actually atleast based on libfrser ;)
-
Carl-Daniel Hailfinger -
Daniel Thompson -
Jonathan A. Kollasch -
Patrick Star -
Urja Rannikko