Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1678455 > unrolled thread

[PATCH v5 08/12] x86/ioapic: Refactor the delay logic in timer_irq_works()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 08/12] x86/ioapic: Refactor the delay logic in timer_irq_works() Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-06-30 06:10 +0200
    Re: [PATCH v5 08/12] x86/ioapic: Refactor the delay logic in  timer_irq_works() Thomas Gleixner <tglx@linutronix.de> - 2017-07-02 21:20 +0200
      Re: [PATCH v5 08/12] x86/ioapic: Refactor the delay logic in  timer_irq_works() Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-07-03 05:30 +0200

#1678455 — [PATCH v5 08/12] x86/ioapic: Refactor the delay logic in timer_irq_works()

Kernel use timer_irq_works() to detects the timer IRQs. It calls
mdelay(10) to delay ten ticks and check whether the timer IRQ work
or not. The mdelay() depends on the loops_per_jiffy which is set up
in calibrate_delay(). Current kernel defaults the IRQ 0 is available
when it calibrates delay.

But it is wrong in the dump-capture kernel with 'notsc' option inherited
from 1st kernel option. dump-capture kernel can't make sure the timer IRQ
works well.

The correct design is making the interrupt mode setup and checking timer
IRQ works in advance of calibrate_delay(). That results in the mdelay()
being unusable in timer_irq_works().

Preparatory patch to make the setup in advance. Refactor the delay logic
by waiting for some cycles. In the system with X86_FEATURE_TSC feature,
Use rdtsc(), others will call __delay() directly.

Note: regard 4G as the max CPU frequence of current single CPU.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/apic/io_apic.c | 45 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 347bb9f..f710077 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1607,6 +1607,43 @@ static int __init notimercheck(char *s)
 }
 __setup("no_timer_check", notimercheck);
 
+static void __init delay_with_tsc(void)
+{
+	unsigned long long start, now;
+	unsigned long ticks = jiffies;
+
+	start = rdtsc();
+
+	/*
+	 * We don't know the TSC frequency yet, but waiting for
+	 * 40000000000/HZ TSC cycles is safe:
+	 * 4 GHz == 10 jiffies
+	 * 1 GHz == 40 jiffies
+	 */
+	do {
+		rep_nop();
+		now = rdtsc();
+	} while ((now - start) < 40000000000UL / HZ &&
+		time_before_eq(jiffies, ticks + 4));
+}
+
+static void __init delay_without_tsc(void)
+{
+	int band = 1;
+	unsigned long ticks = jiffies;
+
+	/*
+	 * We don't know any frequency yet, but waiting for
+	 * 40940000000/HZ cycles is safe:
+	 * 4 GHz == 10 jiffies
+	 * 1 GHz == 40 jiffies
+	 * 1 << 1 + 1 << 2 +...+ 1 << 11 = 4094
+	 */
+	do {
+		__delay(((1 << band++) * 10000000UL) / HZ);
+	} while (band < 12 && time_before_eq(jiffies, ticks + 4));
+}
+
 /*
  * There is a nasty bug in some older SMP boards, their mptable lies
  * about the timer IRQ. We do the following to work around the situation:
@@ -1625,8 +1662,12 @@ static int __init timer_irq_works(void)
 
 	local_save_flags(flags);
 	local_irq_enable();
-	/* Let ten ticks pass... */
-	mdelay((10 * 1000) / HZ);
+
+	if (boot_cpu_has(X86_FEATURE_TSC))
+		delay_with_tsc();
+	else
+		delay_without_tsc();
+
 	local_irq_restore(flags);
 
 	/*
-- 
2.5.5

[toc] | [next] | [standalone]

#1679551 — Re: [PATCH v5 08/12] x86/ioapic: Refactor the delay logic in timer_irq_works()

On Fri, 30 Jun 2017, Dou Liyang wrote:
> +static void __init delay_with_tsc(void)
> +{
> +	unsigned long long start, now;
> +	unsigned long ticks = jiffies;

Please make that

       unsigned long end = jiffies + 4;

ticks really means: number of ticks. But that variable is doing something
different.

> +	start = rdtsc();
> +
> +	/*
> +	 * We don't know the TSC frequency yet, but waiting for
> +	 * 40000000000/HZ TSC cycles is safe:
> +	 * 4 GHz == 10 jiffies
> +	 * 1 GHz == 40 jiffies
> +	 */
> +	do {
> +		rep_nop();
> +		now = rdtsc();
> +	} while ((now - start) < 40000000000UL / HZ &&
> +		time_before_eq(jiffies, ticks + 4));
> +}
> +
> +static void __init delay_without_tsc(void)
> +{
> +	int band = 1;
> +	unsigned long ticks = jiffies;

Please sort variables in reverse fir tree order

	unsigned long end = jiffies + 4;
	int band = 1;

> +
> +	/*
> +	 * We don't know any frequency yet, but waiting for
> +	 * 40940000000/HZ cycles is safe:
> +	 * 4 GHz == 10 jiffies
> +	 * 1 GHz == 40 jiffies
> +	 * 1 << 1 + 1 << 2 +...+ 1 << 11 = 4094
> +	 */
> +	do {
> +		__delay(((1 << band++) * 10000000UL) / HZ);

  s/1/1U/

> +	} while (band < 12 && time_before_eq(jiffies, ticks + 4));
> +}

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1679619 — Re: [PATCH v5 08/12] x86/ioapic: Refactor the delay logic in timer_irq_works()

Hi Thomas,

At 07/03/2017 03:15 AM, Thomas Gleixner wrote:
> On Fri, 30 Jun 2017, Dou Liyang wrote:
>> +static void __init delay_with_tsc(void)
>> +{
>> +	unsigned long long start, now;
>> +	unsigned long ticks = jiffies;
>
> Please make that
>
>        unsigned long end = jiffies + 4;
>
> ticks really means: number of ticks. But that variable is doing something
> different.

um, I see. Will use 'end' instead.

>
>> +	start = rdtsc();
>> +
>> +	/*
>> +	 * We don't know the TSC frequency yet, but waiting for
>> +	 * 40000000000/HZ TSC cycles is safe:
>> +	 * 4 GHz == 10 jiffies
>> +	 * 1 GHz == 40 jiffies
>> +	 */
>> +	do {
>> +		rep_nop();
>> +		now = rdtsc();
>> +	} while ((now - start) < 40000000000UL / HZ &&
>> +		time_before_eq(jiffies, ticks + 4));
>> +}
>> +
>> +static void __init delay_without_tsc(void)
>> +{
>> +	int band = 1;
>> +	unsigned long ticks = jiffies;
>
> Please sort variables in reverse fir tree order
>
> 	unsigned long end = jiffies + 4;
> 	int band = 1;
>

OK, I will.

>> +
>> +	/*
>> +	 * We don't know any frequency yet, but waiting for
>> +	 * 40940000000/HZ cycles is safe:
>> +	 * 4 GHz == 10 jiffies
>> +	 * 1 GHz == 40 jiffies
>> +	 * 1 << 1 + 1 << 2 +...+ 1 << 11 = 4094
>> +	 */
>> +	do {
>> +		__delay(((1 << band++) * 10000000UL) / HZ);
>
>   s/1/1U/
>

Got it!

Thanks,

	dou.


>> +	} while (band < 12 && time_before_eq(jiffies, ticks + 4));
>> +}
>
> Thanks,
>
> 	tglx
>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web