RE: Fixing MTRR smp breakage and suspending sysdevs.

RE: Fixing MTRR smp breakage and suspending sysdevs.

[Li, Shaohua]

>
>Hi!
>
>I've just had a go at fixing the issue with my implementation not
>suspending the sysdevs (I believe swsusp does the same). In the
process,
>I reworked the MTRR support so it's not treated as a sysdev. Instead,
>when we're saving cpu state, the mtrr_save function function is called.
>When we go to restore CPU state, each CPU calls a function that resets
>it's MTRRs and the 'main' cpu then frees the saved data. This is
working
>well here (did a dozen plus suspends on the trot), but I want to check
>that it sounds like the right solution to you.
>
>Perhaps this method should be made more generic? (Are there likely to
be
>other per-cpu state savers needed?)
>
>One thing I have noticed is that by adding the sysdev suspend/resume
>calls, I've gained a few seconds delay. I'll see if I can track down
the
>cause.
Is the problem MTRR resume must be with IRQ enabled, right? Could we
implement a method sysdev resume with IRQ enabled? MTRR driver isn't the
only case. The ACPI Link device is another case, it's a sysdev (it must
resume before any PCI device resumed), but its resume (it uses semaphore
and non-atomic kmalloc) can't invoked with IRQ enabled. I guess cpufreq
driver is another case when suspend/resume SMP is supported.

Thanks,
Shaohua

RE: Fixing MTRR smp breakage and suspending sysdevs.

[Nigel Cunningham]

Hi.

On Wed, 2004-10-27 at 12:48, Li, Shaohua wrote:
> >
> >Hi!
> >
> >I've just had a go at fixing the issue with my implementation not
> >suspending the sysdevs (I believe swsusp does the same). In the
> process,
> >I reworked the MTRR support so it's not treated as a sysdev. Instead,
> >when we're saving cpu state, the mtrr_save function function is called.
> >When we go to restore CPU state, each CPU calls a function that resets
> >it's MTRRs and the 'main' cpu then frees the saved data. This is
> working
> >well here (did a dozen plus suspends on the trot), but I want to check
> >that it sounds like the right solution to you.
> >
> >Perhaps this method should be made more generic? (Are there likely to
> be
> >other per-cpu state savers needed?)
> >
> >One thing I have noticed is that by adding the sysdev suspend/resume
> >calls, I've gained a few seconds delay. I'll see if I can track down
> the
> >cause.

> Is the problem MTRR resume must be with IRQ enabled, right? Could we

That's right.

> implement a method sysdev resume with IRQ enabled? MTRR driver isn't the
> only case. The ACPI Link device is another case, it's a sysdev (it must
> resume before any PCI device resumed), but its resume (it uses semaphore
> and non-atomic kmalloc) can't invoked with IRQ enabled. I guess cpufreq
> driver is another case when suspend/resume SMP is supported.

I'll see if I can find some time to prepare something. Might be a bit
slow; humans don't multitask very well, and I'm trying to at the moment
:>

Nigel

-- 
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Everyone lives by faith. Some people just don't believe it.
Want proof? Try to prove that the theory of evolution is true.

Re: Fixing MTRR smp breakage and suspending sysdevs.

[Dmitry Torokhov]

On Tuesday 26 October 2004 09:48 pm, Li, Shaohua wrote:
> >One thing I have noticed is that by adding the sysdev suspend/resume
> >calls, I've gained a few seconds delay. I'll see if I can track down
> the
> >cause.
> Is the problem MTRR resume must be with IRQ enabled, right? Could we
> implement a method sysdev resume with IRQ enabled?

If I understand correctly the point of classifying device as sysdev is
that it (device) is essential for the system and must be suspended last
and resumed first, presumably with interrupts off. IRQ controller comes
to mind...
 
-- 
Dmitry

Re: Fixing MTRR smp breakage and suspending sysdevs.

[Nigel Cunningham]

Hi.

On Wed, 2004-10-27 at 13:20, Dmitry Torokhov wrote:
> On Tuesday 26 October 2004 09:48 pm, Li, Shaohua wrote:
> > >One thing I have noticed is that by adding the sysdev suspend/resume
> > >calls, I've gained a few seconds delay. I'll see if I can track down
> > the
> > >cause.
> > Is the problem MTRR resume must be with IRQ enabled, right? Could we
> > implement a method sysdev resume with IRQ enabled?
> 
> If I understand correctly the point of classifying device as sysdev is
> that it (device) is essential for the system and must be suspended last
> and resumed first, presumably with interrupts off. IRQ controller comes
> to mind...

