Re: [PATCH v5 2/2] clocksource: add J-Core timer/clocksource driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Thu, Jul 28, 2016 at 06:44:15PM +0200, Thomas Gleixner wrote:
> On Thu, 28 Jul 2016, Rich Felker wrote:
> > On Thu, Jul 28, 2016 at 04:44:05PM +0200, Thomas Gleixner wrote:
> > > > +	return ((u64)sechi << 32 | seclo) * NSEC_PER_SEC + nsec;
> > > 
> > > Wow, that's an expensive thing for a hotpath operation. You really don't have
> > > binary readout register for that clock thingy?
> > 
> > Unforunately the clock is in sec64.nsec32 format instead of a flat
> > nanoseconds count. Daniel Lezcano also suggested just using
> > nanoseconds, which would still need some retry and arithmetic for
> > adding seclo*NSEC_PER_SEC (otherwise it's problematic because it wraps
> > at NSEC_PER_SEC rather than at a power of two) but that does put a
> > hard upper bound on tickless sleep time of 4 sec. In practice it
> > probably doesn't matter. Should I try that instead?
> 
> Up to you. I was just wondering about the MUL.

Well 2 people have raised the issue now so I think I'll go ahead and
try changing it.

> > > > +static notrace u64 jcore_sched_clock_read(void)
> > > > +{
> > > > +	return jcore_clocksource_read(jcore_cs);
> > > 
> > > Why don't you stuff the above code into this function?
> > 
> > I was trying to avoid code duplication, but I could if you think it
> > really matters for performance.
> 
> Did not see where the other usage site was. Must have missed that. So keep it
> as is and please add notrace to jcore_clocksource_read().

Since we have to assume a singleton now anyway, I'm just going to
switch the caller/callee relationship the other way around and make
the sched version use static objects directly. This shought be
slightly faster/smaller.

> > > Please convert this to the new state machine model of cpu
> > > hotplug. CPU_STARTING will be gone very soon. Here is an example:
> > > 
> > > http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=smp/hotplug&id=7e86e8bd8dd67649d176e08d8dfb90039f0a1e98
> > 
> > I don't think that's the commit you wanted to link -- it doesn't show
> > a usage example.
>  
> It's the conversion of a driver from the old to the new style, So it's an
> example how to move your stuff to the new interface or am I missing something
> here?

I botched the link copy and paste. Sorry for the noise. I see it now.

> > I've asked about this before in another context but didn't get an
> > answer -- I'm a bit concerned that, from what I can tell, the new
> > framework is a big singleton does assumes singletons in all the
> > drivers that use it. In practice it doesn't matter as long as there's
> > only one instance of the pit driver, but this seems architecturally
> > really bad and like it's a time bomb waiting to blow up on us in the
> > future. Am I missing something?
> 
> Most of the users are single instance. We have a dynamic interface for the
> online callbacks and we might get one for the prep stage as well.
> 
> Now for the real core stuff like starting/dying we want explicit states and if
> a driver really is multi instance then having a private list there is not
> rocket science. Most of them have an instance list anyway.

For now I'll just remove some of the dynamic allocation I added and
simplify assuming a single instance. It doesn't make sense to be
incurring costs for the nominal ability to have multiple instances
when it doesn't actually work in the new framework (without a lot more
per-driver code duplication that really should be in the framework)
and when it's not needed anyway.

Rich
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux