Re: [Xen-devel] [PATCH 3/5] xen: RCU/x86/ARM: discount CPUs that were idle when grace period started.

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Dario Faggioli <dario.faggioli@xxxxxxxxxx> 07/27/17 10:01 AM >>>
>--- a/xen/arch/x86/acpi/cpu_idle.c
>+++ b/xen/arch/x86/acpi/cpu_idle.c
>@@ -418,14 +418,16 @@ static void acpi_processor_ffh_cstate_enter(struct 
>acpi_processor_cx *cx)
>mwait_idle_with_hints(cx->address, MWAIT_ECX_INTERRUPT_BREAK);
>}
 >
>-static void acpi_idle_do_entry(struct acpi_processor_cx *cx)
>+static void acpi_idle_do_entry(unsigned int cpu, struct acpi_processor_cx *cx)
>{
>+    rcu_idle_enter(cpu);
>+
>switch ( cx->entry_method )
>{
>case ACPI_CSTATE_EM_FFH:
>/* Call into architectural FFH based C-state */
>acpi_processor_ffh_cstate_enter(cx);
>-        return;
>+        break;
>case ACPI_CSTATE_EM_SYSIO:
         >/* IO port based C-state */
>inb(cx->address);
>@@ -433,12 +435,14 @@ static void acpi_idle_do_entry(struct acpi_processor_cx 
>*cx)
>because chipsets cannot guarantee that STPCLK# signal
>gets asserted in time to freeze execution properly. */
>inl(pmtmr_ioport);
>-        return;
>+        break;
     >case ACPI_CSTATE_EM_HALT:

Wiuld be nice if you also added blank lines between the break-s and the
subsequent case labels.

>@@ -756,6 +759,8 @@ static void mwait_idle(void)
>return;
>}
 >
>+    rcu_idle_enter(cpu);
>+
>eax = cx->address;
>cstate = ((eax >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
 >
>@@ -787,6 +792,8 @@ static void mwait_idle(void)
>irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]);
 >
>/* Now back in C0. */
>+    rcu_idle_exit(cpu);
>+
>update_idle_stats(power, cx, before, after);
>local_irq_enable();
 
Compared to the ACPI code, the window is quite a bit larger here. Any reason
you can't put these just around the mwait_idle_with_hints() invocation, which
would match what you do for the ACPI-based driver?

>@@ -248,7 +249,14 @@ static void rcu_start_batch(struct rcu_ctrlblk *rcp)
>smp_wmb();
>rcp->cur++;
 >
>-        cpumask_copy(&rcp->cpumask, &cpu_online_map);
>+       /*
>+        * Accessing idle_cpumask before incrementing rcp->cur needs a
>+        * Barrier  Otherwise it can cause tickless idle CPUs to be
>+        * included in rcp->cpumask, which will extend graceperiods
>+        * unnecessarily.
>+        */
>+        smp_mb();
>+        cpumask_andnot(&rcp->cpumask, &cpu_online_map, &rcp->idle_cpumask);

I have some trouble with understanding the comment: You don't access
->idle_cpumask before you increment ->cur. (Also, as a general remark,
please go through patch description and comments again to correct
spelling and alike issues.)

>@@ -474,7 +482,23 @@ static struct notifier_block cpu_nfb = {
>void __init rcu_init(void)
>{
>void *cpu = (void *)(long)smp_processor_id();
>+
>+    cpumask_setall(&rcu_ctrlblk.idle_cpumask);

The CPU you're running on surely is not idle initially?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel