Subject: Re: [PATCH] clocksource: Store reg field within struct clocksource
To: Russell King - ARM Linux <linux@arm.linux.org.uk>,
        Thomas Gleixner <tglx@linutronix.de>
CC: Daniel Lezcano <daniel.lezcano@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>, Mans Rullgard <mans@mansr.com>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Nicolas Pitre <nico@linaro.org>, Tony Lindgren <tony@atomide.com>,
        Sebastian Frias <sebastian_frias@sigmadesigns.com>
References: <564C8086.80804@sigmadesigns.com>
 <20151118172114.GS8644@n2100.arm.linux.org.uk>
 <alpine.DEB.2.11.1511191130480.3898@nanos>
 <20151119103645.GV8644@n2100.arm.linux.org.uk>
From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Message-ID: <564DAAB2.6040706@sigmadesigns.com>
Date: Thu, 19 Nov 2015 11:55:46 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101
 Firefox/41.0 SeaMonkey/2.38
MIME-Version: 1.0
In-Reply-To: <20151119103645.GV8644@n2100.arm.linux.org.uk>
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2718
Lines: 70

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 ____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 can then be grouped with other hot data. A nice side-effect
>>>> of this patch is making container_of() unnecessary, which makes the
>>>> code a bit simpler.
>>>>
>>>> On 32-bit platforms, reg fits in the padding between read and mask,
>>>> meaning no downside from storing it there.
>>>
>>> Just swap the order of 'reg' and 'clksrc'.
>>
>> That might reduce the memory footprint, but it does not bring the
>> iomem pointer closer to the other hotpath clocksource data. So we
>> still need to touch at minimum two cache lines for reading the time.
>>
>> With Marc's change we have all hotpath data in a single cacheline.
> 
> Right, and what it's doing is polluting struct clocksource with lots
> of ifdefs which determine how much data is contained in there.  Seems
> to me to be totally insane.

What I find puzzling is that you would consider the read field to be
a bona fide member of struct clocksource, but reg (the address passed
to read) does not belong inside the struct?

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
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -69,6 +69,7 @@ struct clocksource {
 	 * clocksource itself is cacheline aligned.
 	 */
 	cycle_t (*read)(struct clocksource *cs);
+	void __iomem *reg;
 	cycle_t mask;
 	u32 mult;
 	u32 shift;

> The basic cause of this problem is the ____cacheline_aligned annotation
> which effectively prevents wrapping struct clocksource to provide
> implementation specific data.

True. But note that placing reg inside struct clocksource comes
for free on 32-bit platforms (it just replaces padding).

> Maybe your idea is that struct clocksource should be bloated with all
> implementation specific data in the long term?

reg is not implementation-specific data, right?

Regards.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/