Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH V2] apei/ghes: fix ghes_poll_func by registering in non-deferrable mode
@ 2020-01-08 17:17 Bhaskar Upadhaya
  2020-01-09  9:50 ` Borislav Petkov
  0 siblings, 1 reply; 3+ messages in thread
From: Bhaskar Upadhaya @ 2020-01-08 17:17 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-edac, lenb, rafael
  Cc: bp, gkulkarni, rrichter, bhaskar.upadhaya.linux, Bhaskar Upadhaya

Currently Linux register ghes_poll_func with TIMER_DEFERRABLE flag,
because of which it is serviced when the CPU eventually wakes up with a
subsequent non-deferrable timer and not at the configured polling interval.

For polling mode, the polling interval configured by firmware should not
be exceeded as per ACPI_6_3 spec[refer Table 18-394], So Timer need to
be configured in non-deferrable mode by removing TIMER_DEFERRABLE flag.
With NO_HZ enabled and timer callback being configured in non-deferrable
mode, timer callback will get called exactly after polling interval.

Definition of poll interval as per spec (referred ACPI 6.3):
"Indicates the poll interval in milliseconds OSPM should use to
periodically check the error source for the presence of an error
condition"

We are observing an issue in our ThunderX2 platforms wherein
ghes_poll_func is not called within poll interval when timer is
configured with TIMER_DEFERRABLE flag(For NO_HZ kernel) and hence
we are losing the error records.

Impact of removing TIMER_DEFFERABLE flag
- With NO_HZ enabled, additional timer ticks and unnecessary wakeups of
 the cpu happens exactly after polling interval.

- If polling interval is too small than polling function will be called
 too frequently which may stall the cpu.

Signed-off-by: Bhaskar Upadhaya <bupadhaya@marvell.com>
---
Changes for V2
Incorporated Borislav comments to add below in commit message
- definition of poll interval as per ACPI 6.3
- problem which is occuring on ThunderX2 platform with TIMER_DEFERRABLE flag
   
 drivers/acpi/apei/ghes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 777f6f7122b4..c8f9230f69fb 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -1181,7 +1181,7 @@ static int ghes_probe(struct platform_device *ghes_dev)
 
 	switch (generic->notify.type) {
 	case ACPI_HEST_NOTIFY_POLLED:
-		timer_setup(&ghes->timer, ghes_poll_func, TIMER_DEFERRABLE);
+		timer_setup(&ghes->timer, ghes_poll_func, 0);
 		ghes_add_timer(ghes);
 		break;
 	case ACPI_HEST_NOTIFY_EXTERNAL:
-- 
2.17.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V2] apei/ghes: fix ghes_poll_func by registering in non-deferrable mode
  2020-01-08 17:17 [PATCH V2] apei/ghes: fix ghes_poll_func by registering in non-deferrable mode Bhaskar Upadhaya
@ 2020-01-09  9:50 ` Borislav Petkov
  2020-01-13 10:51   ` Rafael J. Wysocki
  0 siblings, 1 reply; 3+ messages in thread
From: Borislav Petkov @ 2020-01-09  9:50 UTC (permalink / raw)
  To: Bhaskar Upadhaya
  Cc: linux-kernel, linux-acpi, linux-edac, lenb, rafael, gkulkarni,
	rrichter, bhaskar.upadhaya.linux

On Wed, Jan 08, 2020 at 09:17:38AM -0800, Bhaskar Upadhaya wrote:
> Currently Linux register ghes_poll_func with TIMER_DEFERRABLE flag,
> because of which it is serviced when the CPU eventually wakes up with a
> subsequent non-deferrable timer and not at the configured polling interval.
> 
> For polling mode, the polling interval configured by firmware should not
> be exceeded as per ACPI_6_3 spec[refer Table 18-394], So Timer need to
> be configured in non-deferrable mode by removing TIMER_DEFERRABLE flag.
> With NO_HZ enabled and timer callback being configured in non-deferrable
> mode, timer callback will get called exactly after polling interval.
> 
> Definition of poll interval as per spec (referred ACPI 6.3):
> "Indicates the poll interval in milliseconds OSPM should use to
> periodically check the error source for the presence of an error
> condition"
> 
> We are observing an issue in our ThunderX2 platforms wherein
> ghes_poll_func is not called within poll interval when timer is
> configured with TIMER_DEFERRABLE flag(For NO_HZ kernel) and hence
> we are losing the error records.
> 
> Impact of removing TIMER_DEFFERABLE flag
> - With NO_HZ enabled, additional timer ticks and unnecessary wakeups of
>  the cpu happens exactly after polling interval.
> 
> - If polling interval is too small than polling function will be called
>  too frequently which may stall the cpu.

If that becomes a problem, the polling interval setting should be fixed
to filter too small values.

Anyway, I went and streamlined your commit message:

    apei/ghes: Do not delay GHES polling

    Currently, the ghes_poll_func() timer callback is registered with the
    TIMER_DEFERRABLE flag. Thus, it is run when the CPU eventually wakes
    up together with a subsequent non-deferrable timer and not at the precisely
    configured polling interval.

    For polling mode, the polling interval configured by firmware should not
    be exceeded according to the ACPI spec 6.3, Table 18-394. The definition
    of the polling interval is:

    "Indicates the poll interval in milliseconds OSPM should use to
    periodically check the error source for the presence of an error
    condition."

    If this interval is extended due to the timer callback deferring, error
    records can get lost. Which we are observing on our ThunderX2 platforms.

    Therefore, remove the TIMER_DEFERRABLE flag so that the timer callback
    executes at the precise interval.

and made it more readable, hopefully.

Rafael, pls fixup when applying.

With that:

Acked-by: Borislav Petkov <bp@suse.de>

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V2] apei/ghes: fix ghes_poll_func by registering in non-deferrable mode
  2020-01-09  9:50 ` Borislav Petkov
