Re: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
Linus Torvalds wrote: In particular, I don't see why you didn't just put this in the generic calibrate_delay() routine. You seem to have basically duplicated it, and added the "guess from an external timer" code - and I don't see what's non-generic about that, except for some trivial "what's the current timer" lookup. The trivial lookup is hidden in the __delay() function. Making it return the number of "loops" it actually waited would help having a robust generic code. Regards Martin - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
Venkatesh Pallipadi wrote: (0) Estimate a value for loops_per_jiffy (1) While (loops_per_jiffy estimate is accurate enough) (2) wait for jiffy transition (jiffy1) (3) Note down current tsc (tsc1) (4) loop until tsc becomes tsc1 + loops_per_jiffy (5) check whether jiffy changed since jiffy1 or not > and refine loops_per_jiffy estimate Case 2: If SMIs happen between (3) and (4) above, then we can end up with a loops_per_jiffy value that is too high. I don't think this can really happen (at least not on the bootstrap CPU) because even if an SMI occurs, the CPU must first serve the timer interrupt and increment jiffies before it continues looping. However "LPJ too short" is what needs to be avoided at all cost. Regards and thanks for working on this problem, Martin - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
On Fri, 28 Jan 2005, Venkatesh Pallipadi wrote: > > Current tsc based delay_calibration can result in significant errors in > loops_per_jiffy count when the platform events like SMIs (System > Management Interrupts that are non-maskable) are present. This could > lead to potential kernel panic(). This issue is becoming more visible > with 2.6 kernel (as default HZ is 1000) and on platforms with higher SMI > handling latencies. During the boot time, SMIs are mostly used by BIOS > (for things like legacy keyboard emulation). Hmm. I see the problem, but I don't know that I'm 100% happy with this patch, though. In particular, I don't see why you didn't just put this in the generic calibrate_delay() routine. You seem to have basically duplicated it, and added the "guess from an external timer" code - and I don't see what's non-generic about that, except for some trivial "what's the current timer" lookup. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
>-Original Message-
>From: Andi Kleen [mailto:[EMAIL PROTECTED]
>Sent: Friday, January 28, 2005 10:24 PM
>To: Pallipadi, Venkatesh
>Cc: Seth, Rohit; Mallick, Asit K;
>linux-kernel@vger.kernel.org; [EMAIL PROTECTED]
>Subject: Re: [Discuss][i386] Platform SMIs and their
>interferance with tsc based delay calibration
>
>Venkatesh Pallipadi <[EMAIL PROTECTED]> writes:
>> +
>> +/*
>> + * If the upper limit and lower limit of the tsc_rate
>is more than
>> + * 12.5% apart.
>> + */
>> +if (pre_start == 0 || pre_end == 0 ||
>> +(tsc_rate_max - tsc_rate_min) > (tsc_rate_max >> 3)) {
>> +printk(KERN_WARNING "TSC calibration may not be
>precise. "
>> + "Too many SMIs? "
>> + "Consider running with \"lpj=\" boot option\n");
>> +return 0;
>> +}
>
>I think it would be better to rerun it a few times automatically
>before giving up. This way it would hopefully work
>transparently but slower
>for most users.
Agreed. Actually, I was doing that earlier, with each retry
calibrating for 1 HZ. But, once I moved to 10 HZ, I removed the retires.
>The message is too obscure too to be usable and needs
>more explanation.
I will try to improve the message in next revision of the patch.
>And also in case the platforms in questions support EM64T
>x86-64 would need to be changed too :)
Yes. I will send out a patch for x86-64 too, once the i386 patch
gets finalized. I wanted to have a shorted patch reviewed first :).
Thanks,
Venki
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
>-Original Message- >From: Andrew Morton [mailto:[EMAIL PROTECTED] >Subject: Re: [Discuss][i386] Platform SMIs and their >interferance with tsc based delay calibration > > >Please don't send emails which contain 500-column lines? Sorry. Something got messed up during cut and paste onto my mailer. >> Solution: >> The patch below makes the calibration routine aware of >asynchronous events >> like SMIs. We increase the delay calibration time and also >identify any >> significant errors (greater than 12.5%) in the calibration >and notify it >> to user. Like to know your comments on this. > >I find calibrate_delay_tsc() quite confusing. Are you sure that the >variable names are correct? > > + tsc_rate_max = (post_end - pre_start) / DELAY_CALIBRATION_TICKS; > + tsc_rate_min = (pre_end - post_start) / DELAY_CALIBRATION_TICKS; > >that looks strange. I'm sure it all makes sense if one understands the >algorithm, but it shouldn't be this hard. Please reissue the >patch with >adequate comments which describe what the code is doing. > I will resend the patch soon with more comments. I think the variable names here are bit confusing. >Shouldn't calibrate_delay_tsc() be __devinit? (That may >generate warnings >from reference_discarded.pl, but they're false positives) > > >From a maintainability POV it's not good that x86 is no longer >using the >generic calibrate_delay() code. Can you rework the code so that all >architectures must implement arch_calibrate_delay(), then >provide stubs for >all except x86? After all, other architectures/platforms may >have the same >problem. > Agreed. I will add a stub in other architectures. That way we don't have to duplicate the current delay_calibration under i386. Thanks, Venki - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
-Original Message- From: Andrew Morton [mailto:[EMAIL PROTECTED] Subject: Re: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration Please don't send emails which contain 500-column lines? Sorry. Something got messed up during cut and paste onto my mailer. Solution: The patch below makes the calibration routine aware of asynchronous events like SMIs. We increase the delay calibration time and also identify any significant errors (greater than 12.5%) in the calibration and notify it to user. Like to know your comments on this. I find calibrate_delay_tsc() quite confusing. Are you sure that the variable names are correct? + tsc_rate_max = (post_end - pre_start) / DELAY_CALIBRATION_TICKS; + tsc_rate_min = (pre_end - post_start) / DELAY_CALIBRATION_TICKS; that looks strange. I'm sure it all makes sense if one understands the algorithm, but it shouldn't be this hard. Please reissue the patch with adequate comments which describe what the code is doing. I will resend the patch soon with more comments. I think the variable names here are bit confusing. Shouldn't calibrate_delay_tsc() be __devinit? (That may generate warnings from reference_discarded.pl, but they're false positives) From a maintainability POV it's not good that x86 is no longer using the generic calibrate_delay() code. Can you rework the code so that all architectures must implement arch_calibrate_delay(), then provide stubs for all except x86? After all, other architectures/platforms may have the same problem. Agreed. I will add a stub in other architectures. That way we don't have to duplicate the current delay_calibration under i386. Thanks, Venki - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
-Original Message-
From: Andi Kleen [mailto:[EMAIL PROTECTED]
Sent: Friday, January 28, 2005 10:24 PM
To: Pallipadi, Venkatesh
Cc: Seth, Rohit; Mallick, Asit K;
linux-kernel@vger.kernel.org; [EMAIL PROTECTED]
Subject: Re: [Discuss][i386] Platform SMIs and their
interferance with tsc based delay calibration
Venkatesh Pallipadi [EMAIL PROTECTED] writes:
+
+/*
+ * If the upper limit and lower limit of the tsc_rate
is more than
+ * 12.5% apart.
+ */
+if (pre_start == 0 || pre_end == 0 ||
+(tsc_rate_max - tsc_rate_min) (tsc_rate_max 3)) {
+printk(KERN_WARNING TSC calibration may not be
precise.
+ Too many SMIs?
+ Consider running with \lpj=\ boot option\n);
+return 0;
+}
I think it would be better to rerun it a few times automatically
before giving up. This way it would hopefully work
transparently but slower
for most users.
Agreed. Actually, I was doing that earlier, with each retry
calibrating for 1 HZ. But, once I moved to 10 HZ, I removed the retires.
The message is too obscure too to be usable and needs
more explanation.
I will try to improve the message in next revision of the patch.
And also in case the platforms in questions support EM64T
x86-64 would need to be changed too :)
Yes. I will send out a patch for x86-64 too, once the i386 patch
gets finalized. I wanted to have a shorted patch reviewed first :).
Thanks,
Venki
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
On Fri, 28 Jan 2005, Venkatesh Pallipadi wrote: Current tsc based delay_calibration can result in significant errors in loops_per_jiffy count when the platform events like SMIs (System Management Interrupts that are non-maskable) are present. This could lead to potential kernel panic(). This issue is becoming more visible with 2.6 kernel (as default HZ is 1000) and on platforms with higher SMI handling latencies. During the boot time, SMIs are mostly used by BIOS (for things like legacy keyboard emulation). Hmm. I see the problem, but I don't know that I'm 100% happy with this patch, though. In particular, I don't see why you didn't just put this in the generic calibrate_delay() routine. You seem to have basically duplicated it, and added the guess from an external timer code - and I don't see what's non-generic about that, except for some trivial what's the current timer lookup. Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
Venkatesh Pallipadi <[EMAIL PROTECTED]> writes:
> +
> + /*
> + * If the upper limit and lower limit of the tsc_rate is more than
> + * 12.5% apart.
> + */
> + if (pre_start == 0 || pre_end == 0 ||
> + (tsc_rate_max - tsc_rate_min) > (tsc_rate_max >> 3)) {
> + printk(KERN_WARNING "TSC calibration may not be precise. "
> +"Too many SMIs? "
> +"Consider running with \"lpj=\" boot option\n");
> + return 0;
> + }
I think it would be better to rerun it a few times automatically
before giving up. This way it would hopefully work transparently but slower
for most users. The message is too obscure too to be usable and needs
more explanation.
And also in case the platforms in questions support EM64T
x86-64 would need to be changed too :)
-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
Please don't send emails which contain 500-column lines? Venkatesh Pallipadi <[EMAIL PROTECTED]> wrote: > > Current tsc based delay_calibration can result in significant errors in > loops_per_jiffy count when the platform events like SMIs (System Management > Interrupts that are non-maskable) are present. This seems like an unsolveable problem. > Solution: > The patch below makes the calibration routine aware of asynchronous events > like SMIs. We increase the delay calibration time and also identify any > significant errors (greater than 12.5%) in the calibration and notify it > to user. Like to know your comments on this. I find calibrate_delay_tsc() quite confusing. Are you sure that the variable names are correct? + tsc_rate_max = (post_end - pre_start) / DELAY_CALIBRATION_TICKS; + tsc_rate_min = (pre_end - post_start) / DELAY_CALIBRATION_TICKS; that looks strange. I'm sure it all makes sense if one understands the algorithm, but it shouldn't be this hard. Please reissue the patch with adequate comments which describe what the code is doing. Shouldn't calibrate_delay_tsc() be __devinit? (That may generate warnings from reference_discarded.pl, but they're false positives) >From a maintainability POV it's not good that x86 is no longer using the generic calibrate_delay() code. Can you rework the code so that all architectures must implement arch_calibrate_delay(), then provide stubs for all except x86? After all, other architectures/platforms may have the same problem. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
Please don't send emails which contain 500-column lines? Venkatesh Pallipadi [EMAIL PROTECTED] wrote: Current tsc based delay_calibration can result in significant errors in loops_per_jiffy count when the platform events like SMIs (System Management Interrupts that are non-maskable) are present. This seems like an unsolveable problem. Solution: The patch below makes the calibration routine aware of asynchronous events like SMIs. We increase the delay calibration time and also identify any significant errors (greater than 12.5%) in the calibration and notify it to user. Like to know your comments on this. I find calibrate_delay_tsc() quite confusing. Are you sure that the variable names are correct? + tsc_rate_max = (post_end - pre_start) / DELAY_CALIBRATION_TICKS; + tsc_rate_min = (pre_end - post_start) / DELAY_CALIBRATION_TICKS; that looks strange. I'm sure it all makes sense if one understands the algorithm, but it shouldn't be this hard. Please reissue the patch with adequate comments which describe what the code is doing. Shouldn't calibrate_delay_tsc() be __devinit? (That may generate warnings from reference_discarded.pl, but they're false positives) From a maintainability POV it's not good that x86 is no longer using the generic calibrate_delay() code. Can you rework the code so that all architectures must implement arch_calibrate_delay(), then provide stubs for all except x86? After all, other architectures/platforms may have the same problem. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Discuss][i386] Platform SMIs and their interferance with tsc based delay calibration
Venkatesh Pallipadi [EMAIL PROTECTED] writes:
+
+ /*
+ * If the upper limit and lower limit of the tsc_rate is more than
+ * 12.5% apart.
+ */
+ if (pre_start == 0 || pre_end == 0 ||
+ (tsc_rate_max - tsc_rate_min) (tsc_rate_max 3)) {
+ printk(KERN_WARNING TSC calibration may not be precise.
+Too many SMIs?
+Consider running with \lpj=\ boot option\n);
+ return 0;
+ }
I think it would be better to rerun it a few times automatically
before giving up. This way it would hopefully work transparently but slower
for most users. The message is too obscure too to be usable and needs
more explanation.
And also in case the platforms in questions support EM64T
x86-64 would need to be changed too :)
-Andi
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
