-mm merge plans for 2.6.20

Quoted 9 lines · Andrew Morton

Quoted 4 lines · On Mon, 04 Dec 2006 23:56:42 -0500 Jeff Garzik <[email protected]> wrote:
Quoted 1 line · Andrew Morton
libata_resume_fix.patch
I thought this was resolved long ago? Are there still open reports that this solves, where upstream doesn't work?
Heck, I don't know.

You probably pinched it from my website originally, and I nabbed it from an LKML posting 18 months ago. It's a patch that was required here back in the 2.6.12 - 2.6.16 era, and it is no longer needed now.

Drop it.

Conke Hu

2006-12-06 19:19 UTC #5 lore raw

Quoted 21 lines · Andrew Morton

Quoted 6 lines · On Mon, 04 Dec 2006 23:56:42 -0500 Jeff Garzik <[email protected]> wrote:
Quoted 2 lines · Andrew Morton
via-pata-controller-xfer-fixes.patch
via-pata-controller-xfer-fixes-fix.patch
Tejun's 3d3cca37559e3ab2b574eda11ed5207ccdb8980a has been ack'd by the reporter as fixing things, so these two shouldn't be needed.
OK thanks, I dropped it.
Quoted 5 lines
Quoted 2 lines
libata_resume_fix.patch
I thought this was resolved long ago? Are there still open reports that this solves, where upstream doesn't work?
Heck, I don't know.
Quoted 1 line

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
ahci-ati-sb600-sata-support-for-various-modes.patch
With the PCI quirk, I thought ATI was finally sorted?
Was it? I don't know that either.
I'll drop these too.
-

Hi Jeff, Andrew
The following patch is ATI's final solution. It was ACKed by Alan. Jeff, you're the maintainer of libata, but this patch is based on
pci/quirks.c, so I don't know who will apply this patch? You or somebody else?
Andrew, could you please drop ATI's previous patch and add this one
in next -mm patch? The previous patch I sent (ahci-ati-sb600-sata-support-for-various-modes.patch) is not as good as this one :)

Best regards,
Conke @AMD/ATI

[------------------PATCH------------------]

--- linux-2.6.19-rc6-git4/drivers/pci/quirks.c.orig 2006-11-23
19:45:49.000000000 +0800
+++ linux-2.6.19-rc6-git4/drivers/pci/quirks.c 2006-11-23 19:34:23.000000000 +0800
@@ -795,6 +795,25 @@ static void __init quirk_mediagx_master(

}
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CYRIX,
PCI_DEVICE_ID_CYRIX_PCI_MASTER, quirk_mediagx_master ); +
+#if defined(CONFIG_SATA_AHCI) || defined(CONFIG_SATA_AHCI_MODULE) +static void __devinit quirk_sb600_sata(struct pci_dev *pdev) +{
+ /* set sb600 sata to ahci mode */
+ if ((pdev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
+ u8 tmp;
+

+               pci_read_config_byte(pdev, 0x40, &tmp);
+               pci_write_config_byte(pdev, 0x40, tmp|1);
+               pci_write_config_byte(pdev, 0x9, 1);
+               pci_write_config_byte(pdev, 0xa, 6);
+               pci_write_config_byte(pdev, 0x40, tmp);

+
+ pdev->class = 0x010601;
+ }
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATI,
PCI_DEVICE_ID_ATI_IXP600_SATA, quirk_sb600_sata);
+#endif

/*
* As per PCI spec, ignore base address registers 0-3 of the IDE
controllers

Randy Dunlap

2006-12-06 19:25 UTC #6 lore raw

Quoted 78 lines · On Thu, 07 Dec 2006 03:19:40 +0800 Conke Hu wrote:

Quoted 21 lines · Andrew Morton
Quoted 6 lines · On Mon, 04 Dec 2006 23:56:42 -0500 Jeff Garzik <[email protected]> wrote:
Quoted 2 lines · Andrew Morton
via-pata-controller-xfer-fixes.patch
via-pata-controller-xfer-fixes-fix.patch
Tejun's 3d3cca37559e3ab2b574eda11ed5207ccdb8980a has been ack'd by the reporter as fixing things, so these two shouldn't be needed.
OK thanks, I dropped it.
Quoted 5 lines
Quoted 2 lines
libata_resume_fix.patch
I thought this was resolved long ago? Are there still open reports that this solves, where upstream doesn't work?
Heck, I don't know.
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
ahci-ati-sb600-sata-support-for-various-modes.patch
With the PCI quirk, I thought ATI was finally sorted?
Was it? I don't know that either.
I'll drop these too.
-
Hi Jeff, Andrew
The following patch is ATI's final solution. It was ACKed by Alan. Jeff, you're the maintainer of libata, but this patch is based on
pci/quirks.c, so I don't know who will apply this patch? You or somebody else?
Andrew, could you please drop ATI's previous patch and add this one
in next -mm patch? The previous patch I sent (ahci-ati-sb600-sata-support-for-various-modes.patch) is not as good as this one :)
Best regards,
Conke @AMD/ATI
[------------------PATCH------------------]
--- linux-2.6.19-rc6-git4/drivers/pci/quirks.c.orig 2006-11-23
19:45:49.000000000 +0800
+++ linux-2.6.19-rc6-git4/drivers/pci/quirks.c 2006-11-23 19:34:23.000000000 +0800
(quoted diff) +19 -0
@@ -795,6 +795,25 @@ static void __init quirk_mediagx_master(
       }
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CYRIX,
 PCI_DEVICE_ID_CYRIX_PCI_MASTER, quirk_mediagx_master );
+
+#if defined(CONFIG_SATA_AHCI) || defined(CONFIG_SATA_AHCI_MODULE)
+static void __devinit quirk_sb600_sata(struct pci_dev *pdev)
+{
+       /* set sb600 sata to ahci mode */
+       if ((pdev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
+               u8 tmp;
+
+               pci_read_config_byte(pdev, 0x40, &tmp);
+               pci_write_config_byte(pdev, 0x40, tmp|1);
+               pci_write_config_byte(pdev, 0x9, 1);
+               pci_write_config_byte(pdev, 0xa, 6);
+               pci_write_config_byte(pdev, 0x40, tmp);
+
+               pdev->class = 0x010601;
+       }
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATI,
 PCI_DEVICE_ID_ATI_IXP600_SATA, quirk_sb600_sata);
+#endif
 

Quoted 3 lines

/*
* As per PCI spec, ignore base address registers 0-3 of the IDE
controllers

Can you use fewer magic numbers, please?
At least some of those are already #defined in
include/linux/pci_regs.h.

---
~Randy

Jeff Garzik

2006-12-06 19:40 UTC #7 lore raw

In reply to Conke Hu

Quoted 8 lines · Conke Hu

The following patch is ATI's final solution. It was ACKed by Alan. Jeff, you're the maintainer of libata, but this patch is based on
pci/quirks.c, so I don't know who will apply this patch? You or somebody else?
Andrew, could you please drop ATI's previous patch and add this one
in next -mm patch? The previous patch I sent (ahci-ati-sb600-sata-support-for-various-modes.patch) is not as good as this one :)

I ACK'd it as well, though probably Andrew or Greg should push it.

Jeff

Andrew Morton

author

2006-12-06 22:36 UTC #8 lore raw

In reply to Conke Hu

Quoted 42 lines · On Thu, 07 Dec 2006 03:19:40 +0800 Conke Hu <[email protected]> wrote:

Quoted 21 lines · Andrew Morton
Quoted 6 lines · On Mon, 04 Dec 2006 23:56:42 -0500 Jeff Garzik <[email protected]> wrote:
Quoted 2 lines · Andrew Morton
via-pata-controller-xfer-fixes.patch
via-pata-controller-xfer-fixes-fix.patch
Tejun's 3d3cca37559e3ab2b574eda11ed5207ccdb8980a has been ack'd by the reporter as fixing things, so these two shouldn't be needed.
OK thanks, I dropped it.
Quoted 5 lines
Quoted 2 lines
libata_resume_fix.patch
I thought this was resolved long ago? Are there still open reports that this solves, where upstream doesn't work?
Heck, I don't know.
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
ahci-ati-sb600-sata-support-for-various-modes.patch
With the PCI quirk, I thought ATI was finally sorted?
Was it? I don't know that either.
I'll drop these too.
-
Hi Jeff, Andrew
The following patch is ATI's final solution. It was ACKed by Alan. Jeff, you're the maintainer of libata, but this patch is based on
pci/quirks.c, so I don't know who will apply this patch? You or somebody else?
Andrew, could you please drop ATI's previous patch and add this one
in next -mm patch? The previous patch I sent (ahci-ati-sb600-sata-support-for-various-modes.patch) is not as good as this one :)

Quoted 2 lines

Best regards,
Conke @AMD/ATI

Please send a signed-off-by: for this work, and a changelog which tells us what it does and why the kernel needs this change.

I shall then remove the ifdef (it's __devinit, and other quirks don't do this, and vmlinux doesn't know whether or not this driver will later be loaded) and I shall fix up the word wrapping and I shall convert the spaces back into tabs and I shall then send it on to Greg, thanks.

Quoted 31 lines

[------------------PATCH------------------]

--- linux-2.6.19-rc6-git4/drivers/pci/quirks.c.orig 2006-11-23
19:45:49.000000000 +0800
+++ linux-2.6.19-rc6-git4/drivers/pci/quirks.c 2006-11-23 19:34:23.000000000 +0800

(quoted diff) +19 -0

@@ -795,6 +795,25 @@ static void __init quirk_mediagx_master(
       }
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CYRIX,
 PCI_DEVICE_ID_CYRIX_PCI_MASTER, quirk_mediagx_master );
+
+#if defined(CONFIG_SATA_AHCI) || defined(CONFIG_SATA_AHCI_MODULE)
+static void __devinit quirk_sb600_sata(struct pci_dev *pdev)
+{
+       /* set sb600 sata to ahci mode */
+       if ((pdev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
+               u8 tmp;
+
+               pci_read_config_byte(pdev, 0x40, &tmp);
+               pci_write_config_byte(pdev, 0x40, tmp|1);
+               pci_write_config_byte(pdev, 0x9, 1);
+               pci_write_config_byte(pdev, 0xa, 6);
+               pci_write_config_byte(pdev, 0x40, tmp);
+
+               pdev->class = 0x010601;
+       }
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATI,
 PCI_DEVICE_ID_ATI_IXP600_SATA, quirk_sb600_sata);
+#endif

Paul Mackerras

2006-12-05 05:32 UTC #9 lore raw

In reply to Paul Mackerras

Quoted 2 lines · Andrew Morton

ppc-m48t35-add-missing-bracket.patch
powerpc-replace-kmallocmemset-with-kzalloc.patch

These are already in Linus' tree.

Quoted 1 line

Am holding onto these until the powerpc git tree gets un-messied up.

It should be fine now. Linus has pulled it, so there are currently no changes in powerpc.git relative to Linus' tree.

Quoted 3 lines

radix-tree-rcu-lockless-readside.patch
There's no reason to merge this yet.

We want to use it in some powerpc arch code. Currently we use a per-cpu array of spinlocks, and this patch would let us get rid of that array.

Paul.

Andrew Morton

author

2006-12-05 05:42 UTC #10 lore raw

Quoted 7 lines · On Tue, 5 Dec 2006 16:14:03 +1100 Paul Mackerras <[email protected]> wrote:

Quoted 3 lines
radix-tree-rcu-lockless-readside.patch
There's no reason to merge this yet.
We want to use it in some powerpc arch code. Currently we use a per-cpu array of spinlocks, and this patch would let us get rid of that array.

ok, let's merge it then.

Nick Piggin

2006-12-05 05:54 UTC #11 lore raw

Quoted 6 lines · Andrew Morton

Quoted 2 lines · On Tue, 5 Dec 2006 16:14:03 +1100 Paul Mackerras <[email protected]> wrote:
Quoted 2 lines
radix-tree-rcu-lockless-readside.patch

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
There's no reason to merge this yet.
We want to use it in some powerpc arch code. Currently we use a per-cpu array of spinlocks, and this patch would let us get rid of that array.
ok, let's merge it then.

Well that wasn't as hard as I thought ;) No arguments from me!

Signature · 2 lines

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com

Nick Piggin

2006-12-05 05:50 UTC #12 lore raw

Quoted 10 lines · Paul Mackerras

Quoted 3 lines · Andrew Morton
radix-tree-rcu-lockless-readside.patch
There's no reason to merge this yet.
We want to use it in some powerpc arch code. Currently we use a per-cpu array of spinlocks, and this patch would let us get rid of that array.

I'd like to get another patch in here before going upstream if possible. It is not a correctness fix, but it is a bit of a rework.

I also wouldn't mind getting the readahead path, if not the full pagecache readside, out from under tree_lock in -mm kernels to exercise the radix-tree concurrency a bit more.

It's just been painfully slow, recently because of these more important buffered write vs deadlock and pagefault vs invalidate problems that I've been working on. I don't feel I can load up -mm with too much unrelated stuff that messes with mm/pagecache internals.

I guess the per-cpu spinlocks are pretty reasonable for scalability, and you are mainly looking to eliminate the lock/unlock cost in your interrupt path?

Nick

Signature · 2 lines

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com

Gautham R Shenoy

2006-12-05 08:34 UTC #13 lore raw

Hi Andrew,

Quoted 3 lines

remove-hotplug-cpu-crap-from-cpufreq.patch
Sent to cpufreq maintainer

I suspect that Davej posted this patch because he was getting lockdep warnings-reports from people complaining of ondemand-governor performing spurious unlock_cpu_hotplug.
That problem has been fixed in the mainline by the commit
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=4b96b1a10cb00c867103b21f0f2a6c91b705db11

If there are any other issues with cpufreq-cpuhotplug in the mainline, I'm more than willing to help out fix them. As of now, I cannot seem to spot anything serious in the mainline as such.
Hence, merging this isn't an immediate need IMHO.

Quoted 13 lines

hotplug-cpu-clean-up-hotcpu_notifier-use.patch hotplug-cpu-clean-up-hotcpu_notifier-use-vs-gregkh-driver-cpu-topology-consider-sysfs_create_group-return-value.patch
extend-notifier_call_chain-to-count-nr_calls-made.patch extend-notifier_call_chain-to-count-nr_calls-made-fixes.patch extend-notifier_call_chain-to-count-nr_calls-made-fixes-2.patch define-and-use-new-eventscpu_lock_acquire-and-cpu_lock_release.patch define-and-use-new-eventscpu_lock_acquire-and-cpu_lock_release-fix.patch eliminate-lock_cpu_hotplug-in-kernel-schedc.patch eliminate-lock_cpu_hotplug-in-kernel-schedc-fix.patch handle-cpu_lock_acquire-and-cpu_lock_release-in-workqueue_cpu_callback.patch

Quoted 2 lines

Shall merge.

Merging this would still give the circular-locking dependency warnings which I posted the other day. Unless we have a clean way to get cpu-hotplug-protection for cpufreq, I don't see a point in merging this stuff.

Cpufreq hotplug-interactions can be sorted out.
I have a few patches which I need to test out before posting them.

Other than that, there are issues regarding the workqueue-hotplug-"locking" which needs to be addressed, probably in a seperate thread.

So could you please reconsider this decision to merge the hotplug-locking rework, and let it stabilize in -mm for sometime ?

Thanks and Regards
gautham.

Signature · 5 lines

Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Peter Zijlstra

2006-12-05 08:55 UTC #14 lore raw

Quoted 3 lines

mm-call-into-direct-reclaim-without-pf_memalloc-set.patch mm-cleanup-and-document-reclaim-recursion.patch

Drop these, I'm not sure of them anymore. And I am starting to grow a nagging feeling they are wrong.

Pavel Machek

2006-12-05 11:06 UTC #15 lore raw

ext2 future [was Re: -mm merge plans for 2.6.20]

Hi!

Quoted 10 lines

ext2-reservations.patch
ext2-fix-reservation-extension.patch
make-ext2_get_blocks-static.patch
ext2-balloc-fix-_with_rsv-freeze.patch
ext2-balloc-reset-windowsz-when-full.patch
ext2-balloc-fix-off-by-one-against-rsv_end.patch
ext2-balloc-fix-off-by-one-against-grp_goal.patch
ext2-balloc-say-rb_entry-not-list_entry.patch
ext2-balloc-use-io_error-label.patch

Quoted 2 lines

Not for 2.6.20. In fact it's unclear whether this should ever be merged - ext2 is more an "example filesytem" nowadays. We'll see.

If ext2 is "example filesystem"... perhaps we should add "no journal" mode to ext3 or something? We still want high-performance, not-journalled filesystem, I believe.
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

John W. Linville

2006-12-05 13:23 UTC #16 lore raw

Quoted 6 lines · Andrew Morton

hostap-replace-kmallocmemset-with-kzalloc.patch prism54-replace-kmallocmemset-with-kzalloc.patch ipw2200-replace-kmallocmemset-with-kcalloc.patch softmac-fix-unbalanced-mutex_lock-unlock-in-ieee80211softmac_wx_set_mlme.patch
Not sent to John - I forgot.

I've got them, and I plan to merge them.

Thanks,

John

Signature · 2 lines

John W. Linville
[email protected]

Roman Zippel

2006-12-05 14:27 UTC #17 lore raw

Hi,

Quoted 2 lines · Andrew Morton

kbuild-dont-put-temp-files-in-the-source-tree.patch actually-delete-the-as-instr-ld-option-tmp-file.patch

Andi had objections about the mktemp usage and I agree with him. The proposed patch in bugzilla didn't have this and no further justification was given for why it's needed.
Below is a replacement patch with some improvements:
- kbuild doesn't use $(AS), so use $(CC)
- tmp dir needs only to be calculated once
- reformat a bit to keep it under 80 columns and to be more readable

bye, Roman

Signed-off-by Roman Zippel <[email protected]>

---
 scripts/Kbuild.include |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

scripts/Kbuild.include +12 -7

Index: linux-2.6-git/scripts/Kbuild.include
===================================================================
@@ -56,6 +56,9 @@ endef
 # gcc support functions
 # See documentation in Documentation/kbuild/makefiles.txt
 
+# output directory for tests below
+TMPOUT := $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/)
+
 # as-option
 # Usage: cflags-y += $(call as-option, -Wa$(comma)-isa=foo,)

