Thanks for the review. I have made some tweaks per your suggestions and
pushed it.
On Monday, August 12, 2019 at 3:59:26 AM UTC-4, Nadav Har'El wrote:
>
> In Sat, Jul 6, 2019 at 6:37 AM Waldemar Kozaczuk <jwkoz...@gmail.com
> <javascript:>> 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 per-cpu and incrementing it
>> when wrap around is detected.
>>
>
> I have a few comments below, but in general I think this direction is
> sort-of-acceptable in the sense that
> it is better than the current situation, which doesn't handle the
> wrap-around at all. So I think we can
> commit this (though please look at my comments below).
>
> But in the long run, I can't say I thin this approach is very reliable. If
> one CPU misses a wrap-around,
> it will have its clock differing from that of other CPUs and this will
> never be corrected. If all of them miss
> the wrap-around (e.g., suspend), none of them get the up-to-date time. To
> fix those things, maybe we
> need to query the RTC on every wrap-around, and perhaps keep the same
> upper-counter on all CPUs
> (but if we do that, we need to do this safely and correctly).
>
I think that querying RTC on every wrap-around to calculate the new value
of the upper counter (instead of incrementing it)
might be the easiest thing to do.
>
> If you want to plan a long-term solution, I suggest you take a look at
> what Linux does for 32-bit HPET.
> I suggest this for two reasons: First, a lot of smart people contributed
> to Linux, and there is a lot of
> experience with their code. But second, since 99% of the OSs running on
> these hypervisors are Linux,
> the authors of these hypervisors probably tuned their implementation of
> HPET to work correctly on
> Linux, so anything deviating from what Linux does there might not work as
> well.
>
Unfortunately, I am not smart enough nor have time to get smarter to read
that code and understand the intention behind it ;-) Again this is just to
make OSv run on the non-production type of hypervisor - hyperkit - on Mac.
I think it is good enough until somebody complains.
>
>
>
>> Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com <javascript:>>
>> ---
>> drivers/hpet.cc | 53 ++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 46 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/hpet.cc b/drivers/hpet.cc
>> index 22afae7d..d08afa76 100644
>> --- a/drivers/hpet.cc
>> +++ b/drivers/hpet.cc
>> @@ -18,6 +18,7 @@ extern "C" {
>> #include <osv/xen.hh>
>> #include <osv/irqlock.hh>
>> #include "rtc.hh"
>> +#include <osv/percpu.hh>
>>
>> using boost::intrusive::get_parent_from_member;
>>
>> @@ -33,23 +34,61 @@ protected:
>>
>> #define HPET_COUNTER 0x0f0
>>
>> -//FIXME: Enhance this class to handle wrap-around
>> +// The hpet clocks are tricky to implement right. Ideally hpet clocks,
>> +// especially those with 32-bit main counter, should be avoided in
>> +// virtualized environments. Unfortunately some hypervisors like
>> +// hyperkit and other bhyve derivatives provide 32-bit hpet clock only
>> and
>> +// we would like to support those hypervisors as well to let potential
>> +// users test OSv on OSX and FreeBSD.
>> +// The implementation below is a compromise that delivers monotonic
>> +// reads but may suffer from inaccurate time reads especially when
>> +// OSv guest gets suspended by host. In essence it maintains it own
>> +// upper 32-bit counter per cpu which gets incremented every time
>> +// it gets less than previously saved main counter read from the host.
>> +// In future we can improve it by handling wrap-around interrupts but
>> +// but it is not clear if it would be better and especially worth the
>> effort.
>>
> 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);
>> - }
>> + hpet_32bit_clock(mmioaddr_t hpet_mmio_address) :
>> hpetclock(hpet_mmio_address),
>> + cpu_notifier([&] { init_on_cpu(); }) {}
>> +
>> 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:
>> + void init_on_cpu() {
>> + (*_upper_32bit_counter).store(0, std::memory_order_relaxed);
>> + (*_last_read_counter).store(0, std::memory_order_relaxed);
>>
>
> Is this even needed? Unless I'm misremembering, per-cpu variables are
> zero-initialized by default, as all global variables.
>
> }
>> +
>> + s64 fetch_counter() {
>> + auto last_valid_counter =
>> (*_last_read_counter).load(std::memory_order_relaxed);
>> + auto upper_counter =
>> (*_upper_32bit_counter).load(std::memory_order_relaxed);
>> +
>> + s64 counter = mmio_getl(_addr + HPET_COUNTER);
>> + if (counter < last_valid_counter) {
>> + // Wrap-around - increment upper counter
>> + (*_upper_32bit_counter).compare_exchange_weak(upper_counter,
>> upper_counter + 1);
>>
>
> compare_exchange_weak should be done in a loop, I think you wanted _strong?
>
> I'm starting to think maybe you did these atomic operations as protection
> against multiple threads, on the same CPU, trying to get time concurrently?
> This can be done by the cheaper (on many-core machines) preempt_lock and I
> think the code will also be easier to understand.
> Although, to be honest, the difference will probably be negligable (the
> compare_exchange above happens rarely, and just the relaxed load has no
> performance penalty.
>
> + }
>> +
>> + (*_last_read_counter).store(counter, std::memory_order_relaxed);
>> + return ((*_upper_32bit_counter).load(std::memory_order_relaxed)
>> << 32) + counter;
>> + }
>> +
>> + static percpu<std::atomic<s64>> _upper_32bit_counter;
>> + static percpu<std::atomic<s64>> _last_read_counter;
>>
> +
>> + sched::cpu::notifier cpu_notifier;
>> };
>>
>> +PERCPU(std::atomic<s64>, hpet_32bit_clock::_upper_32bit_counter);
>> +PERCPU(std::atomic<s64>, hpet_32bit_clock::_last_read_counter);
>>
> +
>> class hpet_64bit_clock : public hpetclock {
>> public:
>> hpet_64bit_clock(mmioaddr_t hpet_mmio_address) :
>> hpetclock(hpet_mmio_address) {}
>> --
>> 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...@googlegroups.com <javascript:>.
>> To view this discussion on the web visit
>> https://groups.google.com/d/msgid/osv-dev/20190706033715.5981-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/0b6e5155-bd2a-444d-a48a-4c292b08d799%40googlegroups.com.
