* proc_misc.c bug
@ 2003-04-10 22:02 David Mosberger
2003-04-10 21:44 ` Alan Cox
2003-04-10 22:18 ` Andrew Morton
0 siblings, 2 replies; 12+ messages in thread
From: David Mosberger @ 2003-04-10 22:02 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel
interrupts_open() can easily try to kmalloc() more memory than
supported by kmalloc. E.g., with 16KB page size and NR_CPUS==64, it
would try to allocate 147456 bytes.
The workaround below is to allocate 4KB per 8 CPUs. Not really a
solution, but the fundamental problem is that /proc/interrupts
shouldn't use a fixed buffer size in the first place. I suppose
another solution would be to use vmalloc() instead. It all feels like
bandaids though.
--david
===== fs/proc/proc_misc.c 1.71 vs edited =====
--- 1.71/fs/proc/proc_misc.c Sat Mar 22 22:14:49 2003
+++ edited/fs/proc/proc_misc.c Thu Apr 10 14:35:16 2003
@@ -388,7 +388,7 @@
extern int show_interrupts(struct seq_file *p, void *v);
static int interrupts_open(struct inode *inode, struct file *file)
{
- unsigned size = PAGE_SIZE * (1 + NR_CPUS / 8);
+ unsigned size = 4096 * (1 + NR_CPUS / 8);
char *buf = kmalloc(size, GFP_KERNEL);
struct seq_file *m;
int res;
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: proc_misc.c bug 2003-04-10 22:02 proc_misc.c bug David Mosberger @ 2003-04-10 21:44 ` Alan Cox 2003-04-10 22:49 ` Randy.Dunlap ` (2 more replies) 2003-04-10 22:18 ` Andrew Morton 1 sibling, 3 replies; 12+ messages in thread From: Alan Cox @ 2003-04-10 21:44 UTC (permalink / raw) To: davidm; +Cc: akpm, Linux Kernel Mailing List On Thu, 2003-04-10 at 23:02, David Mosberger wrote: > The workaround below is to allocate 4KB per 8 CPUs. Not really a > solution, but the fundamental problem is that /proc/interrupts > shouldn't use a fixed buffer size in the first place. I suppose > another solution would be to use vmalloc() instead. It all feels like > bandaids though. How about switching to Al's seqfile interface ? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: proc_misc.c bug 2003-04-10 21:44 ` Alan Cox @ 2003-04-10 22:49 ` Randy.Dunlap 2003-04-11 5:01 ` Randy.Dunlap 2003-04-10 22:53 ` Andrew Morton 2003-04-11 0:27 ` David Mosberger 2 siblings, 1 reply; 12+ messages in thread From: Randy.Dunlap @ 2003-04-10 22:49 UTC (permalink / raw) To: Alan Cox; +Cc: davidm, akpm, linux-kernel On 10 Apr 2003 22:44:17 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: | On Thu, 2003-04-10 at 23:02, David Mosberger wrote: | > The workaround below is to allocate 4KB per 8 CPUs. Not really a | > solution, but the fundamental problem is that /proc/interrupts | > shouldn't use a fixed buffer size in the first place. I suppose | > another solution would be to use vmalloc() instead. It all feels like | > bandaids though. | | How about switching to Al's seqfile interface ? It's already using it, but it uses the simple/single version of it, which doesn't automagically extend the output buffer area when it's full, so a max size buffer has to be allocated for it up front. I'll look at changing it unless somebody beats me (to it :). -- ~Randy ['tangent' is not a verb...unless you believe that "in English any noun can be verbed."] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: proc_misc.c bug 2003-04-10 22:49 ` Randy.Dunlap @ 2003-04-11 5:01 ` Randy.Dunlap 2003-04-11 5:28 ` Andrew Morton 2003-04-11 5:41 ` David Mosberger 0 siblings, 2 replies; 12+ messages in thread From: Randy.Dunlap @ 2003-04-11 5:01 UTC (permalink / raw) To: rddunlap; +Cc: alan, davidm, akpm, linux-kernel > On 10 Apr 2003 22:44:17 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: > > | On Thu, 2003-04-10 at 23:02, David Mosberger wrote: > | > The workaround below is to allocate 4KB per 8 CPUs. Not really a | > > solution, but the fundamental problem is that /proc/interrupts | > shouldn't > use a fixed buffer size in the first place. I suppose | > another solution > would be to use vmalloc() instead. It all feels like | > bandaids though. > | > | How about switching to Al's seqfile interface ? > > It's already using it, but it uses the simple/single version of it, which > doesn't automagically extend the output buffer area when > it's full, so a max size buffer has to be allocated for it > up front. > > I'll look at changing it unless somebody beats me (to it :). OK, I've looked at it and concluded that it's not bad the way it is (after David's patch is applied). However, that really depends on whether the static NR_CPUS is well-tuned or not. If it's not tuned, then modifying the output to use the iterative seq_file methods would make sense. But if it's not tuned, someone is (usually) wasting lots of memory anyway. [adding this:] Or a better approximation instead of NR_CPUS might be to add and use something a bit more intelligent about how many interrupts are in use. However, this makes my later argument not so strong. The reason that I say it's not bad the way it is is that it does one kmalloc() for the output buffer and then it can run quickly and do its job [but it's limited to the original kmalloc buffer size]. If it's modified to use seq_file iteration, it will start out with kmalloc(PAGE_SIZE) in seq_read(), and when that fills up, a buffer twice that size is kmalloc-ed, repeat.... with earlier buffers being freed first. Does someone want to disagree now? go a() instead. It all feels like | > bandaids though. > | > | How about switching to Al's seqfile interface ? > > It's already using it, but it uses the simple/single version of it, which > doesn't automagically extend the output buffer area when > it's full, so a max size buffer has to be allocated for it > up front. > > I'll look at changing it unless somebody beats me (to it :). OK, I've looked at it and concluded that it's not bad the way it is (after David's patch is applied). However, that really depends on whether the static NR_CPUS is well-tuned or not. If it's not tuned, then modifying the output to use the iterative seq_file methods would make sense. But if it's not tuned, someone is (usually) wasting lots of memory anyway. [adding this:] Or a better approximation instead of NR_CPUS might be to add and use something a bit more intelligent about how many interrupts are in use. However, this makes my later argument not so strong. The reason that I say it's not bad the way it is is that it does one kmalloc() for the output buffer and then it can run quickly and do its job [but it's limited to the original kmalloc buffer size]. If it's modified to use seq_file iteration, it will start out with kmalloc(PAGE_SIZE) in seq_read(), and when that fills up, a buffer twice that size is kmalloc-ed, repeat.... with earlier buffers being freed first. Does someone want to disagree now? go ahead...i'm listening. Maybe the reason to modify it is that NR_CPUS is not a good approximation/hint/clue. ~Randy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: proc_misc.c bug 2003-04-11 5:01 ` Randy.Dunlap @ 2003-04-11 5:28 ` Andrew Morton 2003-04-11 5:41 ` David Mosberger 1 sibling, 0 replies; 12+ messages in thread From: Andrew Morton @ 2003-04-11 5:28 UTC (permalink / raw) To: Randy.Dunlap; +Cc: alan, davidm, linux-kernel "Randy.Dunlap" <rddunlap@osdl.org> wrote: > > Maybe the reason to modify it is that NR_CPUS is not a good > approximation/hint/clue. NR_CPUS is not tooooo bad an approximation. After all, the output size is approximately proportional to the number of CPUs. Multiplied by the number of interrupts :( CPU0 CPU1 0: 11982656 0 IO-APIC-edge timer 1: 26 0 IO-APIC-edge i8042 2: 0 0 XT-PIC cascade We could perhaps just convert it to num_online_cpus() and run away. Depends how heoric you're feeling. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: proc_misc.c bug 2003-04-11 5:01 ` Randy.Dunlap 2003-04-11 5:28 ` Andrew Morton @ 2003-04-11 5:41 ` David Mosberger 2003-04-11 5:46 ` Zwane Mwaikambo 2003-04-11 17:29 ` Randy.Dunlap 1 sibling, 2 replies; 12+ messages in thread From: David Mosberger @ 2003-04-11 5:41 UTC (permalink / raw) To: Randy.Dunlap; +Cc: alan, davidm, akpm, linux-kernel >>>>> On Thu, 10 Apr 2003 22:01:43 -0700 (PDT), "Randy.Dunlap" <rddunlap@osdl.org> said: Randy> OK, I've looked at it and concluded that it's not bad the way Randy> it is (after David's patch is applied). However, that really Randy> depends on whether the static NR_CPUS is well-tuned or not. Randy> If it's not tuned, then modifying the output to use the Randy> iterative seq_file methods would make sense. But if it's not Randy> tuned, someone is (usually) wasting lots of memory anyway. Randy> [snip...] Randy> Does someone want to disagree now? go ahead...i'm listening. Randy> Maybe the reason to modify it is that NR_CPUS is not a good Randy> approximation/hint/clue. Wouldn't the kmalloc() likely fail in fragmented conditions? Also, I'm wondering whether there is such a thing as "well-tuned" in this case. For example, in the extreme case of the SGI SN2 machine, each CPU could in theory have up to 256 interrupt sources (OK, perhaps it's only 256 interrupts per 2 CPUs, but it's still a lot of interrupts to go around ;-). OTOH, most ia64 machines out there have less than 256 interrupt per _system_. That's a large variation. --david ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: proc_misc.c bug 2003-04-11 5:41 ` David Mosberger @ 2003-04-11 5:46 ` Zwane Mwaikambo 2003-04-11 17:29 ` Randy.Dunlap 1 sibling, 0 replies; 12+ messages in thread From: Zwane Mwaikambo @ 2003-04-11 5:46 UTC (permalink / raw) To: davidm; +Cc: Randy.Dunlap, alan, akpm, linux-kernel On Thu, 10 Apr 2003, David Mosberger wrote: > Wouldn't the kmalloc() likely fail in fragmented conditions? Also, > I'm wondering whether there is such a thing as "well-tuned" in this > case. For example, in the extreme case of the SGI SN2 machine, each > CPU could in theory have up to 256 interrupt sources (OK, perhaps it's > only 256 interrupts per 2 CPUs, but it's still a lot of interrupts to > go around ;-). OTOH, most ia64 machines out there have less than 256 > interrupt per _system_. That's a large variation. I think NR_CPUS belongs in there somewhere, this is what triggered the original change; CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 CPU6 CPU7 CPU8 CPU9 CPU10 CPU11 CPU12 CPU13 CPU14 CPU15 CPU16 CPU17 CPU18 CPU19 CPU20 CPU21 CPU22 CPU23 CPU24 CPU25 CPU26 CPU27 CPU28 CPU29 CPU30 CPU31 0: 3652909 0 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 local-APIC-edge timer 2: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 XT-PIC cascade 4: 2274 2134 1993 2147 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-edge serial 8: 0 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level rtc 19: 1082 1068 527 1118 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level eth0 37: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level eth7 38: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level eth6 39: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level eth5 40: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level eth4 41: 3558 2894 2648 3244 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level qlogicisp 89: 0 0 0 0 4 4 4 3 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level qlogicisp 115: 0 0 0 0 0 0 0 0 361 366 366 347 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level eth1 163: 0 0 0 0 0 0 0 0 0 0 0 0 383 385 385 384 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level eth2 211: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 375 376 374 375 0 0 0 0 0 0 0 0 0 0 0 0 IO-APIC-level eth3 NMI: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 LOC: 3604478 3603640 3603819 3603799 3603663 3603641 3603627 3603601 3603603 3603583 3603563 3603542 3603588 3603567 3603546 3603526 3603416 3603394 3603374 3603352 3603601 3603578 3603557 3603536 3603707 3603686 3603665 3603644 3603430 3603410 3603388 3603368 ERR: 0 MIS: 0 -- function.linuxpower.ca ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: proc_misc.c bug 2003-04-11 5:41 ` David Mosberger 2003-04-11 5:46 ` Zwane Mwaikambo @ 2003-04-11 17:29 ` Randy.Dunlap 2003-04-11 18:32 ` David Mosberger 1 sibling, 1 reply; 12+ messages in thread From: Randy.Dunlap @ 2003-04-11 17:29 UTC (permalink / raw) To: davidm; +Cc: alan, akpm, linux-kernel On Thu, 10 Apr 2003 22:41:23 -0700 David Mosberger <davidm@napali.hpl.hp.com> wrote: | >>>>> On Thu, 10 Apr 2003 22:01:43 -0700 (PDT), "Randy.Dunlap" <rddunlap@osdl.org> said: | | Randy> OK, I've looked at it and concluded that it's not bad the way | Randy> it is (after David's patch is applied). However, that really | Randy> depends on whether the static NR_CPUS is well-tuned or not. | Randy> If it's not tuned, then modifying the output to use the | Randy> iterative seq_file methods would make sense. But if it's not | Randy> tuned, someone is (usually) wasting lots of memory anyway. | | Randy> [snip...] | | Randy> Does someone want to disagree now? go ahead...i'm listening. | Randy> Maybe the reason to modify it is that NR_CPUS is not a good | Randy> approximation/hint/clue. | | Wouldn't the kmalloc() likely fail in fragmented conditions? Also, | I'm wondering whether there is such a thing as "well-tuned" in this | case. For example, in the extreme case of the SGI SN2 machine, each | CPU could in theory have up to 256 interrupt sources (OK, perhaps it's | only 256 interrupts per 2 CPUs, but it's still a lot of interrupts to | go around ;-). OTOH, most ia64 machines out there have less than 256 | interrupt per _system_. That's a large variation. For kmalloc() failing, do you mean the first (large) kmalloc() or the repeated ones that grow in size each time? I would think that just doing one big kmalloc up front is desireable to repeated ones...if the first one does a decent estimate of its max size. I just don't know how likely that is. -- ~Randy ['tangent' is not a verb...unless you believe that "in English any noun can be verbed."] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: proc_misc.c bug 2003-04-11 17:29 ` Randy.Dunlap @ 2003-04-11 18:32 ` David Mosberger 0 siblings, 0 replies; 12+ messages in thread From: David Mosberger @ 2003-04-11 18:32 UTC (permalink / raw) To: Randy.Dunlap; +Cc: davidm, alan, akpm, linux-kernel >>>>> On Fri, 11 Apr 2003 10:29:46 -0700, "Randy.Dunlap" <rddunlap@osdl.org> said: Randy> For kmalloc() failing, do you mean the first (large) Randy> kmalloc() or the repeated ones that grow in size each time? I was concerned about the kmalloc() in interrupts_open(). --david ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: proc_misc.c bug 2003-04-10 21:44 ` Alan Cox 2003-04-10 22:49 ` Randy.Dunlap @ 2003-04-10 22:53 ` Andrew Morton 2003-04-11 0:27 ` David Mosberger 2 siblings, 0 replies; 12+ messages in thread From: Andrew Morton @ 2003-04-10 22:53 UTC (permalink / raw) To: Alan Cox; +Cc: davidm, linux-kernel Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: > > On Thu, 2003-04-10 at 23:02, David Mosberger wrote: > > The workaround below is to allocate 4KB per 8 CPUs. Not really a > > solution, but the fundamental problem is that /proc/interrupts > > shouldn't use a fixed buffer size in the first place. I suppose > > another solution would be to use vmalloc() instead. It all feels like > > bandaids though. > > How about switching to Al's seqfile interface ? It's already using it! Not sure why it needs to buffer all the text in one big slurp. There are about 20 different show_interrupts()es which need updating if someone decides to fix it for real. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: proc_misc.c bug 2003-04-10 21:44 ` Alan Cox 2003-04-10 22:49 ` Randy.Dunlap 2003-04-10 22:53 ` Andrew Morton @ 2003-04-11 0:27 ` David Mosberger 2 siblings, 0 replies; 12+ messages in thread From: David Mosberger @ 2003-04-11 0:27 UTC (permalink / raw) To: Alan Cox; +Cc: davidm, akpm, Linux Kernel Mailing List >>>>> On 10 Apr 2003 22:44:17 +0100, Alan Cox <alan@lxorguk.ukuu.org.uk> said: Alan> On Thu, 2003-04-10 at 23:02, David Mosberger wrote: >> The workaround below is to allocate 4KB per 8 CPUs. Not really a >> solution, but the fundamental problem is that /proc/interrupts >> shouldn't use a fixed buffer size in the first place. I suppose >> another solution would be to use vmalloc() instead. It all feels like >> bandaids though. Alan> How about switching to Al's seqfile interface ? That's obviously the non-bandaid solution I was hinting at. Any volunteers? --david ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: proc_misc.c bug 2003-04-10 22:02 proc_misc.c bug David Mosberger 2003-04-10 21:44 ` Alan Cox @ 2003-04-10 22:18 ` Andrew Morton 1 sibling, 0 replies; 12+ messages in thread From: Andrew Morton @ 2003-04-10 22:18 UTC (permalink / raw) To: davidm; +Cc: linux-kernel David Mosberger <davidm@napali.hpl.hp.com> wrote: > > interrupts_open() can easily try to kmalloc() more memory than > supported by kmalloc. E.g., with 16KB page size and NR_CPUS==64, it > would try to allocate 147456 bytes. > > The workaround below is to allocate 4KB per 8 CPUs. Not really a > solution, but the fundamental problem is that /proc/interrupts > shouldn't use a fixed buffer size in the first place. I suppose > another solution would be to use vmalloc() instead. It all feels like > bandaids though. > > --david > > ===== fs/proc/proc_misc.c 1.71 vs edited ===== > --- 1.71/fs/proc/proc_misc.c Sat Mar 22 22:14:49 2003 > +++ edited/fs/proc/proc_misc.c Thu Apr 10 14:35:16 2003 > @@ -388,7 +388,7 @@ > extern int show_interrupts(struct seq_file *p, void *v); > static int interrupts_open(struct inode *inode, struct file *file) > { > - unsigned size = PAGE_SIZE * (1 + NR_CPUS / 8); > + unsigned size = 4096 * (1 + NR_CPUS / 8); urgh, consider me thwapped. There continue to be a lot of places where we assume that pages are 4k (eg: sizing of the free page reserves). But few are as fatal as this one... Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2003-04-11 18:21 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-04-10 22:02 proc_misc.c bug David Mosberger 2003-04-10 21:44 ` Alan Cox 2003-04-10 22:49 ` Randy.Dunlap 2003-04-11 5:01 ` Randy.Dunlap 2003-04-11 5:28 ` Andrew Morton 2003-04-11 5:41 ` David Mosberger 2003-04-11 5:46 ` Zwane Mwaikambo 2003-04-11 17:29 ` Randy.Dunlap 2003-04-11 18:32 ` David Mosberger 2003-04-10 22:53 ` Andrew Morton 2003-04-11 0:27 ` David Mosberger 2003-04-10 22:18 ` Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).