From: Vineet Gupta <Vineet.Gupta1@synopsys.com> To: Daniel Lezcano <daniel.lezcano@linaro.org> Cc: <tglx@linutronix.de>, <linux-kernel@vger.kernel.org>, <linux-snps-arc@lists.infradead.org>, Noam Camus <noamca@mellanox.com>, <Alexey.Brodkin@synopsys.com> Subject: Re: [PATCH 9/9] clocksource: import ARC timer driver Date: Tue, 1 Nov 2016 18:03:06 -0700 [thread overview] Message-ID: <2dcf87ce-a278-8ef8-3c8e-3cf7124603c9@synopsys.com> (raw) In-Reply-To: <20161102001925.GH1506@mai> On 11/01/2016 05:19 PM, Daniel Lezcano wrote: >>> >>> One question: >>> >>> Why ARC_TIMER_RTC can't be used in a SMP system ? Doesn't have each core its >>> own clocksource ? It seems you are assuming a clocksource can be used on SMP >>> only if the clocksource is unique and shared across the cores. >> >> Thats what I thought so far. Thing is, the individual core's counters could get >> out of sync, simply because non masters cores were halted to begin with and came >> up at different points in real time. so a gtod might return different value >> depending on what core it landed on. Does clocksource also does ticks broadcasts >> and such to keep things in sync ? > > Sounds like it is similar than the TSC. Do you agree to have a try by setting > the CONFIG_HAVE_UNSTABLE_SCHED_CLOCK option ? I'm not sure why we would want to enable extra stuff - I see work queues and bunch of per cpu counting / math to adjust for the variance, if this was enabled. Anyhow see more below. > If you can use those per cpu clocksource, performances on your system may > improve with the sched_clock(). Couple of things 1. Currently we don't hookup sched clock to any counter at all (on my todo list for a while). So we only get jiffies64 based value - I know that sucks - causes scheduling to be not super accurate etc - potentially affects benchmarks etc - but that can be fixed easily / independent of this. 2. Say we did have sched_clock() driven by hardware - in SMP system I would still prefer it to be driven by "common" GFRC and not "per cpu" RTC. The overhead of HAVE_UNSTABLE_SCHED_CLOCK looks way way more than reading GFRC counter like this. local_irq_save(flags); __mcip_cmd(CMD_GFRC_READ_LO, 0); stamp.l = read_aux_reg(ARC_REG_MCIP_READBACK); __mcip_cmd(CMD_GFRC_READ_HI, 0); stamp.h = read_aux_reg(ARC_REG_MCIP_READBACK); local_irq_restore(flags); GFRC reading by 2 cores concurrently doesn't require any synchronization at all. The irq disabling around it is to make sure we didn't get a bogus readout lest an interrupt came in between the read of 2 words. But if sched_clock can guarantee that irqs are disable - I can probably even remove it at least for the purpose of sched clock. However I think we are digressing here a bit. IMHO, what clock we choose to drive sched should not really be driven by the driver. It must be for the arch to decide. We should first focus on how the clockevent/sources are programmed first and then dive into sched_clock_xx as that doesn't exist at the moment for ARC. > >> Because of the git mv you, diff didn't include bulk of driver code which would >> make for bulk of review anyways. So perhaps in v2 I don't do the git mv. OK ? > > That means I will review and comment existing code. It is not a problem for me > if you agree to do the changes. Sure, the whole point is to make things better as an outcome of review. I have no issues changing code provided we don't add major performance regressions. Thx, -Vineet
WARNING: multiple messages have this Message-ID (diff)
From: Vineet.Gupta1@synopsys.com (Vineet Gupta) To: linux-snps-arc@lists.infradead.org Subject: [PATCH 9/9] clocksource: import ARC timer driver Date: Tue, 1 Nov 2016 18:03:06 -0700 [thread overview] Message-ID: <2dcf87ce-a278-8ef8-3c8e-3cf7124603c9@synopsys.com> (raw) In-Reply-To: <20161102001925.GH1506@mai> On 11/01/2016 05:19 PM, Daniel Lezcano wrote: >>> >>> One question: >>> >>> Why ARC_TIMER_RTC can't be used in a SMP system ? Doesn't have each core its >>> own clocksource ? It seems you are assuming a clocksource can be used on SMP >>> only if the clocksource is unique and shared across the cores. >> >> Thats what I thought so far. Thing is, the individual core's counters could get >> out of sync, simply because non masters cores were halted to begin with and came >> up at different points in real time. so a gtod might return different value >> depending on what core it landed on. Does clocksource also does ticks broadcasts >> and such to keep things in sync ? > > Sounds like it is similar than the TSC. Do you agree to have a try by setting > the CONFIG_HAVE_UNSTABLE_SCHED_CLOCK option ? I'm not sure why we would want to enable extra stuff - I see work queues and bunch of per cpu counting / math to adjust for the variance, if this was enabled. Anyhow see more below. > If you can use those per cpu clocksource, performances on your system may > improve with the sched_clock(). Couple of things 1. Currently we don't hookup sched clock to any counter at all (on my todo list for a while). So we only get jiffies64 based value - I know that sucks - causes scheduling to be not super accurate etc - potentially affects benchmarks etc - but that can be fixed easily / independent of this. 2. Say we did have sched_clock() driven by hardware - in SMP system I would still prefer it to be driven by "common" GFRC and not "per cpu" RTC. The overhead of HAVE_UNSTABLE_SCHED_CLOCK looks way way more than reading GFRC counter like this. local_irq_save(flags); __mcip_cmd(CMD_GFRC_READ_LO, 0); stamp.l = read_aux_reg(ARC_REG_MCIP_READBACK); __mcip_cmd(CMD_GFRC_READ_HI, 0); stamp.h = read_aux_reg(ARC_REG_MCIP_READBACK); local_irq_restore(flags); GFRC reading by 2 cores concurrently doesn't require any synchronization at all. The irq disabling around it is to make sure we didn't get a bogus readout lest an interrupt came in between the read of 2 words. But if sched_clock can guarantee that irqs are disable - I can probably even remove it at least for the purpose of sched clock. However I think we are digressing here a bit. IMHO, what clock we choose to drive sched should not really be driven by the driver. It must be for the arch to decide. We should first focus on how the clockevent/sources are programmed first and then dive into sched_clock_xx as that doesn't exist at the moment for ARC. > >> Because of the git mv you, diff didn't include bulk of driver code which would >> make for bulk of review anyways. So perhaps in v2 I don't do the git mv. OK ? > > That means I will review and comment existing code. It is not a problem for me > if you agree to do the changes. Sure, the whole point is to make things better as an outcome of review. I have no issues changing code provided we don't add major performance regressions. Thx, -Vineet
next prev parent reply other threads:[~2016-11-02 1:03 UTC|newest] Thread overview: 95+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-10-23 12:12 [PATCH v2 0/3] Add clockevet for timer-nps driver to NPS400 SoC Noam Camus 2016-10-23 12:12 ` Noam Camus 2016-10-23 12:12 ` [PATCH v2 1/3] soc: Support for NPS HW scheduling Noam Camus 2016-10-23 12:12 ` Noam Camus 2016-10-31 10:26 ` Daniel Lezcano 2016-10-31 10:26 ` Daniel Lezcano 2016-10-31 12:26 ` Noam Camus 2016-10-23 12:12 ` [PATCH v2 2/3] clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer Noam Camus 2016-10-23 12:12 ` Noam Camus 2016-10-31 10:28 ` Daniel Lezcano 2016-10-31 10:28 ` Daniel Lezcano 2016-10-31 15:19 ` Noam Camus 2016-10-23 12:12 ` [PATCH v2 3/3] clocksource: Add clockevent support to NPS400 driver Noam Camus 2016-10-23 12:12 ` Noam Camus 2016-10-30 20:41 ` Rob Herring 2016-10-31 10:52 ` Daniel Lezcano 2016-10-31 17:03 ` Noam Camus 2016-10-31 17:51 ` Vineet Gupta 2016-10-31 22:48 ` [PATCH 0/9] Move ARC timer code into drivers/clocksource/ Vineet Gupta 2016-10-31 22:48 ` Vineet Gupta 2016-10-31 22:48 ` [PATCH 1/9] ARC: timer: gfrc, rtc: Read BCR to detect whether hardware exists Vineet Gupta 2016-10-31 22:48 ` Vineet Gupta 2016-11-03 17:00 ` Daniel Lezcano 2016-11-03 17:00 ` Daniel Lezcano 2016-11-03 17:41 ` Vineet Gupta 2016-11-03 17:41 ` Vineet Gupta 2016-10-31 22:48 ` [PATCH 2/9] ARC: timer: rtc: implement read loop in "C" vs. inline asm Vineet Gupta 2016-10-31 22:48 ` Vineet Gupta 2016-11-03 17:02 ` Daniel Lezcano 2016-11-03 17:02 ` Daniel Lezcano 2016-11-03 17:45 ` Vineet Gupta 2016-11-03 17:45 ` Vineet Gupta 2016-10-31 22:48 ` [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers Vineet Gupta 2016-10-31 22:48 ` Vineet Gupta 2016-11-03 17:09 ` Daniel Lezcano 2016-11-03 17:09 ` Daniel Lezcano 2016-11-03 17:47 ` Vineet Gupta 2016-11-03 17:47 ` Vineet Gupta 2016-11-03 17:51 ` Daniel Lezcano 2016-11-03 17:51 ` Daniel Lezcano 2016-10-31 22:48 ` [PATCH 4/9] ARC: time: move time_init() out of the driver Vineet Gupta 2016-10-31 22:48 ` Vineet Gupta 2016-11-03 17:15 ` Daniel Lezcano 2016-11-03 17:15 ` Daniel Lezcano 2016-10-31 22:48 ` [PATCH 5/9] ARC: breakout aux handling into a seperate header Vineet Gupta 2016-10-31 22:48 ` Vineet Gupta 2016-11-01 7:49 ` Noam Camus 2016-11-01 7:49 ` Noam Camus 2016-10-31 22:48 ` [PATCH 6/9] ARC: move mcip.h into include/soc and adjust the includes Vineet Gupta 2016-10-31 22:48 ` Vineet Gupta 2016-11-03 17:20 ` Daniel Lezcano 2016-11-03 17:20 ` Daniel Lezcano 2016-10-31 22:48 ` [PATCH 7/9] ARC: breakout timer stuff into a seperate header Vineet Gupta 2016-10-31 22:48 ` Vineet Gupta 2016-11-03 17:25 ` Daniel Lezcano 2016-11-03 17:25 ` Daniel Lezcano 2016-10-31 22:48 ` [PATCH 8/9] ARC: timer: rename config options Vineet Gupta 2016-10-31 22:48 ` Vineet Gupta 2016-10-31 22:48 ` [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta 2016-10-31 22:48 ` Vineet Gupta 2016-11-01 0:01 ` kbuild test robot 2016-11-01 0:01 ` kbuild test robot 2016-11-01 0:45 ` Vineet Gupta 2016-11-01 0:45 ` Vineet Gupta 2016-11-01 20:42 ` Daniel Lezcano 2016-11-01 20:42 ` Daniel Lezcano 2016-11-01 20:57 ` Vineet Gupta 2016-11-01 20:57 ` Vineet Gupta 2016-11-02 0:19 ` Daniel Lezcano 2016-11-02 0:19 ` Daniel Lezcano 2016-11-02 1:03 ` Vineet Gupta [this message] 2016-11-02 1:03 ` Vineet Gupta 2016-11-03 16:40 ` Vineet Gupta 2016-11-03 16:40 ` Vineet Gupta 2016-11-03 16:50 ` Daniel Lezcano 2016-11-03 16:50 ` Daniel Lezcano 2016-11-03 17:57 ` Vineet Gupta 2016-11-03 17:57 ` Vineet Gupta 2016-11-03 18:11 ` Daniel Lezcano 2016-11-03 18:11 ` Daniel Lezcano 2016-11-03 18:43 ` Vineet Gupta 2016-11-03 18:43 ` Vineet Gupta 2016-11-03 17:33 ` Daniel Lezcano 2016-11-03 17:33 ` Daniel Lezcano 2016-11-03 18:14 ` Daniel Lezcano 2016-11-03 18:14 ` Daniel Lezcano 2016-11-03 18:47 ` Vineet Gupta 2016-11-03 18:47 ` Vineet Gupta 2016-11-03 17:28 ` [PATCH 0/9] Move ARC timer code into drivers/clocksource/ Daniel Lezcano 2016-11-03 17:28 ` Daniel Lezcano 2016-11-01 20:01 ` [PATCH v2 3/3] clocksource: Add clockevent support to NPS400 driver Daniel Lezcano 2016-11-01 20:01 ` Daniel Lezcano 2016-11-08 8:30 ` Noam Camus 2016-11-10 10:34 ` Daniel Lezcano 2016-11-10 13:00 ` Noam Camus
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2dcf87ce-a278-8ef8-3c8e-3cf7124603c9@synopsys.com \ --to=vineet.gupta1@synopsys.com \ --cc=Alexey.Brodkin@synopsys.com \ --cc=daniel.lezcano@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-snps-arc@lists.infradead.org \ --cc=noamca@mellanox.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.