Yes, but could we not do something like the process with regular
devices. ie a call with interrupts disabled and then a similar call with
interrupts enabled?

Regards,

Nigel 
-- 
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Everyone lives by faith. Some people just don't believe it.
Want proof? Try to prove that the theory of evolution is true.

Re: Fixing MTRR smp breakage and suspending sysdevs.

[Dmitry Torokhov]

On Tuesday 26 October 2004 10:17 pm, Nigel Cunningham wrote:
> Hi.
> 
> On Wed, 2004-10-27 at 13:20, Dmitry Torokhov wrote:
> > On Tuesday 26 October 2004 09:48 pm, Li, Shaohua wrote:
> > > >One thing I have noticed is that by adding the sysdev suspend/resume
> > > >calls, I've gained a few seconds delay. I'll see if I can track down
> > > the
> > > >cause.
> > > Is the problem MTRR resume must be with IRQ enabled, right? Could we
> > > implement a method sysdev resume with IRQ enabled?
> > 
> > If I understand correctly the point of classifying device as sysdev is
> > that it (device) is essential for the system and must be suspended last
> > and resumed first, presumably with interrupts off. IRQ controller comes
> > to mind...
> 
> Yes, but could we not do something like the process with regular
> devices. ie a call with interrupts disabled and then a similar call with
> interrupts enabled?
> 

Yes, re-reading the parent post it seems that's what required for ACPI.
Doing a pass with IRQ ON for regular devices, then IRQ OFF, then IRQ on
again for sysdevs and then again with IRQ off. Man, that's getting messy...

Well, I understand that ACPI is using semaphore and a GFP_KERNEL, but what
is the problem with MTRR? I understand that they should be set with IRQ
off but I highly doibt that enabling IRQ at the end is a requirement.
I think what is described in the commnet is rather a "normal flow of events".

-- 
Dmitry

Re: Fixing MTRR smp breakage and suspending sysdevs.

[Nigel Cunningham]

Hi.

On Wed, 2004-10-27 at 13:50, Dmitry Torokhov wrote:
> Well, I understand that ACPI is using semaphore and a GFP_KERNEL, but what
> is the problem with MTRR? I understand that they should be set with IRQ
> off but I highly doibt that enabling IRQ at the end is a requirement.
> I think what is described in the commnet is rather a "normal flow of events".

The real problem with MTRRs is SMP support: smp_call_function doesn't
like IRQs disabled.

I got around a similar issue with saving CPU state (for suspend-to-disk)
by using the same general sequence that I later discovered described in
arch/i386/kernel/cpu/mtrr/main.c (set_mtrr header) for saving CPU
contexts. I extended it yesterday to do the MTRR settings as well,
before Shaohua pointed to the more general need.

Regards,

Nigel
-- 
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Everyone lives by faith. Some people just don't believe it.
Want proof? Try to prove that the theory of evolution is true.

Re: Fixing MTRR smp breakage and suspending sysdevs.

[Pavel Machek]

Hi!

> >One thing I have noticed is that by adding the sysdev suspend/resume
> >calls, I've gained a few seconds delay. I'll see if I can track down
> the
> >cause.
> Is the problem MTRR resume must be with IRQ enabled, right? Could we
> implement a method sysdev resume with IRQ enabled? MTRR driver isn't
> the

MTRR does not deserve to be sysdev. It is not essential for the
system, it only makes it slow.

> only case. The ACPI Link device is another case, it's a sysdev (it must
> resume before any PCI device resumed), but its resume (it uses semaphore
> and non-atomic kmalloc) can't invoked with IRQ enabled. I guess cpufreq
> driver is another case when suspend/resume SMP is supported.

I do not see how enabling interrupts before setting up IRQs is good
idea.

What about this one, instead?

* ACPI Link device should allocate with GFP_ATOMIC

* during suspend, locks can't be taken. (We stop userland, etc). So it
should be okay to down_trylock() and panic if that does not work.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

time and suspending sysdevs [was Re: Fixing MTRR smp breakage and suspending sysdevs.]

[Pavel Machek]

Hi!

> > >One thing I have noticed is that by adding the sysdev suspend/resume
> > >calls, I've gained a few seconds delay. I'll see if I can track down
> > the
> > >cause.

Sysdev suspend/resume calls should be added to swsusp1, too. Can
someone verify that this fixes stuff? I should get some sleep...

								Pavel

--- clean/kernel/power/disk.c	2004-10-01 00:30:32.000000000 +0200
+++ linux/kernel/power/disk.c	2004-10-29 00:30:40.000000000 +0200
@@ -102,6 +116,7 @@
 
 static void finish(void)
 {
+	sysdev_resume();
 	device_resume();
 	platform_finish();
 	enable_nonboot_cpus();
@@ -133,8 +148,12 @@
 	free_some_memory();
 
 	disable_nonboot_cpus();
-	if ((error = device_suspend(PM_SUSPEND_DISK)))
+	if ((error = device_suspend(PM_SUSPEND_DISK))) {
+		printk("Some devices failed to suspend\n");
 		goto Finish;
+	}
+
+	sysdev_suspend(POWER_SUSPEND_DISK);
 
 	return 0;
  Finish:
--- clean/kernel/power/swsusp.c	2004-10-19 14:16:29.000000000 +0200
+++ linux/kernel/power/swsusp.c	2004-10-29 00:32:26.000000000 +0200
@@ -825,6 +812,7 @@
 int swsusp_write(void)
 {
 	int error;
+	sysdev_resume();
 	device_resume();
 	lock_swapdevices();
 	error = write_suspend_image();


-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: time and suspending sysdevs [was Re: Fixing MTRR smp breakage and suspending sysdevs.]

[Nigel Cunningham]

On Fri, 2004-10-29 at 08:38, Pavel Machek wrote:
> Hi!
> 
> > > >One thing I have noticed is that by adding the sysdev suspend/resume
> > > >calls, I've gained a few seconds delay. I'll see if I can track down
> > > the
> > > >cause.

Don't think I actually mentioned the case: it's the pit timer, it
appears (number before is jiffies). Interestingly, there is a delay in
suspending, but it only shows after we exit the sysdev call (when
interrupts are reenabled? Haven't looked more closely).

Suspending System Devices
Suspending type 'irqrouter':
 4294741499: Starting global drivers irqrouter0
 4294741499: Starting auxillary drivers.
Suspending type 'ioapic':
 4294741499: Starting global drivers ioapic0
 4294741499: Starting auxillary drivers.
 4294741499: Starting generic driver.
 4294741499: Done.
Suspending type 'lapic':
 4294741499: Starting global drivers lapic0
 4294741499: Starting auxillary drivers.
 4294741499: Starting generic driver.
 4294741499: Done.
Suspending type 'timer':
 4294741499: Starting global drivers timer0
 4294741499: Starting auxillary drivers.
Suspending type 'pit':
 4294741499: Starting global drivers pit0
 4294741499: Starting auxillary drivers.
 4294741499: Starting generic driver.
 4294741499: Done.
Suspending type 'i8259':
 4294741499: Starting global drivers i82590
 4294741499: Starting auxillary drivers.
 4294741499: Starting generic driver.
 4294741499: Done.
Suspending type 'cpu':
 4294741499: Starting global drivers cpu0
 4294741499: Starting auxillary drivers.
 4294741499: Starting global drivers cpu1
 4294741499: Starting auxillary drivers.
Back from sysdev_suspend.
sysdev_resume
Resuming System Devices
Resuming type 'cpu':
 4294742128: cpu0
 4294742128: Starting auxillary drivers.
 4294742128: Starting global drivers cpu0
 4294742128: Done.
 4294742128: cpu1
 4294742128: Starting auxillary drivers.
 4294742128: Starting global drivers cpu1
 4294742128: Done.
Resuming type 'i8259':
 4294742128: i82590
 4294742128: Starting generic driver.
 4294742128: Starting auxillary drivers.
 4294742128: Starting global drivers i82590
 4294742128: Done.
Resuming type 'pit':
 4294742128: pit0
 4294742128: Starting generic driver.
 4294772030: Starting auxillary drivers.
 4294772030: Starting global drivers pit0
 4294772030: Done.
Resuming type 'timer':
 4294772030: timer0
 4294772030: Starting generic driver.
 4294772030: Starting auxillary drivers.
 4294772030: Starting global drivers timer0
 4294772030: Done.
Resuming type 'lapic':
 4294772030: lapic0
 4294772030: Starting generic driver.
 4294772030: Starting auxillary drivers.
 4294772030: Starting global drivers lapic0
 4294772030: Done.
Resuming type 'ioapic':
 4294772030: ioapic0
 4294772030: Starting generic driver.
 4294772030: Starting auxillary drivers.
 4294772030: Starting global drivers ioapic0
 4294772030: Done.
Resuming type 'irqrouter':
 4294772030: irqrouter0
 4294772030: Starting generic driver.
 4294772030: Starting auxillary drivers.
 4294772030: Starting global drivers irqrouter0
 4294772030: Done.
power up suspend device tree.
done

Regards,

Nigel

-- 
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Everyone lives by faith. Some people just don't believe it.
Want proof? Try to prove that the theory of evolution is true.