@@ -66,9 +69,11 @@ as-option = $(shell if $(CC) $(CFLAGS) $
 # as-instr
 # Usage: cflags-y += $(call as-instr, instr, option1, option2)
 
-as-instr = $(shell if echo -e "$(1)" | $(AS) >/dev/null 2>&1 -W -Z -o astest$$$$.out ; \
-		   then echo "$(2)"; else echo "$(3)"; fi; \
-	           rm -f astest$$$$.out)
+as-instr = $(shell if echo -e "$(1)" | \
+		      $(CC) $(AFLAGS) -c -xassembler - \
+			    -o $(TMPOUT)astest$$$$.out > /dev/null 2>&1; \
+		   then rm $(TMPOUT)astest$$$$.out; echo "$(2)"; \
+		   else echo "$(3)"; fi)
 
 # cc-option
 # Usage: cflags-y += $(call cc-option, -march=winchip-c6, -march=i586)

@@ -97,10 +102,10 @@ cc-ifversion = $(shell if [ $(call cc-ve
 
 # ld-option
 # Usage: ldflags += $(call ld-option, -Wl$(comma)--hash-style=both)
-ld-option = $(shell if $(CC) $(1) \
-			     -nostdlib -o ldtest$$$$.out -xc /dev/null \
-             > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi; \
-	     rm -f ldtest$$$$.out)
+ld-option = $(shell if $(CC) $(1) -nostdlib -xc /dev/null \
+			     -o $(TMPOUT)ldtest$$$$.out > /dev/null 2>&1; \
+             then rm $(TMPOUT)ldtest$$$$.out; echo "$(1)"; \
+             else echo "$(2)"; fi)
 
 ###
 # Shorthand for $(Q)$(MAKE) -f scripts/Makefile.build obj=

Horst Schirmeier

2006-12-06 03:46 UTC #18 lore raw

Hi,

Quoted 12 lines · Roman Zippel

Quoted 2 lines · Andrew Morton
kbuild-dont-put-temp-files-in-the-source-tree.patch actually-delete-the-as-instr-ld-option-tmp-file.patch
Andi had objections about the mktemp usage and I agree with him. The proposed patch in bugzilla didn't have this and no further justification was given for why it's needed.
Below is a replacement patch with some improvements:
- kbuild doesn't use $(AS), so use $(CC)
- tmp dir needs only to be calculated once
- reformat a bit to keep it under 80 columns and to be more readable

Quoted 3 lines

bye, Roman
Signed-off-by Roman Zippel <[email protected]>

This patch looks good to me, and I'd prefer it over the (twice corrected) one currently in -mm.

- It's close to what Daniel Drake proposed recently [1] (main
difference: corrects the very same problem for ld-option, which might otherwise bite us later) and what's already in Gentoo's patchkernel,

- it abstains from using mktemp, hopefully making Andi happy ;),

- resolves the current Gentoo sandbox issues without touching more kbuild code than necessary.

Kind regards,
Horst

[1]Message-Id: <[email protected]>

Signature · 1 line

PGP-Key 0xD40E0E7A

Dave Jones

2006-12-05 16:02 UTC #19 lore raw

Quoted 7 lines · Andrew Morton

cpufreq-fix-bug-in-duplicate-freq-elimination-code-in-acpi-cpufreq.patch remove-hotplug-cpu-crap-from-cpufreq.patch cpufreq-select-consistently-re-2619-rc5-mm1.patch cpufreq-set-policy-curfreq-on-initialization.patch bug-fix-for-acpi-cpufreq-and-cpufreq_stats-oops-on-frequency-change-notification.patch
Sent to cpufreq maintainer

I'm travelling right now, and somehow managed to oops my home router 3000 miles away making it hard for me to access mail & stuff. (That "page count went negative" BUG() bit me when I did a killall -9 vpnc)

Full bug report, and processing of this backlog will happen as soon as I get back on Sunday :)

Dave

Dave Jones

2006-12-12 17:49 UTC #20 lore raw

Quoted 8 lines · Dave Jones

Quoted 6 lines · Andrew Morton
cpufreq-fix-bug-in-duplicate-freq-elimination-code-in-acpi-cpufreq.patch remove-hotplug-cpu-crap-from-cpufreq.patch cpufreq-select-consistently-re-2619-rc5-mm1.patch cpufreq-set-policy-curfreq-on-initialization.patch bug-fix-for-acpi-cpufreq-and-cpufreq_stats-oops-on-frequency-change-notification.patch

Quoted 5 lines

Quoted 1 line
Sent to cpufreq maintainer
I'm travelling right now, and somehow managed to oops my home router 3000 miles away making it hard for me to access mail & stuff. (That "page count went negative" BUG() bit me when I did a killall -9 vpnc)

Finally managed to fish this out of the logs..

Eeek! page_mapcount(page) went negative! (-2)
page->flags = 404
page->count = 1
page->mapping = 00000000

------------[ cut here ]------------
kernel BUG at mm/rmap.c:587!
invalid opcode: 0000 [#2]
SMP 
last sysfs file: /class/net/lo/type
Modules linked in: tun ipt_MASQUERADE iptable_nat ip_nat ipt_LOG xt_limit ipv6 ip_conntrack_netbios_ns ipt_REJECT xt_state ip_conntrack nfnetlink xt_tcpudp iptable_filter ip_tables x_tables video sbs i2c_ec button battery asus_acpi ac parport_pc lp parport pcspkr ide_cd i2c_viapro i2c_core cdrom 3c59x via_rhine via_ircc mii irda crc_ccitt serio_raw dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd ehci_hcd ohci_hcd uhci_hcd
CPU:    0
EIP:    0060:[<c046396c>]    Not tainted VLI
EFLAGS: 00010246   (2.6.18-1.2849.fc6 #1)

EIP is at page_remove_rmap+0x8b/0xac
eax: 00000000 ebx: c1000000 ecx: ffffffff edx: 00000046
esi: dcaa2f98 edi: 00000020 ebp: cd3f7f00 esp: cd3e4ddc
ds: 007b es: 007b ss: 0068
Process vpnc (pid: 3367, ti=cd3e4000 task=cd3a0b50 task.ti=cd3e4000)

Stack: c0638bad 00000000 c1000000 09bc0000 c045e199 00000000 dcaa2f98 cd3e4e54 
       003ebff8 00000000 00000001 09bdb000 cd377098 c1624a80 c13e5f60 fffffffa 
       ffffffff c1624ad4 cd377098 09bdb000 00000000 cd3e4e54 c1485d4c c1624a80 
Call Trace:
 [<c045e199>] unmap_vmas+0x28e/0x504
 [<c0460c0f>] exit_mmap+0x77/0xf1
 [<c0422d75>] mmput+0x34/0x7a
 [<c0427818>] do_exit+0x20b/0x776
 [<c0427df9>] sys_exit_group+0x0/0xd
 [<cd39a644>] 0xcd39a644
DWARF2 unwinder stuck at 0xcd39a644
Leftover inexact backtrace:
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c0430828>] get_signal_to_deliver+0x38b/0x3b3
 [<c04068d1>] do_IRQ+0xb0/0xbc
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c0403626>] do_notify_resume+0x7e/0x6c1
 [<c045f135>] __handle_mm_fault+0x82c/0x860
 [<c04068d1>] do_IRQ+0xb0/0xbc
 [<c061455e>] _spin_unlock_irq+0x5/0x7
 [<c0612ccc>] schedule+0x960/0x9dd
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c04040a2>] work_notifysig+0x13/0x19
 =======================
Code: 82 1e fc ff 8b 43 10 c7 04 24 ad 8b 63 c0 89 44 24 04 e8 6f 1e fc ff 8b 46 40 85 c0 74 0d 8b 50 08 b8 c6 8b 63 c0 e8 07 ce fd ff <0f> 0b 4b 02 42 8b 63 c0 8b 53 10 89 d8 59 5b 5b 5e 83 f2 01 83 
EIP: [<c046396c>] page_remove_rmap+0x8b/0xac SS:ESP 0068:cd3e4ddc
 <3>BUG: sleeping function called from invalid context at kernel/rwsem.c:20
in_atomic():1, irqs_disabled():0
 [<c04051db>] dump_trace+0x69/0x1af
 [<c0405339>] show_trace_log_lvl+0x18/0x2c
 [<c04058ed>] show_trace+0xf/0x11
 [<c04059ea>] dump_stack+0x15/0x17
 [<c04394a2>] down_read+0x12/0x20
 [<c0431601>] blocking_notifier_call_chain+0xe/0x29
 [<c0427628>] do_exit+0x1b/0x776
 [<c040588e>] die+0x29d/0x2c2
 [<c0405fd3>] do_invalid_op+0xa2/0xab
 [<c0404b85>] error_code+0x39/0x40
DWARF2 unwinder stuck at error_code+0x39/0x40
Leftover inexact backtrace:
 [<c046396c>] page_remove_rmap+0x8b/0xac
 [<c045e199>] unmap_vmas+0x28e/0x504
 [<c0460c0f>] exit_mmap+0x77/0xf1
 [<c0422d75>] mmput+0x34/0x7a
 [<c0427818>] do_exit+0x20b/0x776
 [<c042f153>] __dequeue_signal+0x151/0x15c
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c0427df9>] sys_exit_group+0x0/0xd
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c0430828>] get_signal_to_deliver+0x38b/0x3b3
 [<c04068d1>] do_IRQ+0xb0/0xbc
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c0403626>] do_notify_resume+0x7e/0x6c1
 [<c045f135>] __handle_mm_fault+0x82c/0x860
 [<c04068d1>] do_IRQ+0xb0/0xbc
 [<c061455e>] _spin_unlock_irq+0x5/0x7
 [<c0612ccc>] schedule+0x960/0x9dd
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c04040a2>] work_notifysig+0x13/0x19
 =======================
Fixing recursive fault but reboot is needed!
BUG: scheduling while atomic: vpnc/0x00000001/3367
 [<c04051db>] dump_trace+0x69/0x1af
 [<c0405339>] show_trace_log_lvl+0x18/0x2c
 [<c04058ed>] show_trace+0xf/0x11
 [<c04059ea>] dump_stack+0x15/0x17
 [<c06123af>] schedule+0x43/0x9dd
 [<c04276f7>] do_exit+0xea/0x776
 [<c040588e>] die+0x29d/0x2c2
 [<c0405fd3>] do_invalid_op+0xa2/0xab
 [<c0404b85>] error_code+0x39/0x40
DWARF2 unwinder stuck at error_code+0x39/0x40
Leftover inexact backtrace:
 [<c046396c>] page_remove_rmap+0x8b/0xac
 [<c045e199>] unmap_vmas+0x28e/0x504
 [<c0460c0f>] exit_mmap+0x77/0xf1
 [<c0422d75>] mmput+0x34/0x7a
 [<c0427818>] do_exit+0x20b/0x776
 [<c042f153>] __dequeue_signal+0x151/0x15c
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c0427df9>] sys_exit_group+0x0/0xd
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c0430828>] get_signal_to_deliver+0x38b/0x3b3
 [<c04068d1>] do_IRQ+0xb0/0xbc
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c0403626>] do_notify_resume+0x7e/0x6c1
 [<c045f135>] __handle_mm_fault+0x82c/0x860
 [<c04068d1>] do_IRQ+0xb0/0xbc
 [<c061455e>] _spin_unlock_irq+0x5/0x7
 [<c0612ccc>] schedule+0x960/0x9dd
 [<c0615375>] do_page_fault+0x0/0x4db
 [<c04040a2>] work_notifysig+0x13/0x19
 =======================

That was from a 2.6.18.3 kernel iirc.

Dave

Signature · 1 line

http://www.codemonkey.org.uk

Nick Piggin

2006-12-19 06:20 UTC #21 lore raw

Quoted 1 line · Dave Jones

Eeek! page_mapcount(page) went negative! (-2)

Hmm, probably happened once before, too.

Quoted 1 line

page->flags = 404

What's that? PG_referenced|PG_reserved? So I'd say it is likely that some driver has got its refcounting wrong.

Unfortunately, this debugging output is almost useless when it comes to trying to track down the problem any further.

And I see we've got another report for 2.6.19.1 from Chris, which is equally vague.

IMO the pattern is much too consistent to be able to attribute them all to hardware problems. And considering it takes so long for these things to appear, can we get something like the attached patch upstream at least until we manage to stamp them out? Any other debugging info we can add?

Signature · 1 line

SUSE Labs, Novell Inc.

include/linux/rmap.h +1 -1

Index: linux-2.6/include/linux/rmap.h
===================================================================
@@ -72,7 +72,7 @@ void __anon_vma_link(struct vm_area_stru
 void page_add_anon_rmap(struct page *, struct vm_area_struct *, unsigned long);
 void page_add_new_anon_rmap(struct page *, struct vm_area_struct *, unsigned long);
 void page_add_file_rmap(struct page *);
-void page_remove_rmap(struct page *);
+void page_remove_rmap(struct page *, struct vm_area_struct *);
 
 /**
  * page_dup_rmap - duplicate pte mapping to a page

mm/filemap_xip.c +1 -1

Index: linux-2.6/mm/filemap_xip.c
===================================================================
@@ -189,7 +189,7 @@ __xip_unmap (struct address_space * mapp
 			/* Nuke the page table entry. */
 			flush_cache_page(vma, address, pte_pfn(*pte));
 			pteval = ptep_clear_flush(vma, address, pte);
-			page_remove_rmap(page);
+			page_remove_rmap(page, vma);
 			dec_mm_counter(mm, file_rss);
 			BUG_ON(pte_dirty(pteval));
 			pte_unmap_unlock(pte, ptl);

mm/fremap.c +1 -1

Index: linux-2.6/mm/fremap.c
===================================================================
@@ -33,7 +33,7 @@ static int zap_pte(struct mm_struct *mm,
 		if (page) {
 			if (pte_dirty(pte))
 				set_page_dirty(page);
-			page_remove_rmap(page);
+			page_remove_rmap(page, vma);
 			page_cache_release(page);
 		}
 	} else {

mm/memory.c +2 -2

Index: linux-2.6/mm/memory.c
===================================================================
@@ -681,7 +681,7 @@ static unsigned long zap_pte_range(struc
 					mark_page_accessed(page);
 				file_rss--;
 			}
-			page_remove_rmap(page);
+			page_remove_rmap(page, vma);
 			tlb_remove_page(tlb, page);
 			continue;
 		}

@@ -1576,7 +1576,7 @@ gotten:
 	page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
 	if (likely(pte_same(*page_table, orig_pte))) {
 		if (old_page) {
-			page_remove_rmap(old_page);
+			page_remove_rmap(old_page, vma);
 			if (!PageAnon(old_page)) {
 				dec_mm_counter(mm, file_rss);
 				inc_mm_counter(mm, anon_rss);

mm/rmap.c +7 -3

Index: linux-2.6/mm/rmap.c
===================================================================
@@ -47,6 +47,7 @@
 #include <linux/rmap.h>
 #include <linux/rcupdate.h>
 #include <linux/module.h>
+#include <linux/kallsyms.h>
 
 #include <asm/tlbflush.h>

@@ -567,7 +568,7 @@ void page_add_file_rmap(struct page *pag
  *
  * The caller needs to hold the pte lock.
  */
-void page_remove_rmap(struct page *page)
+void page_remove_rmap(struct page *page, struct vm_area_struct *vma)
 {
 	if (atomic_add_negative(-1, &page->_mapcount)) {
 		if (unlikely(page_mapcount(page) < 0)) {

@@ -575,6 +576,9 @@ void page_remove_rmap(struct page *page)
 			printk (KERN_EMERG "  page->flags = %lx\n", page->flags);
 			printk (KERN_EMERG "  page->count = %x\n", page_count(page));
 			printk (KERN_EMERG "  page->mapping = %p\n", page->mapping);
+			print_symbol (KERN_EMERG "  vma->vm_ops = %s\n", (unsigned long)vma->vm_ops);
+			if (vma->vm_ops)
+				print_symbol (KERN_EMERG "  vma->vm_ops->nopage = %s\n", (unsigned long)vma->vm_ops->nopage);
 			BUG();
 		}

@@ -679,7 +683,7 @@ static int try_to_unmap_one(struct page 
 		dec_mm_counter(mm, file_rss);
 
 
-	page_remove_rmap(page);
+	page_remove_rmap(page, vma);
 	page_cache_release(page);
 
 out_unmap:

@@ -769,7 +773,7 @@ static void try_to_unmap_cluster(unsigne
 		if (pte_dirty(pteval))
 			set_page_dirty(page);
 
-		page_remove_rmap(page);
+		page_remove_rmap(page, vma);
 		page_cache_release(page);
 		dec_mm_counter(mm, file_rss);
 		(*mapcount)--;

Dave Jones

Acked-by

2006-12-19 06:45 UTC #22 lore raw

Quoted 5 lines · Nick Piggin

Quoted 1 line · Dave Jones
Eeek! page_mapcount(page) went negative! (-2)
Hmm, probably happened once before, too.

You're right. Going back further in the log, I noticed
that it had happened again exactly at the time that cron restarted vpnc. The first time, the flags were different..

 Dec  4 00:01:03 firewall kernel: Eeek! page_mapcount(page) went negative! (-1)
 Dec  4 00:01:03 firewall kernel:   page->flags = 400
 Dec  4 00:01:03 firewall kernel:   page->count = 1
 Dec  4 00:01:03 firewall kernel:   page->mapping = 00000000

Quoted 4 lines

Quoted 1 line
page->flags = 404
What's that? PG_referenced|PG_reserved? So I'd say it is likely that some driver has got its refcounting wrong.

At the time that it bit me, here's what was loaded..

tun ipt_MASQUERADE iptable_nat ip_nat ipt_LOG xt_limit ipv6 ip_conntrack_netbios_ns ipt_REJECT xt_state ip_conntrack nfnetlink xt_tcpudp iptable_filter ip_tables x_tables video sbs i2c_ec button battery asus_acpi ac parport_pc lp parport pcspkr ide_cd i2c_viapro i2c_core cdrom 3c59x via_rhine via_ircc mii irda crc_ccitt serio_raw dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd ehci_hcd ohci_hcd uhci_hcd

The scary ones (i2c, irda) weren't in use at all, and had never been opened afaik, so the potential for those to be corrupting memory is slim, but not out of the question. (Why the hell asus_acpi is loaded is a mystery, this isn't an Asus, or a laptop. Probably dumb initscripts).

Quoted 2 lines

And I see we've got another report for 2.6.19.1 from Chris, which is equally vague.

I'll be moving that box to 2.6.19.x at some point real soon, so I'll holler if I see it again on a later kernel.

Quoted 4 lines

IMO the pattern is much too consistent to be able to attribute them all to hardware problems. And considering it takes so long for these things to appear, can we get something like the attached patch upstream at least until we manage to stamp them out?

Sounds like a good idea to me.

Acked-by Dave Jones <[email protected]>

Quoted 1 line

Any other debugging info we can add?

Would it be useful to print the pfn of the page ?
In cases like mine, where it bit twice before it killed the box, it might be interesting to see if its always the same page. Not sure what that would prove/disprove though.

Dave

Signature · 1 line

http://www.codemonkey.org.uk

Nick Piggin

2006-12-19 07:03 UTC #23 lore raw

Quoted 7 lines · Dave Jones

Quoted 5 lines · Nick Piggin
Quoted 1 line · Dave Jones
Eeek! page_mapcount(page) went negative! (-2)
Hmm, probably happened once before, too.

Quoted 8 lines

You're right. Going back further in the log, I noticed
that it had happened again exactly at the time that cron restarted vpnc. The first time, the flags were different..
 Dec  4 00:01:03 firewall kernel: Eeek! page_mapcount(page) went negative! (-1)
 Dec  4 00:01:03 firewall kernel:   page->flags = 400
 Dec  4 00:01:03 firewall kernel:   page->count = 1
 Dec  4 00:01:03 firewall kernel:   page->mapping = 00000000

Still reserved, with a NULL mapping. I'd say it could be the same page.

Quoted 15 lines

Quoted 5 lines
Quoted 2 lines
page->flags = 404
What's that? PG_referenced|PG_reserved? So I'd say it is likely that some driver has got its refcounting wrong.
At the time that it bit me, here's what was loaded..
tun ipt_MASQUERADE iptable_nat ip_nat ipt_LOG xt_limit ipv6 ip_conntrack_netbios_ns ipt_REJECT xt_state ip_conntrack nfnetlink xt_tcpudp iptable_filter ip_tables x_tables video sbs i2c_ec button battery asus_acpi ac parport_pc lp parport pcspkr ide_cd i2c_viapro i2c_core cdrom 3c59x via_rhine via_ircc mii irda crc_ccitt serio_raw dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd ehci_hcd ohci_hcd uhci_hcd

Quoted 4 lines

The scary ones (i2c, irda) weren't in use at all, and had never been opened afaik, so the potential for those to be corrupting memory is slim, but not out of the question. (Why the hell asus_acpi is loaded is a mystery, this isn't an Asus, or a laptop. Probably dumb initscripts).

OK that could be useful if I do some grepping and see which ones are setting PG_reserved.

Quoted 6 lines

Quoted 2 lines
And I see we've got another report for 2.6.19.1 from Chris, which is equally vague.
I'll be moving that box to 2.6.19.x at some point real soon, so I'll holler if I see it again on a later kernel.

Quoted 8 lines

Quoted 4 lines
IMO the pattern is much too consistent to be able to attribute them all to hardware problems. And considering it takes so long for these things to appear, can we get something like the attached patch upstream at least until we manage to stamp them out?
Sounds like a good idea to me.
Acked-by Dave Jones <[email protected]>

Thanks.

Quoted 7 lines

Quoted 2 lines
Any other debugging info we can add?
Would it be useful to print the pfn of the page ?
In cases like mine, where it bit twice before it killed the box, it might be interesting to see if its always the same page. Not sure what that would prove/disprove though.

Might help. I guess the site where it is allocated from might be another one, although I'm hoping that if we know what ->nopage is being used then we'll be able to track it. OTOH it may be using remap_pfn_range from fops->mmap, rather than nopage... I wonder how we could get at that info? vma->vm_file->f_op->mmap?

Signature · 1 line

SUSE Labs, Novell Inc.

include/linux/rmap.h +1 -1

Index: linux-2.6/include/linux/rmap.h
===================================================================
@@ -72,7 +72,7 @@ void __anon_vma_link(struct vm_area_stru
 void page_add_anon_rmap(struct page *, struct vm_area_struct *, unsigned long);
 void page_add_new_anon_rmap(struct page *, struct vm_area_struct *, unsigned long);
 void page_add_file_rmap(struct page *);
-void page_remove_rmap(struct page *);
+void page_remove_rmap(struct page *, struct vm_area_struct *);
 
 /**
  * page_dup_rmap - duplicate pte mapping to a page

mm/filemap_xip.c +1 -1

Index: linux-2.6/mm/filemap_xip.c
===================================================================
@@ -189,7 +189,7 @@ __xip_unmap (struct address_space * mapp
 			/* Nuke the page table entry. */
 			flush_cache_page(vma, address, pte_pfn(*pte));
 			pteval = ptep_clear_flush(vma, address, pte);
-			page_remove_rmap(page);
+			page_remove_rmap(page, vma);
 			dec_mm_counter(mm, file_rss);
 			BUG_ON(pte_dirty(pteval));
 			pte_unmap_unlock(pte, ptl);

mm/fremap.c +1 -1

Index: linux-2.6/mm/fremap.c
===================================================================
@@ -33,7 +33,7 @@ static int zap_pte(struct mm_struct *mm,
 		if (page) {
 			if (pte_dirty(pte))
 				set_page_dirty(page);
-			page_remove_rmap(page);
+			page_remove_rmap(page, vma);
 			page_cache_release(page);
 		}
 	} else {

mm/memory.c +2 -2

Index: linux-2.6/mm/memory.c
===================================================================
@@ -681,7 +681,7 @@ static unsigned long zap_pte_range(struc
 					mark_page_accessed(page);
 				file_rss--;
 			}
-			page_remove_rmap(page);
+			page_remove_rmap(page, vma);
 			tlb_remove_page(tlb, page);
 			continue;
 		}

@@ -1576,7 +1576,7 @@ gotten:
 	page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
 	if (likely(pte_same(*page_table, orig_pte))) {
 		if (old_page) {
-			page_remove_rmap(old_page);
+			page_remove_rmap(old_page, vma);
 			if (!PageAnon(old_page)) {
 				dec_mm_counter(mm, file_rss);
 				inc_mm_counter(mm, anon_rss);

mm/rmap.c +10 -3

Index: linux-2.6/mm/rmap.c
===================================================================
@@ -47,6 +47,7 @@
 #include <linux/rmap.h>
 #include <linux/rcupdate.h>
 #include <linux/module.h>
+#include <linux/kallsyms.h>
 
 #include <asm/tlbflush.h>

@@ -567,14 +568,20 @@ void page_add_file_rmap(struct page *pag
  *
  * The caller needs to hold the pte lock.
  */
-void page_remove_rmap(struct page *page)
+void page_remove_rmap(struct page *page, struct vm_area_struct *vma)
 {
 	if (atomic_add_negative(-1, &page->_mapcount)) {
 		if (unlikely(page_mapcount(page) < 0)) {
 			printk (KERN_EMERG "Eeek! page_mapcount(page) went negative! (%d)\n", page_mapcount(page));
+			printk (KERN_EMERG "  page pfn = %lx\n", page_to_pfn(page));
 			printk (KERN_EMERG "  page->flags = %lx\n", page->flags);
 			printk (KERN_EMERG "  page->count = %x\n", page_count(page));
 			printk (KERN_EMERG "  page->mapping = %p\n", page->mapping);
+			print_symbol (KERN_EMERG "  vma->vm_ops = %s\n", (unsigned long)vma->vm_ops);
+			if (vma->vm_ops)
+				print_symbol (KERN_EMERG "  vma->vm_ops->nopage = %s\n", (unsigned long)vma->vm_ops->nopage);
+			if (vma->vm_file && vma->vm_file->f_op)
+				print_symbol (KERN_EMERG "  vma->vm_file->f_op->mmap = %s\n", (unsigned long)vma->vm_file->f_op->mmap);
 			BUG();
 		}

@@ -679,7 +686,7 @@ static int try_to_unmap_one(struct page 
 		dec_mm_counter(mm, file_rss);
 
 
-	page_remove_rmap(page);
+	page_remove_rmap(page, vma);
 	page_cache_release(page);
 
 out_unmap:

@@ -769,7 +776,7 @@ static void try_to_unmap_cluster(unsigne
 		if (pte_dirty(pteval))
 			set_page_dirty(page);
 
-		page_remove_rmap(page);
+		page_remove_rmap(page, vma);
 		page_cache_release(page);
 		dec_mm_counter(mm, file_rss);
 		(*mapcount)--;

Dave Jones

2007-01-07 17:36 UTC #24 lore raw

Re: page_mapcount(page) went negative

In reply to Nick Piggin

Quoted 14 lines · Nick Piggin

Quoted 1 line · Dave Jones
Eeek! page_mapcount(page) went negative! (-2)
Hmm, probably happened once before, too.
Quoted 1 line
page->flags = 404
What's that? PG_referenced|PG_reserved? So I'd say it is likely that some driver has got its refcounting wrong.
Unfortunately, this debugging output is almost useless when it comes to trying to track down the problem any further.

Quoted 8 lines

And I see we've got another report for 2.6.19.1 from Chris, which is equally vague.
IMO the pattern is much too consistent to be able to attribute them all to hardware problems. And considering it takes so long for these things to appear, can we get something like the attached patch upstream at least until we manage to stamp them out? Any other debugging info we can add?

I finally got another trace out of a 2.6.19.1 with this debug patch..
I'm starting to wonder if this is hardware related.
That box makes me suspicious as its motherboard has a bunch
of bulging capacitors on it. I had three of these, the previous
two died like so: http://www.flickr.com/photos/kernelslacker/251807263/in/set-72157594298237090/
This board hasn't started 'oozing' yet, but I think it may be on
the cards.

iirc, the previous oopses were around the same time of morning, whilst cron.daily was hammering the hell out of the disk. Either there's a subtle bug, or this board just can't take the pressure.

Dave

Jan  7 04:24:35 firewall kernel: Eeek! page_mapcount(page) went negative! (-1)
Jan  7 04:24:35 firewall kernel:   page->flags = 414
Jan  7 04:24:35 firewall kernel:   page->count = 1
Jan  7 04:24:35 firewall kernel:   page->mapping = 00000000
Jan  7 04:24:35 firewall kernel:   vma->vm_ops->nopage = filemap_nopage+0x0/0x33e
Jan  7 04:24:35 firewall kernel: ------------[ cut here ]------------
Jan  7 04:24:35 firewall kernel: kernel BUG at mm/rmap.c:581!
Jan  7 04:24:35 firewall kernel: invalid opcode: 0000 [#1]
Jan  7 04:24:35 firewall kernel: SMP 
Jan  7 04:24:35 firewall kernel: last sysfs file: /class/net/lo/type
Jan  7 04:24:35 firewall kernel: Modules linked in: tun ipt_MASQUERADE iptable_nat ip_nat ipt_LOG xt_limit ipv6 ip_conntrack_netbios_ns ipt_REJECT xt_state i
p_conntrack nfnetlink xt_tcpudp iptable_filter ip_tables x_tables video sbs i2c_ec button battery asus_acpi ac parport_pc lp parport sr_mod sg pcspkr i2c_via
pro ide_cd i2c_core 3c59x via_rhine cdrom mii via_ircc irda crc_ccitt serio_raw dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd ehci_hcd ohci_hcd uhci_hcd
Jan  7 04:24:35 firewall kernel: CPU:    0
Jan  7 04:24:35 firewall kernel: EIP:    0060:[<c046d995>]    Not tainted VLI
Jan  7 04:24:35 firewall kernel: EFLAGS: 00010286   (2.6.19-1.2886.fc6debug #1)
Jan  7 04:24:35 firewall kernel: EIP is at page_remove_rmap+0x8b/0xac
Jan  7 04:24:35 firewall kernel: eax: 00000034   ebx: c1000000   ecx: c042818c   edx: da0b3550
Jan  7 04:24:35 firewall kernel: esi: daecd0c0   edi: 00000020   ebp: cd386140   esp: ce2f4dc4
Jan  7 04:24:35 firewall kernel: ds: 007b   es: 007b   ss: 0068
Jan  7 04:24:35 firewall kernel: Process zcat (pid: 22721, ti=ce2f4000 task=da0b3550 task.ti=ce2f4000)
Jan  7 04:24:35 firewall kernel: Stack: c066b7b6 00000000 c1000000 08050000 c0467c63 c06c7f10 00000046 00000000 
Jan  7 04:24:35 firewall kernel:        daecd0c0 ce2f4e44 003f7ffd 00000000 00000001 08055000 cf354080 dde93520 
Jan  7 04:24:35 firewall kernel:        c1401e80 00000000 fffffff9 dde9358c cf354080 08055000 00000000 ce2f4e44 
Jan  7 04:24:35 firewall kernel: Call Trace:
Jan  7 04:24:35 firewall kernel:  [<c0467c63>] unmap_vmas+0x2a6/0x536
Jan  7 04:24:35 firewall kernel:  [<c046aaa8>] exit_mmap+0x77/0x105
Jan  7 04:24:35 firewall kernel:  [<c0425471>] mmput+0x37/0x80
Jan  7 04:24:35 firewall kernel:  [<c042a4b5>] do_exit+0x21c/0x7a3
Jan  7 04:24:35 firewall kernel:  [<c042aac9>] sys_exit_group+0x0/0xd
Jan  7 04:24:35 firewall kernel:  [<c043383d>] get_signal_to_deliver+0x38b/0x3b3
Jan  7 04:24:35 firewall kernel:  [<c0403677>] do_notify_resume+0x83/0x6c6
Jan  7 04:24:35 firewall kernel:  [<c0404159>] work_notifysig+0x13/0x1a
Jan  7 04:24:35 firewall kernel:  [<0804a84e>] 0x804a84e
Jan  7 04:24:35 firewall kernel:  =======================
Jan  7 04:24:35 firewall kernel: Code: 4c a4 fb ff 8b 43 10 c7 04 24 b6 b7 66 c0 89 44 24 04 e8 39 a4 fb ff 8b 46 40 85 c0 74 0d 8b 50 08 b8 cf b7 66 c0 e8 d
8 a9 fd ff <0f> 0b 45 02 4b b7 66 c0 8b 53 10 89 d8 59 5b 5b 5e 83 f2 01 83 
Jan  7 04:24:35 firewall kernel: EIP: [<c046d995>] page_remove_rmap+0x8b/0xac SS:ESP 0068:ce2f4dc4
Jan  7 04:24:35 firewall kernel:  <3>BUG: sleeping function called from invalid context at kernel/rwsem.c:20
Jan  7 04:24:35 firewall kernel: in_atomic():1, irqs_disabled():0
Jan  7 04:24:35 firewall kernel:  [<c0405108>] dump_trace+0x69/0x1b6
Jan  7 04:24:35 firewall kernel:  [<c040526d>] show_trace_log_lvl+0x18/0x2c
Jan  7 04:24:35 firewall kernel:  [<c0405868>] show_trace+0xf/0x11
Jan  7 04:24:35 firewall kernel:  [<c0405965>] dump_stack+0x15/0x17
Jan  7 04:24:35 firewall kernel:  [<c043ccca>] down_read+0x15/0x4e
Jan  7 04:24:35 firewall kernel:  [<c04345b6>] blocking_notifier_call_chain+0xe/0x29
Jan  7 04:24:35 firewall kernel:  [<c042a2b4>] do_exit+0x1b/0x7a3
Jan  7 04:24:35 firewall kernel:  [<c0405809>] die+0x2c3/0x2e8
Jan  7 04:24:35 firewall kernel:  [<c0405d4a>] do_invalid_op+0xa2/0xab
Jan  7 04:24:35 firewall kernel:  [<c063f7a1>] error_code+0x39/0x40
Jan  7 04:24:35 firewall kernel:  [<c046d995>] page_remove_rmap+0x8b/0xac
Jan  7 04:24:35 firewall kernel:  [<c0467c63>] unmap_vmas+0x2a6/0x536
Jan  7 04:24:35 firewall kernel:  [<c046aaa8>] exit_mmap+0x77/0x105
Jan  7 04:24:35 firewall kernel:  [<c0425471>] mmput+0x37/0x80
Jan  7 04:24:35 firewall kernel:  [<c042a4b5>] do_exit+0x21c/0x7a3
Jan  7 04:24:35 firewall kernel:  [<c042aac9>] sys_exit_group+0x0/0xd
Jan  7 04:24:35 firewall kernel:  [<c043383d>] get_signal_to_deliver+0x38b/0x3b3
Jan  7 04:24:35 firewall kernel:  [<c0403677>] do_notify_resume+0x83/0x6c6
Jan  7 04:24:35 firewall kernel:  [<c0404159>] work_notifysig+0x13/0x1a
Jan  7 04:24:35 firewall kernel:  [<0804a84e>] 0x804a84e
Jan  7 04:24:35 firewall kernel:  =======================
Jan  7 04:24:35 firewall kernel: Fixing recursive fault but reboot is needed!
Jan  7 04:24:35 firewall kernel: BUG: scheduling while atomic: zcat/0x00000001/22721
Jan  7 04:24:35 firewall kernel:  [<c0405108>] dump_trace+0x69/0x1b6
Jan  7 04:24:35 firewall kernel:  [<c040526d>] show_trace_log_lvl+0x18/0x2c
Jan  7 04:24:35 firewall kernel:  [<c0405868>] show_trace+0xf/0x11
Jan  7 04:24:35 firewall kernel:  [<c0405965>] dump_stack+0x15/0x17
Jan  7 04:24:35 firewall kernel:  [<c063c72f>] __sched_text_start+0x4f/0xa58
Jan  7 04:24:35 firewall kernel:  [<c042a397>] do_exit+0xfe/0x7a3
Jan  7 04:24:35 firewall kernel:  [<c0405809>] die+0x2c3/0x2e8
Jan  7 04:24:35 firewall kernel:  [<c0405d4a>] do_invalid_op+0xa2/0xab
Jan  7 04:24:35 firewall kernel:  [<c063f7a1>] error_code+0x39/0x40
Jan  7 04:24:35 firewall kernel:  [<c046d995>] page_remove_rmap+0x8b/0xac
Jan  7 04:24:35 firewall kernel:  [<c0467c63>] unmap_vmas+0x2a6/0x536
Jan  7 04:24:35 firewall kernel:  [<c046aaa8>] exit_mmap+0x77/0x105
Jan  7 04:24:35 firewall kernel:  [<c0425471>] mmput+0x37/0x80
Jan  7 04:24:35 firewall kernel:  [<c042a4b5>] do_exit+0x21c/0x7a3
Jan  7 04:24:35 firewall kernel:  [<c042aac9>] sys_exit_group+0x0/0xd
Jan  7 04:24:35 firewall kernel:  [<c043383d>] get_signal_to_deliver+0x38b/0x3b3
Jan  7 04:24:35 firewall kernel:  [<c0403677>] do_notify_resume+0x83/0x6c6
Jan  7 04:24:35 firewall kernel:  [<c0404159>] work_notifysig+0x13/0x1a
Jan  7 04:24:35 firewall kernel:  [<0804a84e>] 0x804a84e
Jan  7 04:24:35 firewall kernel:  =======================

Signature · 1 line

http://www.codemonkey.org.uk

Nick Piggin

2007-01-10 23:53 UTC #25 lore raw

Re: page_mapcount(page) went negative

Quoted 16 lines · Dave Jones

Quoted 5 lines · Nick Piggin
IMO the pattern is much too consistent to be able to attribute them all to hardware problems. And considering it takes so long for these things to appear, can we get something like the attached patch upstream at least until we manage to stamp them out? Any other debugging info we can add?
I finally got another trace out of a 2.6.19.1 with this debug patch..
I'm starting to wonder if this is hardware related.
That box makes me suspicious as its motherboard has a bunch
of bulging capacitors on it. I had three of these, the previous
two died like so: http://www.flickr.com/photos/kernelslacker/251807263/in/set-72157594298237090/
This board hasn't started 'oozing' yet, but I think it may be on
the cards.

Quoted 3 lines

iirc, the previous oopses were around the same time of morning, whilst cron.daily was hammering the hell out of the disk. Either there's a subtle bug, or this board just can't take the pressure.

The reason why I'm inclined to think there might be a kernel bug is that this bug seems to trigger a disproportionately high number of times if you consider the frequency of reports vs frequency of reports of bad page state or bad pte bugs, for example.

Quoted 2 lines

Jan  7 04:24:35 firewall kernel: Eeek! page_mapcount(page) went negative! (-1)
Jan  7 04:24:35 firewall kernel:   page->flags = 414

referenced|dirty|reserved

Quoted 2 lines

Jan  7 04:24:35 firewall kernel:   page->count = 1
Jan  7 04:24:35 firewall kernel:   page->mapping = 00000000

OK, so it is !PageAnon. Seems like the file was truncated?! Then it shouldn't be mapped at all? OTOH the truncate could easily have skipped such a page:
truncate doesn't rely on mapcount like invalidate.

Hmm, it could be invalidated or vmtruncate_range()d, if you've hit the nopage vs invalidate race. But would that cause the mapcount to skew? I don't see how...

Quoted 1 line

Jan  7 04:24:35 firewall kernel:   vma->vm_ops->nopage = filemap_nopage+0x0/0x33e

filemap_nopage. Then PG_reserved should not be getting set, AFAIKS.

Might pte corruption have caused us to get completely the wrong page here?

Quoted 28 lines

Jan  7 04:24:35 firewall kernel: ------------[ cut here ]------------
Jan  7 04:24:35 firewall kernel: kernel BUG at mm/rmap.c:581!
Jan  7 04:24:35 firewall kernel: invalid opcode: 0000 [#1]
Jan  7 04:24:35 firewall kernel: SMP Jan  7 04:24:35 firewall kernel: last sysfs file: /class/net/lo/type
Jan  7 04:24:35 firewall kernel: Modules linked in: tun ipt_MASQUERADE iptable_nat ip_nat ipt_LOG xt_limit ipv6 ip_conntrack_netbios_ns ipt_REJECT xt_state i
p_conntrack nfnetlink xt_tcpudp iptable_filter ip_tables x_tables video sbs i2c_ec button battery asus_acpi ac parport_pc lp parport sr_mod sg pcspkr i2c_via
pro ide_cd i2c_core 3c59x via_rhine cdrom mii via_ircc irda crc_ccitt serio_raw dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd ehci_hcd ohci_hcd uhci_hcd
Jan  7 04:24:35 firewall kernel: CPU:    0
Jan  7 04:24:35 firewall kernel: EIP:    0060:[<c046d995>]    Not tainted VLI
Jan  7 04:24:35 firewall kernel: EFLAGS: 00010286   (2.6.19-1.2886.fc6debug #1)
Jan  7 04:24:35 firewall kernel: EIP is at page_remove_rmap+0x8b/0xac
Jan  7 04:24:35 firewall kernel: eax: 00000034   ebx: c1000000   ecx: c042818c   edx: da0b3550
Jan  7 04:24:35 firewall kernel: esi: daecd0c0   edi: 00000020   ebp: cd386140   esp: ce2f4dc4
Jan  7 04:24:35 firewall kernel: ds: 007b   es: 007b   ss: 0068
Jan  7 04:24:35 firewall kernel: Process zcat (pid: 22721, ti=ce2f4000 task=da0b3550 task.ti=ce2f4000)
Jan  7 04:24:35 firewall kernel: Stack: c066b7b6 00000000 c1000000 08050000 c0467c63 c06c7f10 00000046 00000000 Jan  7 04:24:35 firewall kernel:        daecd0c0 ce2f4e44 003f7ffd 00000000 00000001 08055000 cf354080 dde93520 Jan  7 04:24:35 firewall kernel:        c1401e80 00000000 fffffff9 dde9358c cf354080 08055000 00000000 ce2f4e44 Jan  7 04:24:35 firewall kernel: Call Trace:
Jan  7 04:24:35 firewall kernel:  [<c0467c63>] unmap_vmas+0x2a6/0x536
Jan  7 04:24:35 firewall kernel:  [<c046aaa8>] exit_mmap+0x77/0x105
Jan  7 04:24:35 firewall kernel:  [<c0425471>] mmput+0x37/0x80
Jan  7 04:24:35 firewall kernel:  [<c042a4b5>] do_exit+0x21c/0x7a3
Jan  7 04:24:35 firewall kernel:  [<c042aac9>] sys_exit_group+0x0/0xd
Jan  7 04:24:35 firewall kernel:  [<c043383d>] get_signal_to_deliver+0x38b/0x3b3
Jan  7 04:24:35 firewall kernel:  [<c0403677>] do_notify_resume+0x83/0x6c6
Jan  7 04:24:35 firewall kernel:  [<c0404159>] work_notifysig+0x13/0x1a
Jan  7 04:24:35 firewall kernel:  [<0804a84e>] 0x804a84e
Jan  7 04:24:35 firewall kernel:  =======================
Jan  7 04:24:35 firewall kernel: Code: 4c a4 fb ff 8b 43 10 c7 04 24 b6 b7 66 c0 89 44 24 04 e8 39 a4 fb ff 8b 46 40 85 c0 74 0d 8b 50 08 b8 cf b7 66 c0 e8 d

8 a9 fd ff <0f> 0b 45 02 4b b7 66 c0 8b 53 10 89 d8 59 5b 5b 5e 83 f2 01 83

Signature · 2 lines

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com

James Simmons

2006-12-05 17:35 UTC #26 lore raw

In reply to James Simmons

Quoted 2 lines

video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch

Does this patch update the fbdev drivers?

Quoted 9 lines

add-display-output-class-support.patch
add-output-class-document.patch
drivers-add-lcd-support-3.patch
drivers-add-lcd-support-3-Kconfig-fix.patch
drivers-add-lcd-support-update-4.patch
drivers-add-lcd-support-update-5.patch
drivers-add-lcd-support-update6.patch
drivers-add-lcd-support-update-7.patch
drivers-add-lcd-support-update-8.patch

Ug. We have alot of interfaces attempting to do the same thing. We also have the lcd class_dev in drivers/video/backlight. I did some work which I will post to interested parties in the hopes of getting one interface to make everyone happy.

Andrew Morton

author

2006-12-05 18:01 UTC #27 lore raw

Quoted 4 lines · On Tue, 5 Dec 2006 17:35:20 +0000 (GMT) James Simmons <[email protected]>…

Quoted 2 lines
video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Does this patch update the fbdev drivers?

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.19-rc6/2.6.19-rc6-mm2/broken-out/video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch

Seems not. Should it?

Quoted 10 lines

Quoted 9 lines
add-display-output-class-support.patch
add-output-class-document.patch
drivers-add-lcd-support-3.patch
drivers-add-lcd-support-3-Kconfig-fix.patch
drivers-add-lcd-support-update-4.patch
drivers-add-lcd-support-update-5.patch
drivers-add-lcd-support-update6.patch
drivers-add-lcd-support-update-7.patch
drivers-add-lcd-support-update-8.patch

Quoted 4 lines

Ug. We have alot of interfaces attempting to do the same thing. We also have the lcd class_dev in drivers/video/backlight. I did some work which I will post to interested parties in the hopes of getting one interface to make everyone happy.

Well can you please work out what we should do with Miguel?

James Simmons

2006-12-05 18:26 UTC #28 lore raw

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Does this patch update the fbdev drivers?
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.19-rc6/2.6.19-rc6-mm2/broken-out/video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Seems not. Should it?

Yes. Its bizarre. The drivers compile with the wrong method prototype. I updated the fbdev drivers to the new backlight_device_register and it compiled as expect. There are a few other problems with teh fbdev drivers. I will send a patch.

Quoted 10 lines

Quoted 10 lines
Quoted 9 lines
add-display-output-class-support.patch
add-output-class-document.patch
drivers-add-lcd-support-3.patch
drivers-add-lcd-support-3-Kconfig-fix.patch
drivers-add-lcd-support-update-4.patch
drivers-add-lcd-support-update-5.patch
drivers-add-lcd-support-update6.patch
drivers-add-lcd-support-update-7.patch
drivers-add-lcd-support-update-8.patch

Quoted 6 lines

Quoted 4 lines
Ug. We have alot of interfaces attempting to do the same thing. We also have the lcd class_dev in drivers/video/backlight. I did some work which I will post to interested parties in the hopes of getting one interface to make everyone happy.
Well can you please work out what we should do with Miguel?

I sent a email already. The details will be hammered out.

James Simmons

2006-12-05 18:37 UTC #29 lore raw

PATCH 1/1 backlight sysfs change to the fbdev drivers

This patch updates the framebuffer drivers that use the backlight api to the change of passing in the struct device to backlight_device_register in the -mm tree.

Please apply.

Signed-off: James Simmons <[email protected]>

drivers/video/aty/aty128fb.c +1 -1

index 276a215..00254d5 100644
@@ -1829,7 +1829,7 @@ static void aty128_bl_init(struct aty128
 
 	snprintf(name, sizeof(name), "aty128bl%d", info->node);
 
-	bd = backlight_device_register(name, par, &aty128_bl_data);
+	bd = backlight_device_register(name, par->dev, par, &aty128_bl_data);
 	if (IS_ERR(bd)) {
 		info->bl_dev = NULL;
 		printk(KERN_WARNING "aty128: Backlight registration failed\n");

drivers/video/aty/atyfb_base.c +1 -1

index e815b35..4916a7b 100644
@@ -2221,7 +2221,7 @@ static void aty_bl_init(struct atyfb_par
 
 	snprintf(name, sizeof(name), "atybl%d", info->node);
 
-	bd = backlight_device_register(name, par, &aty_bl_data);
+	bd = backlight_device_register(name, par->dev, par, &aty_bl_data);
 	if (IS_ERR(bd)) {
 		info->bl_dev = NULL;
 		printk(KERN_WARNING "aty: Backlight registration failed\n");

drivers/video/aty/radeon_backlight.c +1 -1

index 585eb7b..0faa720 100644
@@ -163,7 +163,7 @@ void radeonfb_bl_init(struct radeonfb_in
 
 	snprintf(name, sizeof(name), "radeonbl%d", rinfo->info->node);
 
-	bd = backlight_device_register(name, pdata, &radeon_bl_data);
+	bd = backlight_device_register(name, rinfo->pdev, pdata, &radeon_bl_data);
 	if (IS_ERR(bd)) {
 		rinfo->info->bl_dev = NULL;
 		printk("radeonfb: Backlight registration failed\n");

drivers/video/nvidia/nv_backlight.c +1 -1

index 5b75ae4..b0d5d85 100644
@@ -141,7 +141,7 @@ void nvidia_bl_init(struct nvidia_par *p
 
 	snprintf(name, sizeof(name), "nvidiabl%d", info->node);
 
-	bd = backlight_device_register(name, par, &nvidia_bl_data);
+	bd = backlight_device_register(name, par->pci_dev, par, &nvidia_bl_data);
 	if (IS_ERR(bd)) {
 		info->bl_dev = NULL;
 		printk(KERN_WARNING "nvidia: Backlight registration failed\n");

drivers/video/riva/fbdev.c +1 -1

index a433cc7..d7dfdb6 100644
@@ -383,7 +383,7 @@ static void riva_bl_init(struct riva_par
 
 	snprintf(name, sizeof(name), "rivabl%d", info->node);
 
-	bd = backlight_device_register(name, par, &riva_bl_data);
+	bd = backlight_device_register(name, par->pdev, par, &riva_bl_data);
 	if (IS_ERR(bd)) {
 		info->bl_dev = NULL;
 		printk(KERN_WARNING "riva: Backlight registration failed\n");

Andrew Morton

author

2006-12-05 19:43 UTC #30 lore raw

Quoted 9 lines · On Tue, 5 Dec 2006 18:25:58 +0000 (GMT) James Simmons <[email protected]>…

Quoted 7 lines
Quoted 3 lines
Quoted 1 line
video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Does this patch update the fbdev drivers?
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.19-rc6/2.6.19-rc6-mm2/broken-out/video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Seems not. Should it?
Yes. Its bizarre.

It is.

Quoted 1 line

The drivers compile with the wrong method prototype.

No, it doesn't get compiled at all.

CONFIG_FB_ATY128_BACKLIGHT has no Kconfig record at all.

CONFIG_FB_NVIDIA_BACKLIGHT has no Kconfig record at all.

CONFIG_FB_RIVA_BACKLIGHT has no Kconfig record at all.

So this is all dead code. There is no way to enable any of it within the config system.

Quoted 4 lines

I
updated the fbdev drivers to the new backlight_device_register and it compiled as expect. There are a few other problems with teh fbdev drivers. I will send a patch.

It's a start ;)

James Simmons

2006-12-05 19:59 UTC #31 lore raw

Quoted 17 lines

Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Does this patch update the fbdev drivers?
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.19-rc6/2.6.19-rc6-mm2/broken-out/video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Seems not. Should it?
Yes. Its bizarre.
It is.
Quoted 1 line
The drivers compile with the wrong method prototype.
No, it doesn't get compiled at all.

Quoted 8 lines

CONFIG_FB_ATY128_BACKLIGHT has no Kconfig record at all.
CONFIG_FB_NVIDIA_BACKLIGHT has no Kconfig record at all.
CONFIG_FB_RIVA_BACKLIGHT has no Kconfig record at all.
So this is all dead code. There is no way to enable any of it within the config system.

Ug. Kconfig in drivers/video has all the above drivers enable the backlight only if PMAC_BACKLIGHT is set. The problem is backlight is after the framebuffer. We should move the backlight menu before the framebuffer configuration.

Andrew Morton

author

2006-12-05 20:21 UTC #32 lore raw

Quoted 21 lines · On Tue, 5 Dec 2006 19:59:01 +0000 (GMT) James Simmons <[email protected]>…

Quoted 21 lines
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Does this patch update the fbdev drivers?
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.19-rc6/2.6.19-rc6-mm2/broken-out/video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Seems not. Should it?
Yes. Its bizarre.
It is.
Quoted 1 line
The drivers compile with the wrong method prototype.
No, it doesn't get compiled at all.
CONFIG_FB_ATY128_BACKLIGHT has no Kconfig record at all.
CONFIG_FB_NVIDIA_BACKLIGHT has no Kconfig record at all.

Quoted 7 lines

Quoted 4 lines
CONFIG_FB_RIVA_BACKLIGHT has no Kconfig record at all.
So this is all dead code. There is no way to enable any of it within the config system.
Ug. Kconfig in drivers/video has all the above drivers enable the backlight only if PMAC_BACKLIGHT is set.

erp, I grepped for CONFIG_foo and not foo. Again.

Quoted 3 lines

The problem is backlight
is after the framebuffer. We should move the backlight menu before the framebuffer configuration.

OK. Can you do a patch sometime please?

James Simmons

2006-12-05 21:34 UTC #33 lore raw

Quoted 11 lines

Quoted 3 lines
Ug. Kconfig in drivers/video has all the above drivers enable the backlight only if PMAC_BACKLIGHT is set.
erp, I grepped for CONFIG_foo and not foo. Again.
Quoted 3 lines
The problem is backlight
is after the framebuffer. We should move the backlight menu before the framebuffer configuration.
OK. Can you do a patch sometime please?

Here you go. This patch places the backlight before the framebuffers. You will now be able to select using the backlight with various framebuffer drivers.

Signed-off: James Simmons <[email protected]>

drivers/video/Kconfig +28 ~~-35~~

index 7a43020..40f6bea 100644
@@ -4,20 +4,9 @@
 
 menu "Graphics support"
 
-config FIRMWARE_EDID
-       bool "Enable firmware EDID"
-       default y
-       ---help---
-         This enables access to the EDID transferred from the firmware.
-	 On the i386, this is from the Video BIOS. Enable this if DDC/I2C
-	 transfers do not work for your driver and if you are using
-	 nvidiafb, i810fb or savagefb.
-
-	 In general, choosing Y for this option is safe.  If you
-	 experience extremely long delays while booting before you get
-	 something on your display, try setting this to N.  Matrox cards in
-	 combination with certain motherboards and monitors are known to
-	 suffer from this problem.
+if SYSFS
+	source "drivers/video/backlight/Kconfig"
+endif
 
 config FB
 	tristate "Support for frame buffer devices"

@@ -53,6 +42,22 @@ config FB
 	  (e.g. an accelerated X server) and that are not frame buffer
 	  device-aware may cause unexpected results. If unsure, say N.
 
+config FIRMWARE_EDID
+       bool "Enable firmware EDID"
+       depends on FB
+       default n 
+       ---help---
+         This enables access to the EDID transferred from the firmware.
+	 On the i386, this is from the Video BIOS. Enable this if DDC/I2C
+	 transfers do not work for your driver and if you are using
+	 nvidiafb, i810fb or savagefb.
+
+	 In general, choosing Y for this option is safe.  If you
+	 experience extremely long delays while booting before you get
+	 something on your display, try setting this to N.  Matrox cards in
+	 combination with certain motherboards and monitors are known to
+	 suffer from this problem.
+
 config FB_DDC
        tristate
        depends on FB && I2C && I2C_ALGOBIT

@@ -90,13 +95,6 @@ config FB_MACMODES
        depends on FB
        default n
 
-config FB_BACKLIGHT
-	bool
-	depends on FB
-	select BACKLIGHT_LCD_SUPPORT
-	select BACKLIGHT_CLASS_DEVICE
-	default n
-
 config FB_MODE_HELPERS
         bool "Enable Video Mode Handling Helpers"
         depends on FB

@@ -126,6 +124,9 @@ config FB_TILEBLITTING
 	 This is particularly important to one driver, matroxfb.  If
 	 unsure, say N.
 
+comment "Frambuffer hardware drivers"
+	depends on FB
+
 config FB_CIRRUS
 	tristate "Cirrus Logic support"
 	depends on FB && (ZORRO || PCI)

@@ -728,8 +729,7 @@ config FB_NVIDIA_I2C
 
 config FB_NVIDIA_BACKLIGHT
 	bool "Support for backlight control"
-	depends on FB_NVIDIA && PMAC_BACKLIGHT
-	select FB_BACKLIGHT
+	depends on FB_NVIDIA && BACKLIGHT_CLASS_DEVICE
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.

@@ -775,8 +775,7 @@ config FB_RIVA_DEBUG
 
 config FB_RIVA_BACKLIGHT
 	bool "Support for backlight control"
-	depends on FB_RIVA && PMAC_BACKLIGHT
-	select FB_BACKLIGHT
+	depends on FB_RIVA && BACKLIGHT_CLASS_DEVICE
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.

@@ -1049,8 +1048,7 @@ config FB_RADEON_I2C
 
 config FB_RADEON_BACKLIGHT
 	bool "Support for backlight control"
-	depends on FB_RADEON && PMAC_BACKLIGHT
-	select FB_BACKLIGHT
+	depends on FB_RADEON && BACKLIGHT_CLASS_DEVICE 
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.

@@ -1081,8 +1079,7 @@ config FB_ATY128
 
 config FB_ATY128_BACKLIGHT
 	bool "Support for backlight control"
-	depends on FB_ATY128 && PMAC_BACKLIGHT
-	select FB_BACKLIGHT
+	depends on FB_ATY128 && BACKLIGHT_CLASS_DEVICE 
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.

@@ -1131,8 +1128,7 @@ config FB_ATY_GX
 
 config FB_ATY_BACKLIGHT
 	bool "Support for backlight control"
-	depends on FB_ATY && PMAC_BACKLIGHT
-	select FB_BACKLIGHT
+	depends on FB_ATY && BACKLIGHT_CLASS_DEVICE 
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.

@@ -1632,6 +1628,7 @@ config FB_VIRTUAL
 	  the vfb_enable=1 option.
 
 	  If unsure, say N.
+
 if VT
 	source "drivers/video/console/Kconfig"
 endif

@@ -1640,9 +1637,5 @@ if FB || SGI_NEWPORT_CONSOLE
 	source "drivers/video/logo/Kconfig"
 endif
 
-if SYSFS
-	source "drivers/video/backlight/Kconfig"
-endif
-
 endmenu

Andrew Morton

author

2006-12-06 23:40 UTC #34 lore raw

Quoted 3 lines · On Tue, 5 Dec 2006 21:34:16 +0000 (GMT) James Simmons <[email protected]>…

Here you go. This patch places the backlight before the framebuffers. You will now be able to select using the backlight with various framebuffer drivers.

This doesn't work right for me. x86_64 allmodconfig enables CONFIG_FB_ATY_BACKLIGHT but fails to enable FB_BACKLIGHT so things won't build.

drivers/video/aty/atyfb_base.c: In function 'aty_bl_get_level_brightness':
drivers/video/aty/atyfb_base.c:2130: error: 'struct fb_info' has no member named 'bl_curve'

I'm not sure how to fix it, either. Are things like CONFIG_FB_ATY_BACKLIGHT _really_ supposed to be pmac-only?

menuconfig says:

Symbol: FB_BACKLIGHT [=FB_BACKLIGHT]

	Selected by: PMAC_BACKLIGHT && (PPC || MAC) && ADB_PMU && FB=y && (BROKEN || !PPC64)

Surely CONFIG_FB_ATY_BACKLIGHT should depend upon FB_BACKLIGHT?

How come I have CONFIG_BACKLIGHT_CLASS_DEVICE and things like that but CONFIG_FB_BACKLIGHT=n?

And a `make oldconfig' says

drivers/macintosh/Kconfig:126:warning: 'select' used by config symbol 'PMAC_BACKLIGHT' refer to undefined symbol 'FB_BACKLIGHT'

Here's what allmodconfig gave:

akpm2:/usr/src/25> grep BACKL .config
CONFIG_BACKLIGHT_LCD_SUPPORT=y
CONFIG_BACKLIGHT_CLASS_DEVICE=m
CONFIG_BACKLIGHT_DEVICE=y
CONFIG_FB_NVIDIA_BACKLIGHT=y
CONFIG_FB_RIVA_BACKLIGHT=y
CONFIG_FB_RADEON_BACKLIGHT=y
CONFIG_FB_ATY128_BACKLIGHT=y
CONFIG_FB_ATY_BACKLIGHT=y

Anyway, it seems all screwed up - I'll drop the patch.

James Simmons

2006-12-07 14:31 UTC #35 lore raw

Quoted 2 lines

Anyway, it seems all screwed up - I'll drop the patch.

I'm working on a new patch. The build system has changed quite a bit from the last time I worked with it 2 years ago.

Miguel Ojeda

2006-12-05 20:40 UTC #36 lore raw

In reply to James Simmons

Quoted 24 lines · James Simmons

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Does this patch update the fbdev drivers?
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.19-rc6/2.6.19-rc6-mm2/broken-out/video-sysfs-support-take-2-add-dev-argument-for-backlight_device_register.patch
Seems not. Should it?
Yes. Its bizarre. The drivers compile with the wrong method prototype. I updated the fbdev drivers to the new backlight_device_register and it compiled as expect. There are a few other problems with teh fbdev drivers. I will send a patch.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
add-display-output-class-support.patch
add-output-class-document.patch
drivers-add-lcd-support-3.patch
drivers-add-lcd-support-3-Kconfig-fix.patch
drivers-add-lcd-support-update-4.patch
drivers-add-lcd-support-update-5.patch
drivers-add-lcd-support-update6.patch
drivers-add-lcd-support-update-7.patch
drivers-add-lcd-support-update-8.patch

Quoted 10 lines

Quoted 6 lines
Quoted 4 lines
Ug. We have alot of interfaces attempting to do the same thing. We also have the lcd class_dev in drivers/video/backlight. I did some work which I will post to interested parties in the hopes of getting one interface to make everyone happy.
Well can you please work out what we should do with Miguel?
I sent a email already. The details will be hammered out.

Right back from exams and with lots of spare time :)

I got your patch, so I'm going to try your patch and exchange all the "platform_device_*" stuff with your display class and move ks0108, cfag12864b and cfag12864bfb to drivers/video/display/

Signature · 2 lines

Miguel Ojeda
http://maxextreme.googlepages.com/index.htm

James Simmons

2006-12-06 14:42 UTC #37 lore raw

Quoted 7 lines

Right back from exams and with lots of spare time :)
I got your patch, so I'm going to try your patch and exchange all the "platform_device_*" stuff with your display class and move ks0108, cfag12864b and cfag12864bfb to drivers/video/display/

Thanks. I will post a new patch soon.

Josef Sipek

2006-12-05 19:18 UTC #38 lore raw

On Mon, Dec 04, 2006 at 08:40:24PM -0800, Andrew Morton wrote:
...

Quoted 11 lines

fsstack-introduce-fsstack_copy_attrinode_.patch fsstack-introduce-fsstack_copy_attrinode_-tidy.patch fsstack-introduce-fsstack_copy_attrinode_-fs-stackc-should-include-linux-fs_stackh.patch ecryptfs-use-fsstacks-generic-copy-inode-attr.patch ecryptfs-use-fsstacks-generic-copy-inode-attr-tidy-fix.patch ecryptfs-use-fsstacks-generic-copy-inode-attr-tidy-fix-fix.patch struct-path-rename-reiserfss-struct-path.patch struct-path-rename-dms-struct-path.patch struct-path-move-struct-path-from-fs-nameic-into.patch struct-path-make-ecryptfs-a-user-of-struct-path.patch vfs-change-struct-file-to-use-struct-path.patch

...

Quoted 2 lines

Shall merge. I guess. Doesn't seem very useful.

I have two more fixes for fsstack/ecryptfs. I'll send them as a reply to this email...

Josef "Jeff" Sipek.

Signature · 3 lines

FORTUNE PROVIDES QUESTIONS FOR THE GREAT ANSWERS: #19
A:      To be or not to be.
Q:      What is the square root of 4b^2?

Josef Sipek

2006-12-05 19:21 UTC #39 lore raw

PATCH 1/2 fsstack: Make fsstack_copy_attr_all copy inode size

fsstack_copy_attr_all should copy the inode size in addition to all the other attributes.

Signed-off-by Josef "Jeff" Sipek <[email protected]>

---
 fs/stack.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

fs/stack.c +2 -0

index 8ffb880..5ddbc34 100644
@@ -34,5 +34,7 @@ void fsstack_copy_attr_all(struct inode
 	dest->i_ctime = src->i_ctime;
 	dest->i_blkbits = src->i_blkbits;
 	dest->i_flags = src->i_flags;
+
+	fsstack_copy_inode_size(dest, src);
 }
 EXPORT_SYMBOL_GPL(fsstack_copy_attr_all);

Signature · 1 line

1.4.3.3

Josef Sipek

2006-12-05 19:22 UTC #40 lore raw

PATCH 2/2 fsstack: Fix up ecryptfs's fsstack usage

In reply to Josef Sipek

Fix up a stray ecryptfs_copy_attr_all call and remove prototypes for ecryptfs_copy_* as they no longer exist.

Signed-off-by Josef "Jeff" Sipek <[email protected]>

---
 fs/ecryptfs/dentry.c          |    2 +-
 fs/ecryptfs/ecryptfs_kernel.h |    4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

fs/ecryptfs/dentry.c +1 -1

index 52d1e36..b0352d8 100644
@@ -61,7 +61,7 @@ static int ecryptfs_d_revalidate(struct
 		struct inode *lower_inode =
 			ecryptfs_inode_to_lower(dentry->d_inode);
 
-		ecryptfs_copy_attr_all(dentry->d_inode, lower_inode);
+		fsstack_copy_attr_all(dentry->d_inode, lower_inode, NULL);
 	}
 out:
 	return rc;

fs/ecryptfs/ecryptfs_kernel.h +1 -3

index 870a65b..afb64bd 100644
@@ -28,6 +28,7 @@
 
 #include <keys/user-type.h>
 #include <linux/fs.h>
+#include <linux/fs_stack.h>
 #include <linux/namei.h>
 #include <linux/scatterlist.h>

@@ -413,9 +414,6 @@ int ecryptfs_encode_filename(struct ecry
 			     const char *name, int length,
 			     char **encoded_name);
 struct dentry *ecryptfs_lower_dentry(struct dentry *this_dentry);
-void ecryptfs_copy_attr_atime(struct inode *dest, const struct inode *src);
-void ecryptfs_copy_attr_all(struct inode *dest, const struct inode *src);
-void ecryptfs_copy_inode_size(struct inode *dst, const struct inode *src);
 void ecryptfs_dump_hex(char *data, int bytes);
 int virt_to_scatterlist(const void *addr, int size, struct scatterlist *sg,
 			int sg_size);

Signature · 1 line

1.4.3.3

Andrew Morton

author

2006-12-05 22:28 UTC #41 lore raw

Re: [PATCH 2/2] fsstack: Fix up ecryptfs's fsstack usage

Quoted 14 lines · On Tue, 5 Dec 2006 14:22:32 -0500 Josef Sipek <[email protected]> wrote:

Fix up a stray ecryptfs_copy_attr_all call and remove prototypes for ecryptfs_copy_* as they no longer exist.
Signed-off-by Josef "Jeff" Sipek <[email protected]>
---
 fs/ecryptfs/dentry.c          |    2 +-
 fs/ecryptfs/ecryptfs_kernel.h |    4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)
fs/ecryptfs/dentry.c +0 -0
index 52d1e36..b0352d8 100644

fs/ecryptfs/dentry.c static int ecryptfs_d_revalidate(struct · line 64

Quoted 5 lines

fs/ecryptfs/dentry.c +1 -1

@@ -61,7 +61,7 @@ static int ecryptfs_d_revalidate(struct
 		struct inode *lower_inode =
 			ecryptfs_inode_to_lower(dentry->d_inode);
 
-		ecryptfs_copy_attr_all(dentry->d_inode, lower_inode);
+		fsstack_copy_attr_all(dentry->d_inode, lower_inode, NULL);

I fixed that two weeks ago.

When your patches are queued in -mm please do test them there, and review others' changes to them, and raise patches against them. Raising patches against one's private tree and not testing the code which is planned to be merged can introduce errors.

Josef Sipek

2006-12-05 22:38 UTC #42 lore raw

Re: [PATCH 2/2] fsstack: Fix up ecryptfs's fsstack usage

Quoted 25 lines · Andrew Morton

Quoted 14 lines · On Tue, 5 Dec 2006 14:22:32 -0500 Josef Sipek <[email protected]> wrote:
Fix up a stray ecryptfs_copy_attr_all call and remove prototypes for ecryptfs_copy_* as they no longer exist.
Signed-off-by Josef "Jeff" Sipek <[email protected]>
---
 fs/ecryptfs/dentry.c          |    2 +-
 fs/ecryptfs/ecryptfs_kernel.h |    4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)
fs/ecryptfs/dentry.c +0 -0
index 52d1e36..b0352d8 100644
fs/ecryptfs/dentry.c static int ecryptfs_d_revalidate(struct · line 64
Quoted 5 lines
fs/ecryptfs/dentry.c +1 -1
@@ -61,7 +61,7 @@ static int ecryptfs_d_revalidate(struct
 		struct inode *lower_inode =
 			ecryptfs_inode_to_lower(dentry->d_inode);
 
-		ecryptfs_copy_attr_all(dentry->d_inode, lower_inode);
+		fsstack_copy_attr_all(dentry->d_inode, lower_inode, NULL);
I fixed that two weeks ago.

Quoted 4 lines

When your patches are queued in -mm please do test them there, and review others' changes to them, and raise patches against them. Raising patches against one's private tree and not testing the code which is planned to be merged can introduce errors.

Sorry about that. I noticed your fix, and the one by Adrian. And I did add them to my fsstack queue.

I must have dropped it accidentally.

Josef "Jeff" Sipek.

Signature · 2 lines

If I have trouble installing Linux, something is wrong. Very wrong.
		- Linus Torvalds

Andrew Morton

author

2006-12-05 22:49 UTC #43 lore raw

Re: [PATCH 2/2] fsstack: Fix up ecryptfs's fsstack usage

Quoted 7 lines · On Tue, 5 Dec 2006 17:38:07 -0500 Josef Sipek <[email protected]> wrote:

Quoted 4 lines
When your patches are queued in -mm please do test them there, and review others' changes to them, and raise patches against them. Raising patches against one's private tree and not testing the code which is planned to be merged can introduce errors.
Sorry about that. I noticed your fix, and the one by Adrian. And I did add them to my fsstack queue.

you don't have an fsstack queue any more ;)

Please, I really do want developers to test their code in -mm once I've merged it. What happens if there's some nasty interaction between your patch and someone else's? We'll not find out about it and it'll get merged.

Josef Sipek

2006-12-05 23:16 UTC #44 lore raw

Re: [PATCH 2/2] fsstack: Fix up ecryptfs's fsstack usage

Quoted 8 lines · Andrew Morton

Quoted 5 lines · On Tue, 5 Dec 2006 17:38:07 -0500 Josef Sipek <[email protected]> wrote:
Quoted 4 lines
When your patches are queued in -mm please do test them there, and review others' changes to them, and raise patches against them. Raising patches against one's private tree and not testing the code which is planned to be merged can introduce errors.

Quoted 4 lines

Quoted 2 lines
Sorry about that. I noticed your fix, and the one by Adrian. And I did add them to my fsstack queue.
you don't have an fsstack queue any more ;)

Good point :)

Quoted 4 lines

Please, I really do want developers to test their code in -mm once I've merged it. What happens if there's some nasty interaction between your patch and someone else's? We'll not find out about it and it'll get merged.

Point taken.

Josef "Jeff" Sipek.

Signature · 1 line

I already backed up the box once, I can do it again.

Ingo Molnar

2006-12-05 21:01 UTC #45 lore raw

Quoted 3 lines · Andrew Morton

gtod-exponential-update_wall_time.patch
Roman wanted this dropped, and afaik that's unresolved.

please drop this one for now, we'll rework it. It's an optimization, not a showstopper must-have component.

Ingo

Ingo Molnar

Acked-by Acked-by Acked-by Acked-by Acked-by Acked-by Acked-by Acked-by Acked-by Acked-by Acked-by Acked-by Acked-by Acked-by

2006-12-05 21:18 UTC #46 lore raw

Re: -mm merge plans for 2.6.20, scheduler bits

Quoted 3 lines · Andrew Morton

schedc-correct-comment-for-this_rq_lock-routine.patch sched-fix-migration-cost-estimator.patch sched-domain-move-sched-group-allocations-to-percpu-area.patch

(already acked)

Quoted 1 line

move_task_off_dead_cpu-should-be-called-with-disabled-ints.patch

Quoted 1 line

sched-domain-increase-the-smt-busy-rebalance-interval.patch

Quoted 1 line

sched-avoid-taking-rq-lock-in-wake_priority_sleeper.patch

Quoted 1 line

sched-remove-staggering-of-load-balancing.patch

Quoted 1 line

sched-disable-interrupts-for-locking-in-load_balance.patch

Quoted 1 line

sched-extract-load-calculation-from-rebalance_tick.patch

Quoted 1 line

sched-move-idle-status-calculation-into-rebalance_tick.patch

Quoted 1 line

sched-use-softirq-for-load-balancing.patch

Quoted 1 line

sched-call-tasklet-less-frequently.patch

patch is misnamed, otherwise:

Quoted 2 lines

sched-add-option-to-serialize-load-balancing.patch sched-add-option-to-serialize-load-balancing-fix.patch

(please merge these two patches into one, the first one is not bisectable.)

Quoted 3 lines

sched-improve-migration-accuracy.patch
sched-improve-migration-accuracy-tidy.patch
sched-improve-migration-accuracy-fix.patch

please merge into one patch.

Quoted 1 line

sched-decrease-number-of-load-balances.patch

this one goes away as per Ken's observation.

Quoted 1 line

sched-optimize-activate_task-for-rt-task.patch

Quoted 1 line

kernel-schedc-whitespace-cleanups.patch

i dont like these:

- cost1 += measure_one(cache, size - i*1024, cpu1, cpu2);
+ cost1 += measure_one(cache, size - i * 1024, cpu1, cpu2);

as it's quite OK to have no spaces in "i*1024", just to indicate precedence of arithmetic ops. But the good bits dominate in this patch so lets have it and i'll undo the bad ones.

Quoted 1 line

kernel-schedc-whitespace-cleanups-more.patch

Ingo

Siddha, Suresh B

2006-12-05 21:25 UTC #47 lore raw

Re: -mm merge plans for 2.6.20, scheduler bits

Quoted 3 lines · Ingo Molnar

Quoted 1 line
sched-decrease-number-of-load-balances.patch
this one goes away as per Ken's observation.

Not really. Ken posted a cleanup patch on top of the above patch. Ken pointed one more cleanup which I will work on it and send it across in a day.

thanks,
suresh

Ingo Molnar

2006-12-05 21:47 UTC #48 lore raw

Re: -mm merge plans for 2.6.20, scheduler bits

Quoted 8 lines · Siddha, Suresh B

Quoted 3 lines · Ingo Molnar
Quoted 1 line
sched-decrease-number-of-load-balances.patch
this one goes away as per Ken's observation.
Not really. Ken posted a cleanup patch on top of the above patch. Ken pointed one more cleanup which I will work on it and send it across in a day.

ok.

Ingo

Miguel Ojeda

2006-12-05 21:29 UTC #49 lore raw

Re: -mm merge plans for 2.6.20, scheduler bits

In reply to Ingo Molnar

Quoted 10 lines · Ingo Molnar

Quoted 2 lines
kernel-schedc-whitespace-cleanups.patch
Acked-by Ingo Molnar <[email protected]>
i dont like these:
(quoted diff) +1 -1
-               cost1 += measure_one(cache, size - i*1024, cpu1, cpu2);
+               cost1 += measure_one(cache, size - i * 1024, cpu1, cpu2);

Quoted 4 lines

as it's quite OK to have no spaces in "i*1024", just to indicate precedence of arithmetic ops. But the good bits dominate in this patch so lets have it and i'll undo the bad ones.

Ok, I will take care of that for future whitespace cleanups, as I see they are welcome :)

Signature · 2 lines

Miguel Ojeda
http://maxextreme.googlepages.com/index.htm

Roman Zippel

2006-12-06 02:59 UTC #50 lore raw

Hi,

Quoted 4 lines · Andrew Morton

[dyntick]
Shall merge, I guess. My confidence is low, but it's Kconfigurable and it needs to get sorted out.

IMO it least at needs one more iteration to address the comments that were made (not just mine), in the short term the less it touches unconditionally the less I care right now.
In the long term IMO this might need a major rework, the basic problem I have is that I don't see how this usable beyond dynticks/hrtimer, e.g. how to dynamically manage multiple timer.

bye, Roman

Andrew Morton

author

2006-12-06 03:30 UTC #51 lore raw

Quoted 5 lines · On Wed, 6 Dec 2006 03:59:41 +0100 (CET) Roman Zippel <[email protected]>…

Hi,
On Mon, 4 Dec 2006, Andrew Morton wrote:

Quoted 8 lines

Quoted 4 lines
[dyntick]
Shall merge, I guess. My confidence is low, but it's Kconfigurable and it needs to get sorted out.
IMO it least at needs one more iteration to address the comments that were made (not just mine), in the short term the less it touches unconditionally the less I care right now.

I don't have a clue which review comments remain unaddressed - do you recall?

I never saw an item-by-item accounting of my own (extensive) review comments, actually. And then an avalanche of new stuff got sent and I didn't have time to go through it all at the same level of detail.

So yeah, I don't have a lot of confidence from that POV either. But otoh, I'm confident that Ingo and Thomas will competently and promptly address regressions, so the risk factor isn't too bad. And changing APIC and timekeeping code sure is risky.

Yes, I too am wobbly about a 2.6.20 merge. But otoh I doubt if much will get changed if it sits in -mm for another two months. As long as it's heading in the right direction and doesn't break things when it is configged-off then OK.

Quoted 3 lines

In the long term IMO this might need a major rework, the basic problem I have is that I don't see how this usable beyond dynticks/hrtimer, e.g. how to dynamically manage multiple timer.

I'm not sure I understand that. Are you referring to multiple, concurrently-operating hardware clock sources? <wonders how that could work> If so, that's more a clocksource thing than a dynticks/hrtimer thing, isn't it?

Thomas Gleixner

2006-12-06 08:29 UTC #52 lore raw

Quoted 5 lines · Andrew Morton

I don't have a clue which review comments remain unaddressed - do you recall?
I never saw an item-by-item accounting of my own (extensive) review comments, actually. And then an avalanche of new stuff got sent and I didn't have time to go through it all at the same level of detail.

Arjan did a review of the replacement and I'm in the process to go through that and Romans comments again. I'll add a complete list of the review issues and the resolution state. Sorry that this was delayed, but I was busy regression testing and hunting nasty details in the last weeks.

Quoted 4 lines

So yeah, I don't have a lot of confidence from that POV either. But otoh, I'm confident that Ingo and Thomas will competently and promptly address regressions, so the risk factor isn't too bad. And changing APIC and timekeeping code sure is risky.

Timekeeping is mostly untouched and the APIC change was done to solve real world issues:

- lapic verification gets stuck for ever
- lapic timer runs too fast

Quoted 8 lines

Quoted 3 lines
In the long term IMO this might need a major rework, the basic problem I have is that I don't see how this usable beyond dynticks/hrtimer, e.g. how to dynamically manage multiple timer.
I'm not sure I understand that. Are you referring to multiple, concurrently-operating hardware clock sources? <wonders how that could work> If so, that's more a clocksource thing than a dynticks/hrtimer thing, isn't it?

If I understand it correctly, Roman wants clockevents to be usable for other things aside hrtimer/dyntick, i.e. let other code request unused timer event hardware for special purposes. I thought about that in the originally but I stayed away from it, as there are no users at the moment and I wanted to avoid the usual "who needs that" comment. Granted that clockevents is not perfect, but it's rather self contained and has no user space exposure, so it can be replaced by something better anytime.

tglx

Roman Zippel

2006-12-06 12:54 UTC #53 lore raw

Hi,

Quoted 5 lines · Thomas Gleixner

If I understand it correctly, Roman wants clockevents to be usable for other things aside hrtimer/dyntick, i.e. let other code request unused timer event hardware for special purposes. I thought about that in the originally but I stayed away from it, as there are no users at the moment and I wanted to avoid the usual "who needs that" comment.

Nonsense, one obvious user would be the scheduler, the current scheduler tick emulation is rather ugly and sort of explains why you need the special wakeup logic I saw in previous versions.
The scheduler should be completely separate from hrtimer, in the long term they might optionally not even use the same clock source (e.g. the scheduler would use a low resolution, but fast clock, while posix timer whould use the high resolution timer).

bye, Roman

Ingo Molnar

2006-12-06 13:12 UTC #54 lore raw

Quoted 10 lines · Roman Zippel

Quoted 6 lines · Thomas Gleixner
If I understand it correctly, Roman wants clockevents to be usable for other things aside hrtimer/dyntick, i.e. let other code request unused timer event hardware for special purposes. I thought about that in the originally but I stayed away from it, as there are no users at the moment and I wanted to avoid the usual "who needs that" comment.
Nonsense, [...]

why do you call Thomas' observation nonsense? Fact: there is no current user of such a facility. What we implemented, high-res timers and dynticks does not need such a facility.

we wholeheartedly agree that such a facility 'would be nice', and 'could be used by existing kernel code' but we didnt want to chew too much at a time.

Quoted 1 line

[...] one obvious user would be the scheduler, [...]

But cleaning up the scheduler tick was not our purpose with high-res timers nor with dynticks. One of your previous complaints was that the patches are too intrusive to be trusted. Now they are too simple?

We'll clean up the scheduler tick and profiling too, but not now.

Quoted 1 line

[...] the current scheduler tick emulation is rather ugly [...]

i disagree with you and it's pretty low-impact anyway. There's still quite many HZ/tick assumptions all around the time code (NTP being one example), we'll deal with those via other patches.

Ingo

Roman Zippel

2006-12-06 14:33 UTC #55 lore raw

Hi,

Quoted 11 lines · Ingo Molnar

Quoted 9 lines · Roman Zippel
Quoted 6 lines · Thomas Gleixner
If I understand it correctly, Roman wants clockevents to be usable for other things aside hrtimer/dyntick, i.e. let other code request unused timer event hardware for special purposes. I thought about that in the originally but I stayed away from it, as there are no users at the moment and I wanted to avoid the usual "who needs that" comment.

Quoted 3 lines

Quoted 1 line
Nonsense, [...]
why do you call Thomas' observation nonsense?

What's the point of this question? Your answer is/was in the "[...]" part.

Quoted 3 lines

Fact: there is no current
user of such a facility. What we implemented, high-res timers and dynticks does not need such a facility.

Fact: there _are_ users which need a timer facility (e.g. the scheduler).

Quoted 3 lines

we wholeheartedly agree that such a facility 'would be nice', and 'could be used by existing kernel code' but we didnt want to chew too much at a time.

Maybe the existing code should have been cleaned up first? Unfortunately the dynticks feature seems to more appealing than clean code...

Quoted 5 lines

Quoted 1 line
[...] one obvious user would be the scheduler, [...]
But cleaning up the scheduler tick was not our purpose with high-res timers nor with dynticks. One of your previous complaints was that the patches are too intrusive to be trusted. Now they are too simple?

Would you please stop these personal attacks?

Quoted 1 line

We'll clean up the scheduler tick and profiling too, but not now.

Since this is very important code, it would be rather useful to know what's coming and to get to an agreement about the general direction this code is taking. This code has to cover a wide range of applications, where as you guys are rather focused on realtime applications, which is ok, but we need to _carefully_ rethink how time is managed within the kernel, instead of randomly poking around in the kernel.

Quoted 5 lines

Quoted 1 line
[...] the current scheduler tick emulation is rather ugly [...]
i disagree with you and it's pretty low-impact anyway. There's still quite many HZ/tick assumptions all around the time code (NTP being one example), we'll deal with those via other patches.

Why do you pick on the NTP code? That's actually one of the places where assumptions about HZ are largely gone. NTP state is updated incrementally and this won't change, but the update frequency can now be easily disconnected from HZ.

bye, Roman

Ingo Molnar

2006-12-06 15:23 UTC #56 lore raw

Quoted 7 lines · Roman Zippel

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
If I understand it correctly, Roman wants clockevents to be usable for other things aside hrtimer/dyntick, i.e. let other code request unused timer event hardware for special purposes. I thought about that in the originally but I stayed away from it, as there are no users at the moment and I wanted to avoid the usual "who needs that" comment.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Nonsense, [...]
why do you call Thomas' observation nonsense?
What's the point of this question? [...]

the point of my question was what it said: to ask you why you called Thomas' pretty fair observation 'nonsense'.

Quoted 1 line

[...] Your answer is/was in the "[...]" part.

meaning:

Quoted 1 line

Quoted 1 line
Quoted 1 line
[...] one obvious user would be the scheduler, [...]

but that is not a refutation of what Thomas said, at all. You say that the scheduler /could/ use such a facility. What Thomas said was that /there are no current users of such a facility/. It is a really simple (and unconditionally true) observation from Thomas. Yes, we could change other kernel code not directly related to high-res timers, but we chose not to.

[ The word 'nonsense' is in general a negative descriptor and implies
that what Thomas said was 'obviously false' - while in reality what we have is /at most/ a difference in opinion. I.e. even if our opinion differs you shouldnt call his opinion "nonsense", unless you are willing to prove that it's false - which you didnt do so far. I claim that what Thomas said is /unconditionally true/, and i can prove it:
it is unconditionally true that such a new facility is not needed by our code, and that no other kernel code is using it at the moment - because the facility does not exist yet. If you misunderstood what Thomas said then please point that out, or prove that his claims are untrue - thanks. ]

Quoted 6 lines

Quoted 3 lines
Fact: there is no current user of such a facility. What we implemented, high-res timers and dynticks does not need such a facility.
Fact: there _are_ users which need a timer facility (e.g. the scheduler).

this is something we are not contesting at all: that a new timer facility /could/ be used by /other/ kernel code, which does /not/ use it right now.

our point is simple: the code we add does not in itself necessiate the new facility, hence we didnt add it. We didnt want to impact other kernel code, just yet. We repeat: we agree (in theory) with such a facility, but we wanted to do it separately.

Quoted 5 lines

Quoted 3 lines
we wholeheartedly agree that such a facility 'would be nice', and 'could be used by existing kernel code' but we didnt want to chew too much at a time.
Maybe the existing code should have been cleaned up first?

yes, we'll do that, and we have a good track record doing such cleanups.

Quoted 2 lines

Unfortunately the dynticks feature seems to more appealing than clean code...

i think this accusation against us is very unfair.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
[...] one obvious user would be the scheduler, [...]
But cleaning up the scheduler tick was not our purpose with high-res timers nor with dynticks. One of your previous complaints was that the patches are too intrusive to be trusted. Now they are too simple?
Would you please stop these personal attacks?

please point it out which portion of the above two sentences you consider a personal attack - or if you cannot, please retract your baseless accusation.

Fact: you did complain in the past about the complexity and/or intrusiveness of our high-res timers patches - we've been working on getting high-res timers upstream for more than a year now.

Fact: you did complain about unused facilities in past patches of ours, and your feedback caused us significant extra work to remove those facilities, just to reintroduce them later again.

(i can provide URLs and Message-IDs for these facts)

this time around we chose the minimalistic approach, we didnt add anything that is not immediately needed, and we didnt want to increase intrusiveness by "cleaning up" other code and changing it over to the new facilities.

It cannot be had both ways: either we stay simpler and less intrusive, or we go more generic but more intrusive.

Ingo

Roman Zippel

2006-12-06 16:43 UTC #57 lore raw

Hi,

Quoted 7 lines · Ingo Molnar

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
If I understand it correctly, Roman wants clockevents to be usable for other things aside hrtimer/dyntick, i.e. let other code request unused timer event hardware for special purposes. I thought about that in the originally but I stayed away from it, as there are no users at the moment and I wanted to avoid the usual "who needs that" comment.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Nonsense, [...]
why do you call Thomas' observation nonsense?
What's the point of this question? [...]
the point of my question was what it said: to ask you why you called Thomas' pretty fair observation 'nonsense'.

The nonsense comment was regarding the 'the usual "who needs that" comment', how is it possible that Thomas can observe something that hasn't happened yet?

Quoted 4 lines

Quoted 1 line
[...] Your answer is/was in the "[...]" part.
meaning:

Quoted 8 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
[...] one obvious user would be the scheduler, [...]
but that is not a refutation of what Thomas said, at all. You say that the scheduler /could/ use such a facility. What Thomas said was that /there are no current users of such a facility/. It is a really simple (and unconditionally true) observation from Thomas. Yes, we could change other kernel code not directly related to high-res timers, but we chose not to.

I didn't say "/could/", the scheduler _needs_ such a facility. If hrtimer and scheduler have to use the same facility, they have to be coordinated somehow. The current patches don't clean up anything here and actually introduce extra complexity via the scheduler tick emulation, with little to no indication how this will be cleaned up. Others may have more faith, that it will somehow work out, but I'd like to get a bit more information beforehand.

Quoted 7 lines

Quoted 6 lines
Quoted 3 lines
Fact: there is no current user of such a facility. What we implemented, high-res timers and dynticks does not need such a facility.
Fact: there _are_ users which need a timer facility (e.g. the scheduler).

Quoted 8 lines

this is something we are not contesting at all: that a new timer facility /could/ be used by /other/ kernel code, which does /not/ use it right now.
our point is simple: the code we add does not in itself necessiate the new facility, hence we didnt add it. We didnt want to impact other kernel code, just yet. We repeat: we agree (in theory) with such a facility, but we wanted to do it separately.

You don't really refute that this eventually will impact other code. The problem here is that there is pretty much no plan how this will happen. Without this information most developers cannot tell (without diving deep into these issues), how this will impact existing code (e.g. all the arch timer code) or how much more cruft this is going to add, which may have to be cleaned up later again. Maybe this code is perfect, but how is anyone going to tell?

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
we wholeheartedly agree that such a facility 'would be nice', and 'could be used by existing kernel code' but we didnt want to chew too much at a time.
Maybe the existing code should have been cleaned up first?
yes, we'll do that, and we have a good track record doing such cleanups.

You didn't answer my question, how are you going to do this _first_?

Quoted 11 lines

Quoted 7 lines
Quoted 5 lines
Quoted 1 line
[...] one obvious user would be the scheduler, [...]
But cleaning up the scheduler tick was not our purpose with high-res timers nor with dynticks. One of your previous complaints was that the patches are too intrusive to be trusted. Now they are too simple?
Would you please stop these personal attacks?
please point it out which portion of the above two sentences you consider a personal attack - or if you cannot, please retract your baseless accusation.

Ingo, above it's you who makes a baseless accusation in the first place. Your accusation is so generic, that I have no other way than to take this personally.

Quoted 7 lines

Fact: you did complain in the past about the complexity and/or intrusiveness of our high-res timers patches - we've been working on getting high-res timers upstream for more than a year now.
Fact: you did complain about unused facilities in past patches of ours, and your feedback caused us significant extra work to remove those facilities, just to reintroduce them later again.

As long as you see my comments only as "complaining", I'm not really surprised we don't get nowhere...

Quoted 4 lines

this time around we chose the minimalistic approach, we didnt add anything that is not immediately needed, and we didnt want to increase intrusiveness by "cleaning up" other code and changing it over to the new facilities.

At the risk of repeating myself:

Quoted 2 lines

It cannot be had both ways: either we stay simpler and less intrusive, or we go more generic but more intrusive.

You miss the point, I don't care how intrusive this is, but with every step we make we have to keep the big picture in mind and the latter is seriously lacking here.
I have my doubts about where this is going and all I'm getting from you now (again) is that you try to turn this around and put all the blame on me. Sorry, Ingo, I'm not taking it and if you continue like this it will end like this every single time.

bye, Roman

Ingo Molnar

2006-12-06 16:59 UTC #58 lore raw

Quoted 10 lines · Roman Zippel

Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
[...] one obvious user would be the scheduler, [...]
but that is not a refutation of what Thomas said, at all. You say that the scheduler /could/ use such a facility. What Thomas said was that /there are no current users of such a facility/. It is a really simple (and unconditionally true) observation from Thomas. Yes, we could change other kernel code not directly related to high-res timers, but we chose not to.
I didn't say "/could/", the scheduler _needs_ such a facility. [...]

i disagree that the scheduler unconditionally 'needs' such a facility. The scheduler clock is pretty special and has other requirements and constraints than generic timekeeping clocks. It /might/ and /could/ utilize such an infrastructure, but it's not at all clear that it 'needs' such a facility.

in any case, i very much entertain the possibility of more synergy in this space, but it's far from obvious and it's definitely not unconditionally 'necessary'. The scheduler and profiling code certainly worked for 15 years without such synergy. What we concentrated on in this patchset is high-resolution timers and dynticks, not scheduler or profiler clock cleanups. We cleaned up everything that we impacted directly, but we also tried to limit the scope of the changes wherever possible.

Ingo

Ingo Molnar

2006-12-06 17:01 UTC #59 lore raw

In reply to Roman Zippel

Quoted 4 lines · Roman Zippel

Quoted 2 lines
It cannot be had both ways: either we stay simpler and less intrusive, or we go more generic but more intrusive.
You miss the point, I don't care how intrusive this is, [...]

we do care about how intrusive this is. I guess we'll have to agree to disagree on that.

Ingo

john stultz

2006-12-12 20:46 UTC #60 lore raw

RFC 1/1 HZ free ntp

In reply to Roman Zippel

Quoted 9 lines · Roman Zippel

Quoted 3 lines · Ingo Molnar
i disagree with you and it's pretty low-impact anyway. There's still quite many HZ/tick assumptions all around the time code (NTP being one example), we'll deal with those via other patches.
Why do you pick on the NTP code? That's actually one of the places where assumptions about HZ are largely gone. NTP state is updated incrementally and this won't change, but the update frequency can now be easily disconnected from HZ.

Hey Roman,
Here's my rough first attempt at doing so. I'd not call it easy, but
maybe you have some suggestions for a simpler way?

Basically INTERVAL_LENGTH_NSEC defines the NTP interval length that the time code will use to accumulate with. In this patch I've pushed it out to a full second, but it could be set via config (NSEC_PER_SEC/HZ for regular systems, something larger for systems using dynticks).

Thoughts?

-john

include/linux/timex.h +3 -2

index db501dc..be13daf 100644
@@ -286,9 +286,10 @@ #endif /* !CONFIG_TIME_INTERPOLATION */
 
 #define TICK_LENGTH_SHIFT	32
 
-/* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */
-extern u64 current_tick_length(void);
+#define INTERVAL_LENGTH_NSEC (NSEC_PER_SEC)
 
+/* Returns how long NTP intervals are at present, in ns / 2^(SHIFT_SCALE-10).*/
+extern u64 ntp_interval_length(void);
 extern void second_overflow(void);
 extern void update_ntp_one_tick(void);
 extern int do_adjtimex(struct timex *);

kernel/hrtimer.c +3 -1

index d0ba190..53979a9 100644
@@ -127,12 +127,14 @@ EXPORT_SYMBOL_GPL(ktime_get_ts);
  */
 static void hrtimer_get_softirq_time(struct hrtimer_base *base)
 {
+	struct timespec ts;
 	ktime_t xtim, tomono;
 	unsigned long seq;
 
 	do {
 		seq = read_seqbegin(&xtime_lock);
-		xtim = timespec_to_ktime(xtime);
+		getnstimeofday(&ts);
+		xtim = timespec_to_ktime(ts);
 		tomono = timespec_to_ktime(wall_to_monotonic);
 
 	} while (read_seqretry(&xtime_lock, seq));

kernel/time/ntp.c +54 ~~-21~~

124 lines not on this page · raw message

kernel/timer.c +4 -4

index 0256ab4..3a9f2b4 100644
@@ -890,7 +890,7 @@ void __init timekeeping_init(void)
 	ntp_clear();
 
 	clock = clocksource_get_next();
-	clocksource_calculate_interval(clock, tick_nsec);
+	clocksource_calculate_interval(clock, INTERVAL_LENGTH_NSEC);
 	clock->cycle_last = clocksource_read(clock);
 
 	write_sequnlock_irqrestore(&xtime_lock, flags);

@@ -980,7 +980,7 @@ static __always_inline int clocksource_b
 	 * Now calculate the error in (1 << look_ahead) ticks, but first
 	 * remove the single look ahead already included in the error.
 	 */
-	tick_error = current_tick_length() >>
+	tick_error = ntp_interval_length() >>
 		(TICK_LENGTH_SHIFT - clock->shift + 1);
 	tick_error -= clock->xtime_interval >> 1;
 	error = ((error - tick_error) >> look_ahead) + tick_error;

@@ -1077,7 +1077,7 @@ #endif
 						>> clock->shift);
 
 		/* accumulate error between NTP and clock interval */
-		clock->error += current_tick_length();
+		clock->error += ntp_interval_length();
 		clock->error -= clock->xtime_interval << (TICK_LENGTH_SHIFT - clock->shift);
 	}

@@ -1092,7 +1092,7 @@ #endif
 	if (change_clocksource()) {
 		clock->error = 0;
 		clock->xtime_nsec = 0;
-		clocksource_calculate_interval(clock, tick_nsec);
+		clocksource_calculate_interval(clock, INTERVAL_LENGTH_NSEC);
 	}
 }

Ingo Molnar

2006-12-13 09:54 UTC #61 lore raw

Re: [RFC] HZ free ntp

Quoted 15 lines · john stultz

Quoted 9 lines · Roman Zippel
Quoted 3 lines · Ingo Molnar
i disagree with you and it's pretty low-impact anyway. There's still quite many HZ/tick assumptions all around the time code (NTP being one example), we'll deal with those via other patches.
Why do you pick on the NTP code? That's actually one of the places where assumptions about HZ are largely gone. NTP state is updated incrementally and this won't change, but the update frequency can now be easily disconnected from HZ.
Hey Roman,
Here's my rough first attempt at doing so. I'd not call it easy, but
maybe you have some suggestions for a simpler way?

Quoted 5 lines

Basically INTERVAL_LENGTH_NSEC defines the NTP interval length that the time code will use to accumulate with. In this patch I've pushed it out to a full second, but it could be set via config (NSEC_PER_SEC/HZ for regular systems, something larger for systems using dynticks).

cool! I'll give this one a go in -rt, combined with the exponential second-overflow patch. (that one is now algorithmically safe, right?)

Ingo

john stultz

2006-12-13 18:49 UTC #62 lore raw

Re: [RFC] HZ free ntp

Quoted 17 lines · Ingo Molnar

Quoted 15 lines · john stultz
Quoted 9 lines · Roman Zippel
Quoted 3 lines · Ingo Molnar
i disagree with you and it's pretty low-impact anyway. There's still quite many HZ/tick assumptions all around the time code (NTP being one example), we'll deal with those via other patches.
Why do you pick on the NTP code? That's actually one of the places where assumptions about HZ are largely gone. NTP state is updated incrementally and this won't change, but the update frequency can now be easily disconnected from HZ.
Hey Roman,
Here's my rough first attempt at doing so. I'd not call it easy, but
maybe you have some suggestions for a simpler way?

Quoted 8 lines

Quoted 5 lines
Basically INTERVAL_LENGTH_NSEC defines the NTP interval length that the time code will use to accumulate with. In this patch I've pushed it out to a full second, but it could be set via config (NSEC_PER_SEC/HZ for regular systems, something larger for systems using dynticks).
cool! I'll give this one a go in -rt, combined with the exponential second-overflow patch. (that one is now algorithmically safe, right?)

No, this one will replace the exponential accumulation patch. So we'll accumulate on second intervals, which should be far enough apart that we won't be spinning in that loop for long.

thanks
-john

Roman Zippel

2006-12-13 13:58 UTC #63 lore raw

Re: [RFC] HZ free ntp

Hi,

Quoted 4 lines · john stultz

Basically INTERVAL_LENGTH_NSEC defines the NTP interval length that the time code will use to accumulate with. In this patch I've pushed it out to a full second, but it could be set via config (NSEC_PER_SEC/HZ for regular systems, something larger for systems using dynticks).

Why do you want to use such an interval? This makes everything only more complicated.
The largest possible interval is freq cycles (or 1 second without adjustments). That is the base interval and without redesigning NTP we can't change that. This base interval can be subdivided into smaller intervals for incremental updates.
You cannot choose arbitrary intervals otherwise you get other problems, e.g. with your patch time_offset handling is broken.

Quoted 8 lines

kernel/time/ntp.c +8 -0

+	/* calculate the length of one NTP adjusted second */
+	second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ);
+	second_length += (s64)CLOCK_TICK_ADJUST;
+	adj_length = (s64)time_freq;
+
+	/* calculate tick length @ HZ*/
+	tick_length = (second_length << TICK_LENGTH_SHIFT)
+			+ (adj_length << (TICK_LENGTH_SHIFT - SHIFT_NSEC));

kernel/time/ntp.c #define CLOCK_TICK_OVERFLOW (LATCH * HZ · line 75

Quoted 10 lines

kernel/time/ntp.c +10 -0

+	do_div(tick_length, HZ);
+	tick_nsec = tick_length >> TICK_LENGTH_SHIFT;
+
+
+	/* calculate interval_length_base */
+	/* XXX - this is broken up to avoid 64bit overlfows */
+	interval_length_base = second_length * INTERVAL_LENGTH_NSEC;
+	interval_length_base <<= 2;
+	do_div(interval_length_base, NSEC_PER_SEC);
+	interval_length_base <<= TICK_LENGTH_SHIFT-2;

You don't have to introduce anything new, it's tick_length that changes and HZ that becomes a variable in this function.

bye, Roman

john stultz

2006-12-13 19:19 UTC #64 lore raw

Re: [RFC] HZ free ntp

Quoted 9 lines · Roman Zippel

Hi,
Quoted 4 lines · john stultz
Basically INTERVAL_LENGTH_NSEC defines the NTP interval length that the time code will use to accumulate with. In this patch I've pushed it out to a full second, but it could be set via config (NSEC_PER_SEC/HZ for regular systems, something larger for systems using dynticks).

Quoted 6 lines

Why do you want to use such an interval? This makes everything only more complicated.
The largest possible interval is freq cycles (or 1 second without adjustments). That is the base interval and without redesigning NTP we can't change that. This base interval can be subdivided into smaller intervals for incremental updates.

Indeed, larger then 1 second intervals would require the second_overflow code to be reworked too. However, I'm not proposing larger then 1 second intervals at this point. I'm just allowing for non-HZ intervals.

Quoted 2 lines

You cannot choose arbitrary intervals otherwise you get other problems, e.g. with your patch time_offset handling is broken.

I'm not seeing this yet. Any more details?

Quoted 13 lines

kernel/time/ntp.c +13 -0

+	/* calculate the length of one NTP adjusted second */
+	second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ);
+	second_length += (s64)CLOCK_TICK_ADJUST;
+	adj_length = (s64)time_freq;
+
+	/* calculate tick length @ HZ*/
+	tick_length = (second_length << TICK_LENGTH_SHIFT)
+			+ (adj_length << (TICK_LENGTH_SHIFT - SHIFT_NSEC));
+	do_div(tick_length, HZ);
+	tick_nsec = tick_length >> TICK_LENGTH_SHIFT;
+
+
+	/* calculate interval_length_base */

Quoted 8 lines

kernel/time/ntp.c #define CLOCK_TICK_OVERFLOW (LATCH * HZ · line 75
Quoted 5 lines
kernel/time/ntp.c +5 -0
+	/* XXX - this is broken up to avoid 64bit overlfows */
+	interval_length_base = second_length * INTERVAL_LENGTH_NSEC;
+	interval_length_base <<= 2;
+	do_div(interval_length_base, NSEC_PER_SEC);
+	interval_length_base <<= TICK_LENGTH_SHIFT-2;
You don't have to introduce anything new, it's tick_length that changes and HZ that becomes a variable in this function.

So, forgive me for rehashing this, but it seems we're cross talking again. The context here is the dynticks code. Where HZ doesn't change, but we get interrupts at much reduced rates. The problem is, if the interrupts slow to ~1 per second frequencies, we end up spending a ton of time in the main update_wall_time loop, processing that 1 second in 1/HZ chunks. The current code is correct, but just wastes a lot of time.

So I'm trying to come up w/ an approach (the earlier, and broken, exponential accumulation patch had the same intention), that allows us to accumulate time in larger chunks. However, in doing so we have to work w/ the ntp.c code which (as Ingo earlier mentioned) has a number of HZ based assumptions.

This last patch tries to give NTP and the timekeeping an agreed upon chunk (I chose "interval" as the term, since "tick" is somewhat connected to HZ) of time upon which accumulation is done, so we can have courser second updates, or finer grained updates, depending on config settings.

In fairness, the patch probably going about this in a less then perfect way, but that's why I'm asking for your feedback and suggestions. What are your thoughts for reducing the time spent in the update_wall_time loop in the context of dynticks?

thanks
-john

Roman Zippel

2006-12-13 20:44 UTC #65 lore raw

Re: [RFC] HZ free ntp

Hi,

Quoted 7 lines · john stultz

Quoted 4 lines
The largest possible interval is freq cycles (or 1 second without adjustments). That is the base interval and without redesigning NTP we can't change that. This base interval can be subdivided into smaller intervals for incremental updates.
Indeed, larger then 1 second intervals would require the second_overflow code to be reworked too.

There isn't much to rework without a complete redesign.

Quoted 4 lines

Quoted 2 lines
You cannot choose arbitrary intervals otherwise you get other problems, e.g. with your patch time_offset handling is broken.
I'm not seeing this yet. Any more details?

time_offset is scaled to HZ in do_adjtimex, which needs to be changed as well.

Quoted 6 lines

Quoted 2 lines
You don't have to introduce anything new, it's tick_length that changes and HZ that becomes a variable in this function.
So, forgive me for rehashing this, but it seems we're cross talking again. The context here is the dynticks code. Where HZ doesn't change, but we get interrupts at much reduced rates.

I know and all you have to change in the ntp and some related code is to replace HZ there with a variable, thus make it changable, so you can increase the update interval (i.e. it becomes 1s/hz instead of 1s/HZ).

Quoted 3 lines

However, in doing so we have to
work w/ the ntp.c code which (as Ingo earlier mentioned) has a number of HZ based assumptions.

Repeating Ingo's nonsense doesn't make it any more true. :-(

bye, Roman

john stultz

2006-12-20 01:36 UTC #66 lore raw

Re: [RFC] HZ free ntp

Quoted 8 lines · Roman Zippel

Quoted 4 lines · john stultz
Quoted 2 lines
You cannot choose arbitrary intervals otherwise you get other problems, e.g. with your patch time_offset handling is broken.
I'm not seeing this yet. Any more details?
time_offset is scaled to HZ in do_adjtimex, which needs to be changed as well.

Ah, thanks! Fixed.

Quoted 10 lines

Quoted 6 lines
Quoted 2 lines
You don't have to introduce anything new, it's tick_length that changes and HZ that becomes a variable in this function.
So, forgive me for rehashing this, but it seems we're cross talking again. The context here is the dynticks code. Where HZ doesn't change, but we get interrupts at much reduced rates.
I know and all you have to change in the ntp and some related code is to replace HZ there with a variable, thus make it changable, so you can increase the update interval (i.e. it becomes 1s/hz instead of 1s/HZ).

Untested patch below. Does this vibe better with you are suggesting?

Any other suggestions or feedback?

thanks
-john

include/linux/timex.h +3 -0

index db501dc..8241e6e 100644
@@ -286,6 +286,9 @@ #endif /* !CONFIG_TIME_INTERPOLATION */
 
 #define TICK_LENGTH_SHIFT	32
 
+#define NTP_INTERVAL_FREQ  (HZ)
+#define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
+
 /* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */
 extern u64 current_tick_length(void);

kernel/time/ntp.c +19 ~~-11~~

index 3afeaa3..eb12509 100644
@@ -24,7 +24,7 @@ static u64 tick_length, tick_length_base
 
 #define MAX_TICKADJ		500		/* microsecs */
 #define MAX_TICKADJ_SCALED	(((u64)(MAX_TICKADJ * NSEC_PER_USEC) << \
-				  TICK_LENGTH_SHIFT) / HZ)
+				  TICK_LENGTH_SHIFT) / NTP_INTERVAL_FREQ)
 
 /*
  * phase-lock loop variables

@@ -46,13 +46,17 @@ #define CLOCK_TICK_ADJUST	(((s64)CLOCK_T
 
 static void ntp_update_frequency(void)
 {
-	tick_length_base = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) << TICK_LENGTH_SHIFT;
-	tick_length_base += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
-	tick_length_base += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC);
+	u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
+				<< TICK_LENGTH_SHIFT;
+	second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
+	second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC);
 
-	do_div(tick_length_base, HZ);
+	tick_length_base = second_length;
 
-	tick_nsec = tick_length_base >> TICK_LENGTH_SHIFT;
+	do_div(second_length, HZ);
+	tick_nsec = second_length >> TICK_LENGTH_SHIFT;
+
+	do_div(tick_length_base, NTP_INTERVAL_FREQ);
 }
 
 /**

@@ -162,7 +166,7 @@ void second_overflow(void)
 			tick_length -= MAX_TICKADJ_SCALED;
 		} else {
 			tick_length += (s64)(time_adjust * NSEC_PER_USEC /
-					     HZ) << TICK_LENGTH_SHIFT;
+					NTP_INTERVAL_FREQ) << TICK_LENGTH_SHIFT;
 			time_adjust = 0;
 		}
 	}

@@ -239,7 +243,8 @@ #endif
 		    result = -EINVAL;
 		    goto leave;
 		}
-		time_freq = ((s64)txc->freq * NSEC_PER_USEC) >> (SHIFT_USEC - SHIFT_NSEC);
+		time_freq = ((s64)txc->freq * NSEC_PER_USEC)
+				>> (SHIFT_USEC - SHIFT_NSEC);
 	    }
 
 	    if (txc->modes & ADJ_MAXERROR) {

@@ -309,7 +314,8 @@ #endif
 		    freq_adj += time_freq;
 		    freq_adj = min(freq_adj, (s64)MAXFREQ_NSEC);
 		    time_freq = max(freq_adj, (s64)-MAXFREQ_NSEC);
-		    time_offset = (time_offset / HZ) << SHIFT_UPDATE;
+		    time_offset = (time_offset / NTP_INTERVAL_FREQ)
+		    			<< SHIFT_UPDATE;
 		} /* STA_PLL */
 	    } /* txc->modes & ADJ_OFFSET */
 	    if (txc->modes & ADJ_TICK)

@@ -324,8 +330,10 @@ leave:	if ((time_status & (STA_UNSYNC|ST
 	if ((txc->modes & ADJ_OFFSET_SINGLESHOT) == ADJ_OFFSET_SINGLESHOT)
 	    txc->offset	   = save_adjust;
 	else
-	    txc->offset    = shift_right(time_offset, SHIFT_UPDATE) * HZ / 1000;
-	txc->freq	   = (time_freq / NSEC_PER_USEC) << (SHIFT_USEC - SHIFT_NSEC);
+	    txc->offset    = shift_right(time_offset, SHIFT_UPDATE)
+	    			* NTP_INTERVAL_FREQ / 1000;
+	txc->freq	   = (time_freq / NSEC_PER_USEC)
+				<< (SHIFT_USEC - SHIFT_NSEC);
 	txc->maxerror	   = time_maxerror;
 	txc->esterror	   = time_esterror;
 	txc->status	   = time_status;

kernel/timer.c +2 -2

index 0256ab4..616b7fd 100644
@@ -890,7 +890,7 @@ void __init timekeeping_init(void)
 	ntp_clear();
 
 	clock = clocksource_get_next();
-	clocksource_calculate_interval(clock, tick_nsec);
+	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
 	clock->cycle_last = clocksource_read(clock);
 
 	write_sequnlock_irqrestore(&xtime_lock, flags);

@@ -1092,7 +1092,7 @@ #endif
 	if (change_clocksource()) {
 		clock->error = 0;
 		clock->xtime_nsec = 0;
-		clocksource_calculate_interval(clock, tick_nsec);
+		clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
 	}
 }

john stultz

2006-12-20 01:57 UTC #67 lore raw

Re: [RFC] HZ free ntp

Quoted 9 lines · john stultz

Quoted 8 lines · Roman Zippel
Quoted 6 lines · john stultz
Quoted 2 lines
You don't have to introduce anything new, it's tick_length that changes and HZ that becomes a variable in this function.
So, forgive me for rehashing this, but it seems we're cross talking again. The context here is the dynticks code. Where HZ doesn't change, but we get interrupts at much reduced rates.

Quoted 5 lines

Quoted 3 lines
I know and all you have to change in the ntp and some related code is to replace HZ there with a variable, thus make it changable, so you can increase the update interval (i.e. it becomes 1s/hz instead of 1s/HZ).
Untested patch below. Does this vibe better with you are suggesting?

And here would be the follow on patch (again *untested*) for CONFIG_NO_HZ slowing the time accumulation down to once per second.

thanks
-john

include/linux/timex.h +4 -0

index 8241e6e..3beb539 100644
@@ -286,7 +286,11 @@ #endif /* !CONFIG_TIME_INTERPOLATION */
 
 #define TICK_LENGTH_SHIFT	32
 
+#ifdef CONFIG_NO_HZ
+#define NTP_INTERVAL_FREQ  (1)
+#else
 #define NTP_INTERVAL_FREQ  (HZ)
+#endif
 #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
 
 /* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */

kernel/hrtimer.c +3 -1

index d0ba190..53979a9 100644
@@ -127,12 +127,14 @@ EXPORT_SYMBOL_GPL(ktime_get_ts);
  */
 static void hrtimer_get_softirq_time(struct hrtimer_base *base)
 {
+	struct timespec ts;
 	ktime_t xtim, tomono;
 	unsigned long seq;
 
 	do {
 		seq = read_seqbegin(&xtime_lock);
-		xtim = timespec_to_ktime(xtime);
+		getnstimeofday(&ts);
+		xtim = timespec_to_ktime(ts);
 		tomono = timespec_to_ktime(wall_to_monotonic);
 
 	} while (read_seqretry(&xtime_lock, seq));

Andrew Morton

author

2006-12-21 04:26 UTC #68 lore raw

Re: [RFC] HZ free ntp

Quoted 10 lines · On Tue, 19 Dec 2006 17:54:18 -0800 john stultz <[email protected]> wrote:

Quoted 9 lines · john stultz
Quoted 8 lines · Roman Zippel
Quoted 6 lines · john stultz
Quoted 2 lines
You don't have to introduce anything new, it's tick_length that changes and HZ that becomes a variable in this function.
So, forgive me for rehashing this, but it seems we're cross talking again. The context here is the dynticks code. Where HZ doesn't change, but we get interrupts at much reduced rates.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
I know and all you have to change in the ntp and some related code is to replace HZ there with a variable, thus make it changable, so you can increase the update interval (i.e. it becomes 1s/hz instead of 1s/HZ).
Untested patch below. Does this vibe better with you are suggesting?
And here would be the follow on patch (again *untested*) for CONFIG_NO_HZ slowing the time accumulation down to once per second.

I'm still awaiting a final-looking version of this patch, fyi.

I don't understand whether this is a theoretical might-happen thing, or if NTP problems have actually been observed in the field?

Either way, I'm sure the final changelog will clear that up ;)

Roman Zippel

2007-01-01 19:28 UTC #69 lore raw

Re: [RFC] HZ free ntp

Quoted 2 lines · john stultz

And here would be the follow on patch (again *untested*) for CONFIG_NO_HZ slowing the time accumulation down to once per second.

Changing it to one creates a potential problem with calling second_overflow(). It should be called every NTP_INTERVAL_FREQ times, but occasionally it's off by one (when xtime is close to a full second and the tick length is different from 1sec). At a higher frequency that's not much of a problem, but at one it means second_overflow() is occasionally called twice a second or skipped for a second. Usually the error should be quite small, but sometimes it can be significant.
So in this case the loop in update_wall_time() should rather look like this:

	while (offset >= clock->cycle_interval) {
		...
		second_overflow();
		while (clock->xtime_nsec >= (u64)NSEC_PER_SEC << clock->shift) {
			clock->xtime_nsec -= (u64)NSEC_PER_SEC << clock->shift;
			xtime.tv_sec++;
		}
		...
	}

(Also note the change from "if" to "while".)

Another problem area is that the clock shift value might need adjustments, since when you reduce the update frequency, it also increases the error adjustment steps and thus influences the jitter. This is especially important if we want to synchronize the TSC on multiple cpu's.

bye, Roman

john stultz

2007-01-02 19:50 UTC #70 lore raw

Re: [RFC] HZ free ntp

Quoted 24 lines · Roman Zippel

Quoted 2 lines · john stultz
And here would be the follow on patch (again *untested*) for CONFIG_NO_HZ slowing the time accumulation down to once per second.
Changing it to one creates a potential problem with calling second_overflow(). It should be called every NTP_INTERVAL_FREQ times, but occasionally it's off by one (when xtime is close to a full second and the tick length is different from 1sec). At a higher frequency that's not much of a problem, but at one it means second_overflow() is occasionally called twice a second or skipped for a second. Usually the error should be quite small, but sometimes it can be significant.
So in this case the loop in update_wall_time() should rather look like this:
	while (offset >= clock->cycle_interval) {
		...
		second_overflow();
		while (clock->xtime_nsec >= (u64)NSEC_PER_SEC << clock->shift) {
			clock->xtime_nsec -= (u64)NSEC_PER_SEC << clock->shift;
			xtime.tv_sec++;
		}
		...
	}

Quoted 1 line

(Also note the change from "if" to "while".)

Ah, good catch! Thank you, I'll add that, retest and send it to akpm.

thanks
-john

john stultz

2007-01-02 20:55 UTC #71 lore raw

Re: [RFC] HZ free ntp

Quoted 7 lines · john stultz

Quoted 6 lines · Roman Zippel
Quoted 2 lines · john stultz
And here would be the follow on patch (again *untested*) for CONFIG_NO_HZ slowing the time accumulation down to once per second.
Changing it to one creates a potential problem with calling second_overflow().

Wait, at first I thought I understood this, but looking closer, I'm not so sure I do.

Quoted 1 line

Quoted 1 line
It should be called every NTP_INTERVAL_FREQ times, but occasionally it's off

Wait, so second_overflow should be called every NTP_INTERVAL_FREQ times (instead of every second)? Surely that's not right.

Quoted 17 lines

Quoted 17 lines
by one (when xtime is close to a full second and the tick length is different from 1sec). At a higher frequency that's not much of a problem, but at one it means second_overflow() is occasionally called twice a second or skipped for a second. Usually the error should be quite small, but sometimes it can be significant.
So in this case the loop in update_wall_time() should rather look like this:
	while (offset >= clock->cycle_interval) {
		...
		second_overflow();
		while (clock->xtime_nsec >= (u64)NSEC_PER_SEC << clock->shift) {
			clock->xtime_nsec -= (u64)NSEC_PER_SEC << clock->shift;
			xtime.tv_sec++;
		}
		...
	}

Quoted 1 line

Quoted 1 line
(Also note the change from "if" to "while".)

This would assume that clock->cycle_interval would *always* be the length of a full second and that isn't what the patch trying to do.

Maybe could you explain this some more?

thanks
-john

Roman Zippel

2007-01-07 02:09 UTC #72 lore raw

Re: [RFC] HZ free ntp

Hi,

Quoted 5 lines · john stultz

Quoted 2 lines
Quoted 2 lines
It should be called every NTP_INTERVAL_FREQ times, but occasionally it's off
Wait, so second_overflow should be called every NTP_INTERVAL_FREQ times (instead of every second)? Surely that's not right.

But it is, that's the reason the various adjustment values are divided by it, so they are applied to the next NTP_INTERVAL_FREQ times.

BTW I think NTP_INTERVAL_FREQ isn't the right name, CLOCK_UPDATE_FREQ would be a better name, currently ntp is the main user, but a clock can also be updated via other means (e.g. adjtimex or another clock).

Quoted 13 lines

Quoted 13 lines
Quoted 13 lines
So in this case the loop in update_wall_time() should rather look like this:
	while (offset >= clock->cycle_interval) {
		...
		second_overflow();
		while (clock->xtime_nsec >= (u64)NSEC_PER_SEC << clock->shift) {
			clock->xtime_nsec -= (u64)NSEC_PER_SEC << clock->shift;
			xtime.tv_sec++;
		}
		...
	}

Quoted 6 lines

Quoted 1 line
Quoted 1 line
(Also note the change from "if" to "while".)
This would assume that clock->cycle_interval would *always* be the length of a full second and that isn't what the patch trying to do.
Maybe could you explain this some more?

As I said this was the case for a value of one.
Anyway, to avoid these problems, I'd prefer to keep it at least at 2 or better at 4. This would still drastically reduce the time spent in the loop and we can revisit the issue later.

bye, Roman

Ingo Molnar

2007-01-22 19:28 UTC #73 lore raw

PATCH 1/1 HZ-free NTP

Quoted 2 lines · john stultz

And here would be the follow on patch (again *untested*) for CONFIG_NO_HZ slowing the time accumulation down to once per second.

thanks John - i've applied the combined patch below to -rt. It appears to work fine for me.

Ingo

------------------------>
From: John Stultz <[email protected]>
Subject: [patch] HZ-free NTP

Signed-off-by Ingo Molnar <[email protected]>

---
 include/linux/timex.h |    4 ++++
 kernel/hrtimer.c      |    4 +++-
 kernel/time/ntp.c     |   30 +++++++++++++++++++-----------
 kernel/timer.c        |    4 ++--
 4 files changed, 28 insertions(+), 14 deletions(-)

include/linux/timex.h +4 -0

Index: linux/include/linux/timex.h
===================================================================
@@ -286,7 +286,11 @@ static inline void time_interpolator_upd
 
 #define TICK_LENGTH_SHIFT	32
 
+#ifdef CONFIG_NO_HZ
+#define NTP_INTERVAL_FREQ  (1)
+#else
 #define NTP_INTERVAL_FREQ  (HZ)
+#endif
 #define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
 
 /* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */

kernel/hrtimer.c +3 -1

Index: linux/kernel/hrtimer.c
===================================================================
@@ -135,12 +135,14 @@ EXPORT_SYMBOL_GPL(ktime_get_ts);
  */
 static void hrtimer_get_softirq_time(struct hrtimer_cpu_base *base)
 {
+	struct timespec ts;
 	ktime_t xtim, tomono;
 	unsigned long seq;
 
 	do {
 		seq = read_seqbegin(&xtime_lock);
-		xtim = timespec_to_ktime(xtime);
+		getnstimeofday(&ts);
+		xtim = timespec_to_ktime(ts);
 		tomono = timespec_to_ktime(wall_to_monotonic);
 
 	} while (read_seqretry(&xtime_lock, seq));

kernel/time/ntp.c +19 ~~-11~~

Index: linux/kernel/time/ntp.c
===================================================================
@@ -24,7 +24,7 @@ static u64 tick_length, tick_length_base
 
 #define MAX_TICKADJ		500		/* microsecs */
 #define MAX_TICKADJ_SCALED	(((u64)(MAX_TICKADJ * NSEC_PER_USEC) << \
-				  TICK_LENGTH_SHIFT) / HZ)
+				  TICK_LENGTH_SHIFT) / NTP_INTERVAL_FREQ)
 
 /*
  * phase-lock loop variables

@@ -46,13 +46,17 @@ long time_adjust;
 
 static void ntp_update_frequency(void)
 {
-	tick_length_base = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) << TICK_LENGTH_SHIFT;
-	tick_length_base += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
-	tick_length_base += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC);
+	u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
+				<< TICK_LENGTH_SHIFT;
+	second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
+	second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC);
 
-	do_div(tick_length_base, HZ);
+	tick_length_base = second_length;
 
-	tick_nsec = tick_length_base >> TICK_LENGTH_SHIFT;
+	do_div(second_length, HZ);
+	tick_nsec = second_length >> TICK_LENGTH_SHIFT;
+
+	do_div(tick_length_base, NTP_INTERVAL_FREQ);
 }
 
 /**

@@ -164,7 +168,7 @@ void second_overflow(void)
 			tick_length -= MAX_TICKADJ_SCALED;
 		} else {
 			tick_length += (s64)(time_adjust * NSEC_PER_USEC /
-					     HZ) << TICK_LENGTH_SHIFT;
+					NTP_INTERVAL_FREQ) << TICK_LENGTH_SHIFT;
 			time_adjust = 0;
 		}
 	}

@@ -241,7 +245,8 @@ int do_adjtimex(struct timex *txc)
 		    result = -EINVAL;
 		    goto leave;
 		}
-		time_freq = ((s64)txc->freq * NSEC_PER_USEC) >> (SHIFT_USEC - SHIFT_NSEC);
+		time_freq = ((s64)txc->freq * NSEC_PER_USEC)
+				>> (SHIFT_USEC - SHIFT_NSEC);
 	    }
 
 	    if (txc->modes & ADJ_MAXERROR) {

@@ -311,7 +316,8 @@ int do_adjtimex(struct timex *txc)
 		    freq_adj += time_freq;
 		    freq_adj = min(freq_adj, (s64)MAXFREQ_NSEC);
 		    time_freq = max(freq_adj, (s64)-MAXFREQ_NSEC);
-		    time_offset = (time_offset / HZ) << SHIFT_UPDATE;
+		    time_offset = (time_offset / NTP_INTERVAL_FREQ)
+		    			<< SHIFT_UPDATE;
 		} /* STA_PLL */
 	    } /* txc->modes & ADJ_OFFSET */
 	    if (txc->modes & ADJ_TICK)

@@ -326,8 +332,10 @@ leave:	if ((time_status & (STA_UNSYNC|ST
 	if ((txc->modes & ADJ_OFFSET_SINGLESHOT) == ADJ_OFFSET_SINGLESHOT)
 	    txc->offset	   = save_adjust;
 	else
-	    txc->offset    = shift_right(time_offset, SHIFT_UPDATE) * HZ / 1000;
-	txc->freq	   = (time_freq / NSEC_PER_USEC) << (SHIFT_USEC - SHIFT_NSEC);
+	    txc->offset    = shift_right(time_offset, SHIFT_UPDATE)
+	    			* NTP_INTERVAL_FREQ / 1000;
+	txc->freq	   = (time_freq / NSEC_PER_USEC)
+				<< (SHIFT_USEC - SHIFT_NSEC);
 	txc->maxerror	   = time_maxerror;
 	txc->esterror	   = time_esterror;
 	txc->status	   = time_status;

kernel/timer.c +2 -2

Index: linux/kernel/timer.c
===================================================================
@@ -1030,7 +1030,7 @@ static void change_clocksource(void)
 
 	clock->error = 0;
 	clock->xtime_nsec = 0;
-	clocksource_calculate_interval(clock, tick_nsec);
+	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
 
 	tick_clock_notify();

@@ -1086,7 +1086,7 @@ void __init timekeeping_init(void)
 	ntp_clear();
 
 	clock = clocksource_get_next();
-	clocksource_calculate_interval(clock, tick_nsec);
+	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
 	clock->cycle_last = clocksource_read(clock);
 
 	xtime.tv_sec = sec;

Ingo Molnar

2007-01-22 19:41 UTC #74 lore raw

Re: [patch] HZ-free NTP

Quoted 2 lines · Ingo Molnar

thanks John - i've applied the combined patch below to -rt. It appears to work fine for me.

updated patch below. (previous one was a delta in timex.h)

Ingo

---------------->
From: John Stultz <[email protected]>
Subject: [patch] HZ-free NTP

Signed-off-by Ingo Molnar <[email protected]>

---
 include/linux/timex.h |    7 +++++++
 kernel/hrtimer.c      |    4 +++-
 kernel/time/ntp.c     |   30 +++++++++++++++++++-----------
 kernel/timer.c        |    4 ++--
 4 files changed, 31 insertions(+), 14 deletions(-)

include/linux/timex.h +7 -0

Index: linux/include/linux/timex.h
===================================================================
@@ -286,6 +286,13 @@ static inline void time_interpolator_upd
 
 #define TICK_LENGTH_SHIFT	32
 
+#ifdef CONFIG_NO_HZ
+#define NTP_INTERVAL_FREQ  (1)
+#else
+#define NTP_INTERVAL_FREQ  (HZ)
+#endif
+#define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
+
 /* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */
 extern u64 current_tick_length(void);

kernel/hrtimer.c +3 -1

Index: linux/kernel/hrtimer.c
===================================================================
@@ -135,12 +135,14 @@ EXPORT_SYMBOL_GPL(ktime_get_ts);
  */
 static void hrtimer_get_softirq_time(struct hrtimer_cpu_base *base)
 {
+	struct timespec ts;
 	ktime_t xtim, tomono;
 	unsigned long seq;
 
 	do {
 		seq = read_seqbegin(&xtime_lock);
-		xtim = timespec_to_ktime(xtime);
+		getnstimeofday(&ts);
+		xtim = timespec_to_ktime(ts);
 		tomono = timespec_to_ktime(wall_to_monotonic);
 
 	} while (read_seqretry(&xtime_lock, seq));

kernel/time/ntp.c +19 ~~-11~~

Index: linux/kernel/time/ntp.c
===================================================================
@@ -24,7 +24,7 @@ static u64 tick_length, tick_length_base
 
 #define MAX_TICKADJ		500		/* microsecs */
 #define MAX_TICKADJ_SCALED	(((u64)(MAX_TICKADJ * NSEC_PER_USEC) << \
-				  TICK_LENGTH_SHIFT) / HZ)
+				  TICK_LENGTH_SHIFT) / NTP_INTERVAL_FREQ)
 
 /*
  * phase-lock loop variables

@@ -46,13 +46,17 @@ long time_adjust;
 
 static void ntp_update_frequency(void)
 {
-	tick_length_base = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ) << TICK_LENGTH_SHIFT;
-	tick_length_base += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
-	tick_length_base += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC);
+	u64 second_length = (u64)(tick_usec * NSEC_PER_USEC * USER_HZ)
+				<< TICK_LENGTH_SHIFT;
+	second_length += (s64)CLOCK_TICK_ADJUST << TICK_LENGTH_SHIFT;
+	second_length += (s64)time_freq << (TICK_LENGTH_SHIFT - SHIFT_NSEC);
 
-	do_div(tick_length_base, HZ);
+	tick_length_base = second_length;
 
-	tick_nsec = tick_length_base >> TICK_LENGTH_SHIFT;
+	do_div(second_length, HZ);
+	tick_nsec = second_length >> TICK_LENGTH_SHIFT;
+
+	do_div(tick_length_base, NTP_INTERVAL_FREQ);
 }
 
 /**

@@ -164,7 +168,7 @@ void second_overflow(void)
 			tick_length -= MAX_TICKADJ_SCALED;
 		} else {
 			tick_length += (s64)(time_adjust * NSEC_PER_USEC /
-					     HZ) << TICK_LENGTH_SHIFT;
+					NTP_INTERVAL_FREQ) << TICK_LENGTH_SHIFT;
 			time_adjust = 0;
 		}
 	}

@@ -241,7 +245,8 @@ int do_adjtimex(struct timex *txc)
 		    result = -EINVAL;
 		    goto leave;
 		}
-		time_freq = ((s64)txc->freq * NSEC_PER_USEC) >> (SHIFT_USEC - SHIFT_NSEC);
+		time_freq = ((s64)txc->freq * NSEC_PER_USEC)
+				>> (SHIFT_USEC - SHIFT_NSEC);
 	    }
 
 	    if (txc->modes & ADJ_MAXERROR) {

@@ -311,7 +316,8 @@ int do_adjtimex(struct timex *txc)
 		    freq_adj += time_freq;
 		    freq_adj = min(freq_adj, (s64)MAXFREQ_NSEC);
 		    time_freq = max(freq_adj, (s64)-MAXFREQ_NSEC);
-		    time_offset = (time_offset / HZ) << SHIFT_UPDATE;
+		    time_offset = (time_offset / NTP_INTERVAL_FREQ)
+		    			<< SHIFT_UPDATE;
 		} /* STA_PLL */
 	    } /* txc->modes & ADJ_OFFSET */
 	    if (txc->modes & ADJ_TICK)

@@ -326,8 +332,10 @@ leave:	if ((time_status & (STA_UNSYNC|ST
 	if ((txc->modes & ADJ_OFFSET_SINGLESHOT) == ADJ_OFFSET_SINGLESHOT)
 	    txc->offset	   = save_adjust;
 	else
-	    txc->offset    = shift_right(time_offset, SHIFT_UPDATE) * HZ / 1000;
-	txc->freq	   = (time_freq / NSEC_PER_USEC) << (SHIFT_USEC - SHIFT_NSEC);
+	    txc->offset    = shift_right(time_offset, SHIFT_UPDATE)
+	    			* NTP_INTERVAL_FREQ / 1000;
+	txc->freq	   = (time_freq / NSEC_PER_USEC)
+				<< (SHIFT_USEC - SHIFT_NSEC);
 	txc->maxerror	   = time_maxerror;
 	txc->esterror	   = time_esterror;
 	txc->status	   = time_status;

kernel/timer.c +2 -2

Index: linux/kernel/timer.c
===================================================================
@@ -1030,7 +1030,7 @@ static void change_clocksource(void)
 
 	clock->error = 0;
 	clock->xtime_nsec = 0;
-	clocksource_calculate_interval(clock, tick_nsec);
+	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
 
 	tick_clock_notify();

@@ -1086,7 +1086,7 @@ void __init timekeeping_init(void)
 	ntp_clear();
 
 	clock = clocksource_get_next();
-	clocksource_calculate_interval(clock, tick_nsec);
+	clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
 	clock->cycle_last = clocksource_read(clock);
 
 	xtime.tv_sec = sec;

Roman Zippel

2007-01-01 19:23 UTC #75 lore raw

Re: [RFC] HZ free ntp

Hi,

Quoted 5 lines · john stultz

Quoted 3 lines
I know and all you have to change in the ntp and some related code is to replace HZ there with a variable, thus make it changable, so you can increase the update interval (i.e. it becomes 1s/hz instead of 1s/HZ).
Untested patch below. Does this vibe better with you are suggesting?

Yes, thanks.
tick_nsec doesn't require special treatment, in the middle term it's obsolete anyway, it could be replaced with (current_tick_length() >> TICK_LENGTH_SHIFT) and current_tick_length() being inlined. NTP_INTERVAL_FREQ could be a real variable (so it can be initialized at runtime), it's already gone from all important paths.
In the short term I'd prefered a clock would store its frequency instead of using NTP_INTERVAL_LENGTH in clocksource_calculate_interval(), so it doesn't has to be derived there.

bye, Roman

john stultz

2007-01-02 19:46 UTC #76 lore raw

Re: [RFC] HZ free ntp

Quoted 6 lines · Roman Zippel

Quoted 4 lines · john stultz
Quoted 3 lines
I know and all you have to change in the ntp and some related code is to replace HZ there with a variable, thus make it changable, so you can increase the update interval (i.e. it becomes 1s/hz instead of 1s/HZ).

Quoted 6 lines

Quoted 1 line
Untested patch below. Does this vibe better with you are suggesting?
Yes, thanks.
tick_nsec doesn't require special treatment, in the middle term it's obsolete anyway, it could be replaced with (current_tick_length() >> TICK_LENGTH_SHIFT) and current_tick_length() being inlined.

If NTP_INTERVAL_FREQ is different then HZ, then tick_nsec still has a different meaning then (current_tick_length() >> TICK_LENGTH_SHIFT). So since tick_nsec is still used in quite a few places, so I'm hesitant to pull it.

Quoted 2 lines

NTP_INTERVAL_FREQ could be a real variable (so it can be initialized at runtime), it's already gone from all important paths.

Wait, so you're suggesting NTP_INTERVAL_FREQ be a dynamic variable instead of a constant? Curious, could you give a bit more detail on why?

Quoted 3 lines

In the short term I'd prefered a clock would store its frequency instead of using NTP_INTERVAL_LENGTH in clocksource_calculate_interval(), so it doesn't has to be derived there.

I don't follow this at all. clocksources do store their own frequency (via mult/shift). Could you explain?

thanks
-john

Roman Zippel

2007-01-07 02:09 UTC #77 lore raw

Re: [RFC] HZ free ntp

Hi,

Quoted 8 lines · john stultz

Quoted 3 lines
tick_nsec doesn't require special treatment, in the middle term it's obsolete anyway, it could be replaced with (current_tick_length() >> TICK_LENGTH_SHIFT) and current_tick_length() being inlined.
If NTP_INTERVAL_FREQ is different then HZ, then tick_nsec still has a different meaning then (current_tick_length() >> TICK_LENGTH_SHIFT). So since tick_nsec is still used in quite a few places, so I'm hesitant to pull it.

The current usage under arch is pretty much bogus and they likely can't use dyntick anyway, so it would be easier to disable tick_nsec completely if dyntick is enabled.

Quoted 5 lines

Quoted 2 lines
NTP_INTERVAL_FREQ could be a real variable (so it can be initialized at runtime), it's already gone from all important paths.
Wait, so you're suggesting NTP_INTERVAL_FREQ be a dynamic variable instead of a constant? Curious, could you give a bit more detail on why?

We already have more than enough config options, where the user has barely any idea what to do, so we should try to configure and initialize as much as possible at runtime depending on what the hardware is capable of.

That reminds me that the main problem left for a dynamic variable is time_offset. It should become a 64 bit value, so SHIFT_UPDATE isn't needed anymore. Right now it depends on HZ to maximize the value range.

Quoted 6 lines

Quoted 3 lines
In the short term I'd prefered a clock would store its frequency instead of using NTP_INTERVAL_LENGTH in clocksource_calculate_interval(), so it doesn't has to be derived there.
I don't follow this at all. clocksources do store their own frequency (via mult/shift). Could you explain?

mult is not a constant and calculating the frequency like this is not very precise.

bye, Roman

Roman Zippel

2006-12-06 12:33 UTC #78 lore raw

Hi,

Quoted 5 lines · Andrew Morton

Quoted 3 lines
IMO it least at needs one more iteration to address the comments that were made (not just mine), in the short term the less it touches unconditionally the less I care right now.
I don't have a clue which review comments remain unaddressed - do you recall?

Outside clockevents I'd like to see at least the flag handling fixed before it gets merged.
Inside clockevents I could poke around forever...

Quoted 8 lines

Quoted 3 lines
In the long term IMO this might need a major rework, the basic problem I have is that I don't see how this usable beyond dynticks/hrtimer, e.g. how to dynamically manage multiple timer.
I'm not sure I understand that. Are you referring to multiple, concurrently-operating hardware clock sources? <wonders how that could work> If so, that's more a clocksource thing than a dynticks/hrtimer thing, isn't it?

A rather simple example would be profiling, where a separate timer is useful to see stuff that runs from the main timer, which is currently invisible.
It's insofar a clocksource thing as clock source and clock events should form a union, currently it's separate and that's a big problem. It's not really problem to have multiple clock sources and they don't really have to be synchronized with each other, but events _are_ connected to the source they are coming from.
In the end we could even expose multiple clocks via the posix clock/timer interface, but with the current design I don't see how this is possible.

bye, Roman

Stephen Smalley

2006-12-08 14:16 UTC #79 lore raw

Quoted 21 lines · Andrew Morton

mprotect-patch-for-use-by-slim.patch integrity-service-api-and-dummy-provider.patch integrity-service-api-and-dummy-provider-cleanup-use-of-configh.patch integrity-service-api-and-dummy-provider-compilation-warning-fix.patch slim-main-patch.patch slim-main-patch-socket_post_create-hook-return-code.patch slim-main-patch-misc-cleanups-requested-at-inclusion-time.patch slim-main-patch-handle-failure-to-register.patch slim-main-patch-fix-bug-with-mm_users-usage.patch slim-main-patch-security-slim-slm_mainc-make-2-functions-static.patch slim-secfs-patch.patch slim-secfs-patch-slim-correct-use-of-snprintf.patch slim-secfs-patch-cleanup-use-of-configh.patch slim-make-and-config-stuff.patch slim-make-and-config-stuff-makefile-fix.patch slim-debug-output.patch slim-fix-security-issue-with-the-task_post_setuid-hook.patch slim-secfs-inode-i_private-build-fix.patch slim-documentation.patch fdtable-make-fdarray-and-fdsets-equal-in-size-slim.patch

Quoted 1 line

Shall hold in -mm.

Why? I haven't seen any evidence that prior review comments have been addressed so far, and a fresh patch set would be beneficial anyway to facilitate full review of the updated code and to allow them to fix their patch descriptions as well (as they were wrong in some instances, describing older versions of the code).

Signature · 2 lines

Stephen Smalley
National Security Agency

Andrew Morton

author

2006-12-08 21:02 UTC #80 lore raw

Quoted 22 lines · On Fri, 08 Dec 2006 09:09:34 -0500 Stephen Smalley <[email protected]> wrote:

Quoted 21 lines · Andrew Morton
mprotect-patch-for-use-by-slim.patch integrity-service-api-and-dummy-provider.patch integrity-service-api-and-dummy-provider-cleanup-use-of-configh.patch integrity-service-api-and-dummy-provider-compilation-warning-fix.patch slim-main-patch.patch slim-main-patch-socket_post_create-hook-return-code.patch slim-main-patch-misc-cleanups-requested-at-inclusion-time.patch slim-main-patch-handle-failure-to-register.patch slim-main-patch-fix-bug-with-mm_users-usage.patch slim-main-patch-security-slim-slm_mainc-make-2-functions-static.patch slim-secfs-patch.patch slim-secfs-patch-slim-correct-use-of-snprintf.patch slim-secfs-patch-cleanup-use-of-configh.patch slim-make-and-config-stuff.patch slim-make-and-config-stuff-makefile-fix.patch slim-debug-output.patch slim-fix-security-issue-with-the-task_post_setuid-hook.patch slim-secfs-inode-i_private-build-fix.patch slim-documentation.patch fdtable-make-fdarray-and-fdsets-equal-in-size-slim.patch

Quoted 3 lines

Quoted 1 line
Shall hold in -mm.
Why?

They're on hold awaiting further development and awaiting a merge/no-merge decision.

They're not causing me any trouble.

Quoted 5 lines

I haven't seen any evidence that prior review comments have been
addressed so far, and a fresh patch set would be beneficial anyway to facilitate full review of the updated code and to allow them to fix their patch descriptions as well (as they were wrong in some instances, describing older versions of the code).

If/when the developers start doing more work, we can then decide whether to use incremental patches or to take a drop-them-and-start-again approach.

(If a whole new patch series comes out, I have tricks which allow me to check that none of the above fixup patches got lost. Those tricks don't work if I drop all the patches first)

But yes, it has been pretty quiet. If there's no intention to proceed with these patches, someone please tell me.

Mimi Zohar

2006-12-10 15:07 UTC #81 lore raw

Andrew Morton <[email protected]> wrote on 12/08/2006 03:58:51 PM:

Quoted 4 lines

Quoted 2 lines
Quoted 1 line
Shall hold in -mm.
Why?
They're on hold awaiting further development and awaiting a

merge/no-merge

Quoted 1 line

They're not causing me any trouble.

Thank you! We are getting ready to re-release EVM as an integrity provider. As this version of EVM will not be using the LSM hooks, which mailing list should we be posting the code?

As for SLIM, we will be addressing Stephen's comments and submitting changes as necessary shortly thereafter.

Mimi Zohar

Randy Dunlap

2006-12-09 09:30 UTC #82 lore raw