On Mon, Nov 14, 2016 at 01:58:15PM +0000, Noam Camus wrote:

quoted

From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
Sent: Monday, November 14, 2016 1:23 PM

quoted

quoted

+ */
+static void nps_clkevent_rm_thread(bool remove_thread) {
+	unsigned int cflags;
+	unsigned int enabled_threads = 0;
+	int thread;
+
+	hw_schd_save(&cflags);

quoted

I'm not used with hardware scheduling. Can you explain why this is needed
here ? What >window race we want to close ?

We are using HW scheduling off/on in order to keep consistency of auxiliary
registers shared among HW threads within the same core.  Example to such
registers NPS_REG_TIMER0_TSI and NPS_REG_TIMER0_CTRL.  Since update procedure
of these registers is not atomic we use save/restore macros to turn off/on
the HW scheduling. This way we insure that no HW scheduling occurs and
another HW thread (represented as another CPU) will execute in this same
critical code path.  If we take for example nps_clkevent_add_thread() we can
see that we are doing some read modify write to NPS_REG_TIMER0_TSI and
optionally writing to NPS_REG_TIMER0_CTRL. This flow should be atomic and is
protected by our save/restore macros.  Do note that interrupts are disabled
at this point so we are safe from all asynchronous events. 

 
The function nps_clkevent_timer_event_setup() writes into the NPS_REG_TIMER0_CTRL
register but there is no critical section there. What prevents another HW thread
to write this register at the same time ?

I do believe we have a framework to access shared registers, otherwise a simple
spinlock would be simpler and perhaps faster than disabling the entire hardware
scheduling for the system, no ?

quoted

quoted

+static void nps_clkevent_add_thread(bool set_event) {
+	int thread;
+	unsigned int cflags, enabled_threads;
+
+	hw_schd_save(&cflags);
+
+	/* add thread to TSI1 */
+	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
+	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
+	enabled_threads |= (1 << thread);
+	write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
+
+	/* set next timer event */
+	if (set_event)
+		write_aux_reg(NPS_REG_TIMER0_CTRL,
+			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+
+	hw_schd_restore(cflags);
+}

quoted

Not sure the boolean parameters for *_rm_thread and *_add_thread helps to
clarify the code. Depending on the race window with hw_schd_save/restore We
should be able to simplify it.

I am not sure I am following you here, how race window may simplify this
code?  If those routines will get no parameter I can't determine when to add
or not (same as remove).  ...

Regarding the comment I did above, it is possible the critical section is
reduced and moved into the shutdown function. Thus, the boolean wouldn't be
needed anymore, well that is conditional to the above comment. Discard the
comment for the moment, until the hw sched vs spinlock vs NPS_REG_TIMER0_CTRL
is sorted out.

quoted

quoted

+static DEFINE_PER_CPU(struct clock_event_device, nps_clockevent_device) = {
+	.name				=	"NPS Timer0",
+	.features			=	CLOCK_EVT_FEAT_ONESHOT |
+						CLOCK_EVT_FEAT_PERIODIC,
+	.rating				=	300,
+	.set_next_event			=	nps_clkevent_set_next_event,
+	.set_state_periodic		=	nps_clkevent_set_periodic,
+	.set_state_oneshot		=	nps_clkevent_set_oneshot,
+	.set_state_oneshot_stopped	=	nps_clkevent_timer_shutdown,
+	.set_state_shutdown		=	nps_clkevent_timer_shutdown,

quoted

Doesn't set_state_shutdown and set_state_oneshot_stopped need to remove the HW thread from the TSI ?

You are correct, I will fix that.

And tick_resume. Perhaps, that is the reason why NO_HZ hangs.

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog