From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933579AbbKMKeD (ORCPT ); Fri, 13 Nov 2015 05:34:03 -0500 Received: from mout.kundenserver.de ([212.227.17.10]:54761 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932563AbbKMKd6 (ORCPT ); Fri, 13 Nov 2015 05:33:58 -0500 From: Arnd Bergmann To: linux-arm-kernel@lists.infradead.org Cc: Jisheng Zhang , kernel@stlinux.com, srinivas.kandagatla@gmail.com, daniel.lezcano@linaro.org, patrice.chotard@st.com, linux-kernel@vger.kernel.org, tglx@linutronix.de, maxime.coquelin@st.com Subject: Re: [PATCH] clocksource/drivers/arm_global_timer: Always use {readl|writel}_relaxed Date: Fri, 13 Nov 2015 11:33:12 +0100 Message-ID: <25602407.8sIohphlWH@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20151113175948.69f610e9@xhacker> References: <1447403678-7217-1-git-send-email-jszhang@marvell.com> <10575502.sxpiT76bOp@wuerfel> <20151113175948.69f610e9@xhacker> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:1id6p4rbuI12RM/MY0sMXihRH2H6l9ShlusgB6E8o0zOXfoFy9B MoHm+0l8HAoCFYRWoOS3KUdjtlZjpMOjw03TcDi62sdLNx7wYmhGNJDus8uWlLSxaofSWQr cNPKc0DEDE3lKJZbUistMg7Yd4vURLKtNc7CHCq65jmkjjrBePF5Wx9pOSvKc4ae79desnA 4VEME31rW8FEMBCmzJs2g== X-UI-Out-Filterresults: notjunk:1;V01:K0:MLPP6aQEF4M=:Yg4Ak7ClA8MK/QWyBeS1yj 3IPalpRWWFOFyoYyi/la6+tuswU9AB4ZMjj54n84n0qxcOTFjEF6NHLS6+6u5cFFOlkps6gkJ vNXnMG7D98tE7fmXyOm1Vp0geT61G4TuhYpN8a8MkmYliBAZZDO+zi2GFiLcLtJ2lBaS8CeXD s7+QHNO7q7TjdMs8YcYYTTgD2uQo34o0QIC1r78yumTqUI52WzYo4G78Yps9E8hqNJGy4Rsp3 3VOq7kHgpzI2YCXp8NyP3kyB5metXFenZsB1jm+oszJeX9gstHjAyazCX40QpOlBTMhiL6Ug2 V9TmL21oZ0wU7qXWfSAqDgg9Sb2E1sf/tPDMS8uc1cmMr+hoLzeW6aTEsZpEiHZrtMwEU0FJH E07nM7n3yMbyCLog9D878zCkdIePF5CHfFRCbBNupT1yqOg1hm7IGMGqZwTS1xqTKFL/gVzrc ACaUmpaH5zayPckQkJrcP1q/bxNQu9pmljV+y6dfLKJJV/SNFd46ddD/KqqdSE8uRibDewPZ7 v17v6OsPm5PtEN0jUQHxqWfDU7uGliAedXLvFzWkz2AFqTH9lCVmRGcVdAOmA3A3BlX6y3Lk/ r6FfbjlP9H6u4F92VZuzoFDEGl43oIQKkQGx3k1dVpI2bu9nnvFGUFA11kkbay4vSJbyLJN73 1pawbOVny6Ch9uV6kNUF6u/zSIDmycDQOwRH3qEWPXC5W3wLTxLGxOjKrK54Ebj7bGcNvLzpe c2xHi8yzGEpXKBmj Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Friday 13 November 2015 17:59:48 Jisheng Zhang wrote: > On Fri, 13 Nov 2015 10:28:01 +0100 > Arnd Bergmann wrote: > > On Friday 13 November 2015 16:40:25 Jisheng Zhang wrote: > > > On Fri, 13 Nov 2015 16:34:38 +0800 > > > > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > > > > index a2cb6fa..84a5a5d 100644 > > > > --- a/drivers/clocksource/arm_global_timer.c > > > > +++ b/drivers/clocksource/arm_global_timer.c > > > > @@ -99,27 +99,27 @@ static void gt_compare_set(unsigned long delta, int periodic) > > > > > > > > counter += delta; > > > > ctrl = GT_CONTROL_TIMER_ENABLE; > > > > - writel(ctrl, gt_base + GT_CONTROL); > > > > - writel(lower_32_bits(counter), gt_base + GT_COMP0); > > > > - writel(upper_32_bits(counter), gt_base + GT_COMP1); > > > > + writel_relaxed(ctrl, gt_base + GT_CONTROL); > > > > + writel_relaxed(lower_32_bits(counter), gt_base + GT_COMP0); > > > > + writel_relaxed(upper_32_bits(counter), gt_base + GT_COMP1); > > > > > > > > if (periodic) { > > > > - writel(delta, gt_base + GT_AUTO_INC); > > > > + writel_relaxed(delta, gt_base + GT_AUTO_INC); > > > > ctrl |= GT_CONTROL_AUTO_INC; > > > > } > > > > > > > > ctrl |= GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE; > > > > - writel(ctrl, gt_base + GT_CONTROL); > > > > + writel_relaxed(ctrl, gt_base + GT_CONTROL); > > > > } > > > > This seems fine. Do you have any performance numbers to show how much > > we save per call on a platform you care about, and how often it is > > called for a typical workload? > > To be honest, all my platforms don't make use of global timer for clockevent, > we use dw_apb_timer and twd or arch_timer instead, but one performance impact > I saw in our case can also apply for the case with global timer as clokevent: > > there are 500-1000 short sleeps, yes not good userspace behavior, so we > program clockevent device 500-1000 times/s. If the system is powered by CA9 > with outer L2 cache, the writel will contend for l2x0_lock for 500-1000 times/s. > Then the L2 cache maintenance from other device driver have more chance to > spinning at the l2x0_lock, so other device driver performance is impacted. Just to make sure I get this right: which outer cache implementation do you use in this case? Most Cortex-A9 use pl310, which does not require l2x0_lock for outer_cache.sync(). The Aurora outer cache sync has a different method and also doesn't use l2x0_lock. Finally, tauros3 doesn't need a cache sync at all. Did you look at an older kernel version? We used to do a loop in the Aurora cache sync operation until I fixed that, so it should be a bit faster now. It will still require doing the actual sync, but at least there should not be any lock contention these days. Arnd