On Fri, Jun 28, 2019 at 4:36 PM Waldemar Kozaczuk <jwkozac...@gmail.com>
wrote:
> This patch enhances the hpet clock with 32-bit main
> counter to handle wrap-arounds. It does it by maintaining
> separate upper 32-bit counter and incrementing it
> when wrap around is detected.
>
> Signed-off-by: Waldemar Kozaczuk <jwkozac...@gmail.com>
> ---
> drivers/hpet.cc | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hpet.cc b/drivers/hpet.cc
> index ba9ed3bc..a6696405 100644
> --- a/drivers/hpet.cc
> +++ b/drivers/hpet.cc
> @@ -34,21 +34,37 @@ protected:
>
> #define HPET_COUNTER 0x0f0
>
> -//FIXME: Enhance this class to handle wrap-around
> class hpet_32bit_clock : public hpetclock {
> public:
> hpet_32bit_clock(mmioaddr_t hpet_mmio_address) :
> hpetclock(hpet_mmio_address) {
> - debug_early_u64("WARNING: hpet with 32-bit counter will wrap
> around in seconds: ",
> - (_period * (1UL << 32)) / 1000000000UL);
> + _upper_32bit_counter.store(0);
> }
> protected:
> virtual s64 time() override __attribute__((no_instrument_function)) {
> - return _wall + mmio_getl(_addr + HPET_COUNTER) * _period;
> + return _wall + fetch_counter() * _period;
> }
>
> virtual s64 uptime() override __attribute__((no_instrument_function))
> {
> - return mmio_getl(_addr + HPET_COUNTER) * _period;
> + return fetch_counter() * _period;
> + }
> +private:
> + s64 fetch_counter() {
> + auto last_valid_counter = _last_read_counter.load();
>
+ auto upper_counter = _upper_32bit_counter.load();
>
Again, can use relaxed memory ordering in this load, or we're trying to
order some memory access with something else? Maybe "counter"?
+
> + s64 counter = mmio_getl(_addr + HPET_COUNTER);
> + if (counter < last_valid_counter) { // Wrap-around
>
+ // In case more than one thread fetches the value and encounter
> + // the wrap-around, make sure the upper counter is only
> incremented
> + // once
> + _upper_32bit_counter.compare_exchange_strong(upper_counter,
> upper_counter + 1);
>
This doesn't look 100% safe to me. Look at the following time sequence on
two CPUs:
CPU 1 CPU2
last_counter=2^32-1 last_counter=2^32-1
upper_counter = 17
counter = 137 (<last_counter)
upper_counter = 18 (with CAS)
upper_counter = 18
counter=785 (<last_counter)
upper_counter = 19 (with CAS)
last_counter = 137
last_counter = 18
I think that the end result of this extremely-unlikely timing is that
upper_counter was incremented twice (from 17 to 19) despite just one
wrap-around period passing.
I wonder if it wouldn't be much simpler to have the upper_counter per-cpu
instead of trying to make it global. We would need
to assume that each of the CPUs calls the clock many times per wrap-around
period to be able to catch this wrap-around, but
I think that with an 8 minute period (IIRC... am I right? can you please
verify?), this will always be the case.
By the way, I believe we also get an interrupt on every wrap-around, so can
use that to count the wrap-around instead of
trying to guess when it happens. Can you please take a look at other HPET
drivers (e.g., in Linux) to see how they handle
32-bit hpet and perhaps get ideas?
> + }
> +
> + _last_read_counter.store(counter);
> + return _upper_32bit_counter.load() * (1UL << 32) + counter;
>
Nitpick: Instead of *(1<<32) you can (s64).. << 32, instead of hoping the
optimizer will replace the multiplication by a shift.
}
> +
> + std::atomic<s64> _upper_32bit_counter;
> };
>
> class hpet_64bit_clock : public hpetclock {
> --
> 2.20.1
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to osv-dev+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/osv-dev/20190628133630.7563-1-jwkozaczuk%40gmail.com
> .
> For more options, visit https://groups.google.com/d/optout.
>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/CANEVyjucG1bUznM0xtZWgS15WVXu_Lhzots-9BXxoOu6orxRLQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