@ 2020-01-13 10:51   ` Rafael J. Wysocki
  0 siblings, 0 replies; 3+ messages in thread
From: Rafael J. Wysocki @ 2020-01-13 10:51 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Bhaskar Upadhaya, Linux Kernel Mailing List,
	ACPI Devel Maling List, open list:EDAC-CORE, Len Brown,
	Rafael J. Wysocki, gkulkarni, Robert Richter,
	bhaskar.upadhaya.linux

On Thu, Jan 9, 2020 at 10:50 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Jan 08, 2020 at 09:17:38AM -0800, Bhaskar Upadhaya wrote:
> > Currently Linux register ghes_poll_func with TIMER_DEFERRABLE flag,
> > because of which it is serviced when the CPU eventually wakes up with a
> > subsequent non-deferrable timer and not at the configured polling interval.
> >
> > For polling mode, the polling interval configured by firmware should not
> > be exceeded as per ACPI_6_3 spec[refer Table 18-394], So Timer need to
> > be configured in non-deferrable mode by removing TIMER_DEFERRABLE flag.
> > With NO_HZ enabled and timer callback being configured in non-deferrable
> > mode, timer callback will get called exactly after polling interval.
> >
> > Definition of poll interval as per spec (referred ACPI 6.3):
> > "Indicates the poll interval in milliseconds OSPM should use to
> > periodically check the error source for the presence of an error
> > condition"
> >
> > We are observing an issue in our ThunderX2 platforms wherein
> > ghes_poll_func is not called within poll interval when timer is
> > configured with TIMER_DEFERRABLE flag(For NO_HZ kernel) and hence
> > we are losing the error records.
> >
> > Impact of removing TIMER_DEFFERABLE flag
> > - With NO_HZ enabled, additional timer ticks and unnecessary wakeups of
> >  the cpu happens exactly after polling interval.
> >
> > - If polling interval is too small than polling function will be called
> >  too frequently which may stall the cpu.
>
> If that becomes a problem, the polling interval setting should be fixed
> to filter too small values.
>
> Anyway, I went and streamlined your commit message:
>
>     apei/ghes: Do not delay GHES polling
>
>     Currently, the ghes_poll_func() timer callback is registered with the
>     TIMER_DEFERRABLE flag. Thus, it is run when the CPU eventually wakes
>     up together with a subsequent non-deferrable timer and not at the precisely
>     configured polling interval.
>
>     For polling mode, the polling interval configured by firmware should not
>     be exceeded according to the ACPI spec 6.3, Table 18-394. The definition
>     of the polling interval is:
>
>     "Indicates the poll interval in milliseconds OSPM should use to
>     periodically check the error source for the presence of an error
>     condition."
>
>     If this interval is extended due to the timer callback deferring, error
>     records can get lost. Which we are observing on our ThunderX2 platforms.
>
>     Therefore, remove the TIMER_DEFERRABLE flag so that the timer callback
>     executes at the precise interval.
>
> and made it more readable, hopefully.
>
> Rafael, pls fixup when applying.

Done.

> With that:
>
> Acked-by: Borislav Petkov <bp@suse.de>

Thanks!

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-08 17:17 [PATCH V2] apei/ghes: fix ghes_poll_func by registering in non-deferrable mode Bhaskar Upadhaya
2020-01-09  9:50 ` Borislav Petkov
2020-01-13 10:51   ` Rafael J. Wysocki

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git