Hi tglx, > -----Original Message----- > From: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > Sent: Wednesday, October 16, 2019 3:29 PM > To: Paolo Bonzini <pbonzini@xxxxxxxxxx> > Cc: Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>; > netdev@xxxxxxxxxxxxxxx; yangbo.lu@xxxxxxx; john.stultz@xxxxxxxxxx; > sean.j.christopherson@xxxxxxxxx; maz@xxxxxxxxxx; > richardcochran@xxxxxxxxx; Mark Rutland <Mark.Rutland@xxxxxxx>; > will@xxxxxxxxxx; Suzuki Poulose <Suzuki.Poulose@xxxxxxx>; linux- > kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > kvmarm@xxxxxxxxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Steve Capper > <Steve.Capper@xxxxxxx>; Kaly Xin (Arm Technology China) > <Kaly.Xin@xxxxxxx>; Justin He (Arm Technology China) > <Justin.He@xxxxxxx>; nd <nd@xxxxxxx> > 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 <pbonzini@xxxxxxxxxx> > > > > > > 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
