From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753796AbbETNJs (ORCPT ); Wed, 20 May 2015 09:09:48 -0400 Received: from www.linutronix.de ([62.245.132.108]:60646 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753660AbbETNJq (ORCPT ); Wed, 20 May 2015 09:09:46 -0400 Date: Wed, 20 May 2015 15:09:53 +0200 (CEST) From: Thomas Gleixner To: Viresh Kumar cc: linaro-kernel@lists.linaro.org, linux-kernel@vger.kernel.org, Ingo Molnar , Peter Zijlstra Subject: Re: [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel In-Reply-To: Message-ID: References: User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 20 May 2015, Viresh Kumar wrote: > Ingo suggested [1] to keep CLOCK_EVT_STATE_* symbols somewhere in > kernel/time/. We couldn't do it as bL_switcher code was using it > earlier. But that's fixed now. And so the first patch moves these > symbols to tick-internal.h. > > Some of the drivers [2] need to verify state of the clockevent device > from their callbacks or interrupt handlers. They look at clock_event_mode and not at state, right? > Because these symbols (defined by 'enum clock_event_state') will now be > internal to the core, we need some helpers to verify state of a > clockevent device. So how are they affected by moving clock_event_state to the core code ? > One way out was to maintain the state in drivers as well, but that would > be unnecessary burden on them. And so the second patch introduces > helpers for these states. And that way we add the overhead of a full function call to those drivers for the interrupt hot path? I just looked at the drivers and there are three classes of mode checks and CLOCK_EVT_MODE_ usage: 1) Interrupt handler: Act depending on the mode: - disable the device in case of oneshot vf_pit_timer timer-atlas7 sh_cmt sh_tmu rockchip_timer qcom-timer fsl_ftm_timer cs5535-clockevt arm_global_timer evt-sb1250 cevt-bcm1480 mips/jz4740 dc21285-timer arc/time alpha/time - conditionally handle the device in case of shared interrupt hardware with trainwreck design or other shortcomings of the timer hardware timer-atmel-pit cs5535-clockevt x86/apic 2) Sensible checks: i8253 init_pit_timer() x86/i8253 init_pit_clocksource() cs5535_mfgpt init_mfgpt_timer() avr32/time comparator_mode() 3) Use cases which can be fixed by conversion to the seperate mode setters x86/apic Calibration code 4) Simple to fix stuff nios2 nios2_timer_config() exynos_mct exynos4_mct_comp0_start() tcb_clksrc tc_mode() 5) Set mode from driver code: time-armada-370-xp armada_370_xp_timer_stop() qcom-timer msm_local_timer_stop() exynos_mct exynos4_local_timer_stop() arm_global_timer gt_clockevents_stop() arm_arch_timer arch_timer_stop() smp_twd twd_timer_stop() hyperv/hv hv_synic_cleanup() 6) Random nonsense: xen/time xen_timerop_set_next_event() xen_vcpuop_set_next_event() hyper/hv hv_ce_set_next_event() sh_cmt sh_cmt_clock_event_next() sh_tmu sh_tmu_clock_event_next() arm/mach-imx/time mxc_set_mode() arm/mach-imx/apit epit_set_mode() mxs_timer mxs_set_mode() That list is way more useful than a pastebin with random grep output, which does not cover use cases which SET a mode from driver code and does not cover local storage of modes. Of course you should have done that analysis before posting some random helper functions. Lets look how useful these functions are for the various use cases #1) Adds function call over head to the timer interrupt Hot path does matter and that function call is a regression. So that's a NONO #2) The function call overhead does not matter much for these, but they could be simply fixed by using local or device storage as well. #3) A non issue #4) Trivial to fix #5) Trivial to replace by the explicit setter functions, but we want to know WHY this is done in the first place #6) Simple to remove random crappola. I already did the patches while analysing the code, so that will be gone soon. Now explain me how your magic functions help. For most of the cases they would be a performance regression. And for the rest they really do not matter at all. Brilliant stuff that. Thanks, tglx