Hi tglx, > -----Original Message----- > From: Thomas Gleixner <[email protected]> > Sent: Wednesday, October 16, 2019 3:29 PM > To: Paolo Bonzini <[email protected]> > Cc: Jianyong Wu (Arm Technology China) <[email protected]>; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; Mark Rutland <[email protected]>; > [email protected]; Suzuki Poulose <[email protected]>; linux- > [email protected]; [email protected]; > [email protected]; [email protected]; Steve Capper > <[email protected]>; Kaly Xin (Arm Technology China) > <[email protected]>; Justin He (Arm Technology China) > <[email protected]>; nd <[email protected]> > Subject: Re: [PATCH v5 3/6] timekeeping: Add clocksource to > system_time_snapshot > > On Wed, 16 Oct 2019, Paolo Bonzini wrote: > > On 15/10/19 22:13, Thomas Gleixner wrote: > > > On Tue, 15 Oct 2019, Paolo Bonzini wrote: > > >> On 15/10/19 12:48, Jianyong Wu wrote: > > >>> > > >>> > > >> > > >> Reviewed-by: Paolo Bonzini <[email protected]> > > > > > > You're sure about having reviewed that in detail? > > > > I did review the patch; the void* ugliness is not in this one, and I > > do have some other qualms on that one. > > > > > This changelog is telling absolutely nothing WHY anything outside of > > > the timekeeping core code needs access to the current clocksource. > > > Neither does it tell why it is safe to provide the pointer to random > > > callers. > > > > Agreed on the changelog, but the pointer to a clocksource is already > > part of the timekeeping external API via struct system_counterval_t. > > get_device_system_crosststamp for example expects a clocksource > > pointer but provides no way to get such a pointer. > > That's a completely different beast, really. > > The clocksource pointer is handed in by the caller and the core code validates > if the clocksource is the same as the current system clocksource and not the > other way round. > > So there is no need for getting that pointer from the core code because the > caller knows already which clocksource needs to be active to make.the whole > cross device timestamp correlation work. And in that case it's the callers > responsibility to ensure that the pointer is valid which is the case for the > current use cases. > I thinks there is something misunderstanding of my patch. See patch 4/6, the reason why I add clocksource is that I want to check if the current clocksouce is arm_arch_counter in virt/kvm/arm/psci.c and nothing to do with get_device_system_crosststamp.
So I really need a mechanism to do that check. Thanks Jianyong > From your other reply: > > > Why add a global id? ARM can add it to archdata similar to how x86 > > has vclock_mode. But I still think the right thing to do is to > > include the full system_counterval_t in the result of > > ktime_get_snapshot. (More in a second, feel free to reply to the other > email only). > > No, the clocksource pointer is not going to be exposed as there is no > guarantee that it will be still around after the call returns. > > It's not even guaranteed to be correct when the store happens in Wu's patch > simply because the store is done outside of the seqcount protected region. Yeah, all of the elements in system_time_snapshot should be captured in consistency. So I think the consistency will be guaranteed if the store ops added in the seqcount region. > > Vs. arch data: arch data is an opaque struct, so you'd need to store a pointer > which has the same issue as the clocksource pointer itself. > > If we want to convey information then it has to be in the generic part of > struct clocksource. > > In fact we could even simplify the existing get_device_system_crosststamp() > use case by using the ID field. > > Thanks, > > tglx
