Re: [PATCH] clocksource: Store reg field within struct clocksource

On 19/11/2015 14:57, Thomas Gleixner wrote: > On Thu, 19 Nov 2015, Marc Gonzalez wrote: >> On 19/11/2015 12:14, Thomas Gleixner wrote: >> >>> So yes, the alignment of the clocksource struct is not longer >>> relevant. The case where we access clocksource->max_cycles is when >>>

Re: [PATCH] clocksource: Store reg field within struct clocksource

On 19/11/2015 14:57, Thomas Gleixner wrote: > On Thu, 19 Nov 2015, Marc Gonzalez wrote: >> On 19/11/2015 12:14, Thomas Gleixner wrote: >> >>> So yes, the alignment of the clocksource struct is not longer >>> relevant. The case where we access clocksource->max_cycles is when >>>

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, 19 Nov 2015, Marc Gonzalez wrote: > On 19/11/2015 12:14, Thomas Gleixner wrote: > > > So yes, the alignment of the clocksource struct is not longer > > relevant. The case where we access clocksource->max_cycles is when > > CONFIG_DEBUG_TIMEKEEPING is enabled, which imposes worse

Re: [PATCH] clocksource: Store reg field within struct clocksource

On 19/11/2015 12:21, Russell King wrote: > When I wrote the MMIO clocksource implementation, there was no > cacheline_aligned on struct clocksource, and the arrangement I came > to for the structure put the 'reg' and 'read' within the same cache line > (note that the MMIO clocksource

Re: [PATCH] clocksource: Store reg field within struct clocksource

On 19/11/2015 12:14, Thomas Gleixner wrote: > So yes, the alignment of the clocksource struct is not longer > relevant. The case where we access clocksource->max_cycles is when > CONFIG_DEBUG_TIMEKEEPING is enabled, which imposes worse performance > problems to timekeeping than the extra

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, Nov 19, 2015 at 11:55:46AM +0100, Marc Gonzalez wrote: > If you just object to the ifdef, then perhaps 'reg' can be included > unconditionally. > > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h > index 278dd279a7a8..50725fd23ab0 100644 > ---

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > On Thu, Nov 19, 2015 at 11:42:48AM +0100, Thomas Gleixner wrote: > > On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > > > The basic cause of this problem is the cacheline_aligned annotation > > > which effectively prevents wrapping

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, Nov 19, 2015 at 11:42:48AM +0100, Thomas Gleixner wrote: > On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > > The basic cause of this problem is the cacheline_aligned annotation > > which effectively prevents wrapping struct clocksource to provide > > implementation specific

Re: [PATCH] clocksource: Store reg field within struct clocksource

On 19/11/2015 11:36, Russell King - ARM Linux wrote: > On Thu, Nov 19, 2015 at 11:33:47AM +0100, Thomas Gleixner wrote: >> Russell, >> >> On Wed, 18 Nov 2015, Russell King - ARM Linux wrote: >> >>> On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: Since 'struct clocksource' is

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > The basic cause of this problem is the cacheline_aligned annotation > which effectively prevents wrapping struct clocksource to provide > implementation specific data. > > Maybe your idea is that struct clocksource should be bloated with

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, Nov 19, 2015 at 11:36:53AM +0100, Thomas Gleixner wrote: > On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > > Maybe the cacheline_aligned is inappropriate then, because it means > > any wrapping of struct clocksource has exactly the same problem. > > We could do that, but that

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > On Thu, Nov 19, 2015 at 10:27:33AM +0100, Marc Gonzalez wrote: > > On 18/11/2015 18:21, Russell King - ARM Linux wrote: > > > On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > > > > > >> Since 'struct clocksource' is

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, Nov 19, 2015 at 11:33:47AM +0100, Thomas Gleixner wrote: > Russell, > > On Wed, 18 Nov 2015, Russell King - ARM Linux wrote: > > > On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > > > Since 'struct clocksource' is cacheline_aligned, gcc must insert > > > a lot of

Re: [PATCH] clocksource: Store reg field within struct clocksource

Russell, On Wed, 18 Nov 2015, Russell King - ARM Linux wrote: > On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > > Since 'struct clocksource' is cacheline_aligned, gcc must insert > > a lot of padding between reg and clksrc in 'struct clocksource_mmio' > > (for example,

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, Nov 19, 2015 at 10:27:33AM +0100, Marc Gonzalez wrote: > On 18/11/2015 18:21, Russell King - ARM Linux wrote: > > On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > > > >> Since 'struct clocksource' is cacheline_aligned, gcc must insert > >> a lot of padding between reg

Re: [PATCH] clocksource: Store reg field within struct clocksource

On 18/11/2015 18:21, Russell King - ARM Linux wrote: > On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > >> Since 'struct clocksource' is cacheline_aligned, gcc must insert >> a lot of padding between reg and clksrc in 'struct clocksource_mmio' >> (for example, L1_CACHE_BYTES =

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, Nov 19, 2015 at 10:27:33AM +0100, Marc Gonzalez wrote: > On 18/11/2015 18:21, Russell King - ARM Linux wrote: > > On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > > > >> Since 'struct clocksource' is cacheline_aligned, gcc must insert > >> a lot of padding between reg

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, Nov 19, 2015 at 11:33:47AM +0100, Thomas Gleixner wrote: > Russell, > > On Wed, 18 Nov 2015, Russell King - ARM Linux wrote: > > > On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > > > Since 'struct clocksource' is cacheline_aligned, gcc must insert > > > a lot of

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > On Thu, Nov 19, 2015 at 10:27:33AM +0100, Marc Gonzalez wrote: > > On 18/11/2015 18:21, Russell King - ARM Linux wrote: > > > On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > > > > > >> Since 'struct clocksource' is

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, Nov 19, 2015 at 11:36:53AM +0100, Thomas Gleixner wrote: > On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > > Maybe the cacheline_aligned is inappropriate then, because it means > > any wrapping of struct clocksource has exactly the same problem. > > We could do that, but that

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > The basic cause of this problem is the cacheline_aligned annotation > which effectively prevents wrapping struct clocksource to provide > implementation specific data. > > Maybe your idea is that struct clocksource should be bloated with

Re: [PATCH] clocksource: Store reg field within struct clocksource

On 18/11/2015 18:21, Russell King - ARM Linux wrote: > On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > >> Since 'struct clocksource' is cacheline_aligned, gcc must insert >> a lot of padding between reg and clksrc in 'struct clocksource_mmio' >> (for example, L1_CACHE_BYTES =

Re: [PATCH] clocksource: Store reg field within struct clocksource

On 19/11/2015 12:21, Russell King wrote: > When I wrote the MMIO clocksource implementation, there was no > cacheline_aligned on struct clocksource, and the arrangement I came > to for the structure put the 'reg' and 'read' within the same cache line > (note that the MMIO clocksource

Re: [PATCH] clocksource: Store reg field within struct clocksource

On 19/11/2015 12:14, Thomas Gleixner wrote: > So yes, the alignment of the clocksource struct is not longer > relevant. The case where we access clocksource->max_cycles is when > CONFIG_DEBUG_TIMEKEEPING is enabled, which imposes worse performance > problems to timekeeping than the extra

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > On Thu, Nov 19, 2015 at 11:42:48AM +0100, Thomas Gleixner wrote: > > On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > > > The basic cause of this problem is the cacheline_aligned annotation > > > which effectively prevents wrapping

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, Nov 19, 2015 at 11:42:48AM +0100, Thomas Gleixner wrote: > On Thu, 19 Nov 2015, Russell King - ARM Linux wrote: > > The basic cause of this problem is the cacheline_aligned annotation > > which effectively prevents wrapping struct clocksource to provide > > implementation specific

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, Nov 19, 2015 at 11:55:46AM +0100, Marc Gonzalez wrote: > If you just object to the ifdef, then perhaps 'reg' can be included > unconditionally. > > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h > index 278dd279a7a8..50725fd23ab0 100644 > ---

Re: [PATCH] clocksource: Store reg field within struct clocksource

Russell, On Wed, 18 Nov 2015, Russell King - ARM Linux wrote: > On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > > Since 'struct clocksource' is cacheline_aligned, gcc must insert > > a lot of padding between reg and clksrc in 'struct clocksource_mmio' > > (for example,

Re: [PATCH] clocksource: Store reg field within struct clocksource

On 19/11/2015 11:36, Russell King - ARM Linux wrote: > On Thu, Nov 19, 2015 at 11:33:47AM +0100, Thomas Gleixner wrote: >> Russell, >> >> On Wed, 18 Nov 2015, Russell King - ARM Linux wrote: >> >>> On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: Since 'struct clocksource' is

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Thu, 19 Nov 2015, Marc Gonzalez wrote: > On 19/11/2015 12:14, Thomas Gleixner wrote: > > > So yes, the alignment of the clocksource struct is not longer > > relevant. The case where we access clocksource->max_cycles is when > > CONFIG_DEBUG_TIMEKEEPING is enabled, which imposes worse

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > Since 'struct clocksource' is cacheline_aligned, gcc must insert > a lot of padding between reg and clksrc in 'struct clocksource_mmio' > (for example, L1_CACHE_BYTES = 64 on ARMv7). > > Storing reg within 'struct clocksource'

Re: [PATCH] clocksource: Store reg field within struct clocksource

Marc Gonzalez writes: > Since 'struct clocksource' is cacheline_aligned, gcc must insert > a lot of padding between reg and clksrc in 'struct clocksource_mmio' > (for example, L1_CACHE_BYTES = 64 on ARMv7). > > Storing reg within 'struct clocksource' removes unnecessary padding, > and reg

Re: [PATCH] clocksource: Store reg field within struct clocksource

On Wed, Nov 18, 2015 at 02:43:34PM +0100, Marc Gonzalez wrote: > Since 'struct clocksource' is cacheline_aligned, gcc must insert > a lot of padding between reg and clksrc in 'struct clocksource_mmio' > (for example, L1_CACHE_BYTES = 64 on ARMv7). > > Storing reg within 'struct clocksource'

Re: [PATCH] clocksource: Store reg field within struct clocksource

Marc Gonzalez writes: > Since 'struct clocksource' is cacheline_aligned, gcc must insert > a lot of padding between reg and clksrc in 'struct clocksource_mmio' > (for example, L1_CACHE_BYTES = 64 on ARMv7). > > Storing reg within 'struct clocksource' removes