| Message ID | 20100708162442.ADD40F891E@sepang.rtg.net |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 2010-07-08 at 10:24 -0600, Tim Gardner wrote: > diff -u ureadahead-0.100.0/debian/changelog ureadahead-0.100.0/debian/changelog > --- ureadahead-0.100.0/debian/changelog > +++ ureadahead-0.100.0/debian/changelog > @@ -1,3 +1,9 @@ > +ureadahead (0.100.0-5ubuntu1) maverick; urgency=low > + > + * > + > + -- Tim Gardner <rtg@lochsa> Thu, 08 Jul 2010 16:04:39 +0000 > + > ureadahead (0.100.0-5) maverick; urgency=low > > * src/pack.c: Amend mount point detection logic to stat the mount point > only in patch2: > unchanged: > --- ureadahead-0.100.0.orig/src/trace.c > +++ ureadahead-0.100.0/src/trace.c > @@ -122,6 +122,7 @@ > int old_open_exec_enabled = 0; > int old_uselib_enabled = 0; > int old_tracing_enabled = 0; > + int old_buffer_size_kb = 0; > struct sigaction act; > struct sigaction old_sigterm; > struct sigaction old_sigint; > @@ -165,7 +166,7 @@ > > old_uselib_enabled = -1; > } > - if (set_value (dfd, "buffer_size_kb", 128000, NULL) < 0) > + if (set_value (dfd, "buffer_size_kb", 128000, &old_buffer_size_kb) < 0) > goto error; > if (set_value (dfd, "tracing_enabled", > TRUE, &old_tracing_enabled) < 0) > @@ -217,6 +218,9 @@ > if (set_value (dfd, "events/fs/do_sys_open/enable", > old_sys_open_enabled, NULL) < 0) > goto error; > + if (set_value (dfd, "buffer_size_kb", > + old_buffer_size_kb, NULL) < 0) > + goto error; > > /* Be nicer */ > if (nice (15)) > I'm guessing that set_value copies what would be the result of "cat buffer_size_kb" into old_buffer_size_kb. Unfortunately, that would end up with: 7 (expanded: 1408) Or something like that. This output signifies that at first we only have 7 KB/cpu of buffer, but as soon as a function trace is started we allocate 1408 KB/cpu. When you manually change the value, you automatically get pushed to the expanded state. This is a one-way operation; we can't go back to the original state once we bump the value in ureadahead. I think that the best thing is to just set it back to 1 (a minimum value). If someone wants to do a trace, they can bump this value up. I'm wary of even going back to the expanded value because in the vast majority of cases we're just wasting memory. -- Chase
Tim, This patch is quite on time for my hibernation bug in a customer's project. The root cause is ureadahead will set the tracing buffer size to 128MB, then do profiling. But after that ureadahead never restores the old tracing buffer size back. Due to the big chunk of memory occupied by ureadahead, hibernation will fail when it preallocate memory to build hibernation image in memory. We tried to restore back the tracing buffer size to workaround this issue. So I just wanna say, please push this patch to ureadahead repo and upload for us. I will test this patch on the machine. Thanks again, -Bryan On 07/09/2010 12:24 AM, Tim Gardner wrote: > diff -u ureadahead-0.100.0/debian/changelog ureadahead-0.100.0/debian/changelog > --- ureadahead-0.100.0/debian/changelog > +++ ureadahead-0.100.0/debian/changelog > @@ -1,3 +1,9 @@ > +ureadahead (0.100.0-5ubuntu1) maverick; urgency=low > + > + * > + > + -- Tim Gardner <rtg@lochsa> Thu, 08 Jul 2010 16:04:39 +0000 > + > ureadahead (0.100.0-5) maverick; urgency=low > > * src/pack.c: Amend mount point detection logic to stat the mount point > only in patch2: > unchanged: > --- ureadahead-0.100.0.orig/src/trace.c > +++ ureadahead-0.100.0/src/trace.c > @@ -122,6 +122,7 @@ > int old_open_exec_enabled = 0; > int old_uselib_enabled = 0; > int old_tracing_enabled = 0; > + int old_buffer_size_kb = 0; > struct sigaction act; > struct sigaction old_sigterm; > struct sigaction old_sigint; > @@ -165,7 +166,7 @@ > > old_uselib_enabled = -1; > } > - if (set_value (dfd, "buffer_size_kb", 128000, NULL) < 0) > + if (set_value (dfd, "buffer_size_kb", 128000, &old_buffer_size_kb) < 0) > goto error; > if (set_value (dfd, "tracing_enabled", > TRUE, &old_tracing_enabled) < 0) > @@ -217,6 +218,9 @@ > if (set_value (dfd, "events/fs/do_sys_open/enable", > old_sys_open_enabled, NULL) < 0) > goto error; > + if (set_value (dfd, "buffer_size_kb", > + old_buffer_size_kb, NULL) < 0) > + goto error; > > /* Be nicer */ > if (nice (15)) >
On 07/08/2010 01:46 PM, Chase Douglas wrote: > On Thu, 2010-07-08 at 10:24 -0600, Tim Gardner wrote: >> diff -u ureadahead-0.100.0/debian/changelog ureadahead-0.100.0/debian/changelog >> --- ureadahead-0.100.0/debian/changelog >> +++ ureadahead-0.100.0/debian/changelog >> @@ -1,3 +1,9 @@ >> +ureadahead (0.100.0-5ubuntu1) maverick; urgency=low >> + >> + * >> + >> + -- Tim Gardner<rtg@lochsa> Thu, 08 Jul 2010 16:04:39 +0000 >> + >> ureadahead (0.100.0-5) maverick; urgency=low >> >> * src/pack.c: Amend mount point detection logic to stat the mount point >> only in patch2: >> unchanged: >> --- ureadahead-0.100.0.orig/src/trace.c >> +++ ureadahead-0.100.0/src/trace.c >> @@ -122,6 +122,7 @@ >> int old_open_exec_enabled = 0; >> int old_uselib_enabled = 0; >> int old_tracing_enabled = 0; >> + int old_buffer_size_kb = 0; >> struct sigaction act; >> struct sigaction old_sigterm; >> struct sigaction old_sigint; >> @@ -165,7 +166,7 @@ >> >> old_uselib_enabled = -1; >> } >> - if (set_value (dfd, "buffer_size_kb", 128000, NULL)< 0) >> + if (set_value (dfd, "buffer_size_kb", 128000,&old_buffer_size_kb)< 0) >> goto error; >> if (set_value (dfd, "tracing_enabled", >> TRUE,&old_tracing_enabled)< 0) >> @@ -217,6 +218,9 @@ >> if (set_value (dfd, "events/fs/do_sys_open/enable", >> old_sys_open_enabled, NULL)< 0) >> goto error; >> + if (set_value (dfd, "buffer_size_kb", >> + old_buffer_size_kb, NULL)< 0) >> + goto error; >> >> /* Be nicer */ >> if (nice (15)) >> > > I'm guessing that set_value copies what would be the result of "cat > buffer_size_kb" into old_buffer_size_kb. Unfortunately, that would end > up with: > > 7 (expanded: 1408) > > Or something like that. This output signifies that at first we only have > 7 KB/cpu of buffer, but as soon as a function trace is started we > allocate 1408 KB/cpu. When you manually change the value, you > automatically get pushed to the expanded state. This is a one-way > operation; we can't go back to the original state once we bump the value > in ureadahead. > > I think that the best thing is to just set it back to 1 (a minimum > value). If someone wants to do a trace, they can bump this value up. I'm > wary of even going back to the expanded value because in the vast > majority of cases we're just wasting memory. > > -- Chase > > The first thing set_value() does is to perform atoi() on the buffer read from /sys/kernel/debug/tracing/buffer_size_kb, the first token of which appears to always be an integer. Isn't that the number we'd like to preserve? rtg
On Thu, 2010-07-08 at 19:19 -0600, Tim Gardner wrote: > On 07/08/2010 01:46 PM, Chase Douglas wrote: > > On Thu, 2010-07-08 at 10:24 -0600, Tim Gardner wrote: > >> diff -u ureadahead-0.100.0/debian/changelog ureadahead-0.100.0/debian/changelog > >> --- ureadahead-0.100.0/debian/changelog > >> +++ ureadahead-0.100.0/debian/changelog > >> @@ -1,3 +1,9 @@ > >> +ureadahead (0.100.0-5ubuntu1) maverick; urgency=low > >> + > >> + * > >> + > >> + -- Tim Gardner<rtg@lochsa> Thu, 08 Jul 2010 16:04:39 +0000 > >> + > >> ureadahead (0.100.0-5) maverick; urgency=low > >> > >> * src/pack.c: Amend mount point detection logic to stat the mount point > >> only in patch2: > >> unchanged: > >> --- ureadahead-0.100.0.orig/src/trace.c > >> +++ ureadahead-0.100.0/src/trace.c > >> @@ -122,6 +122,7 @@ > >> int old_open_exec_enabled = 0; > >> int old_uselib_enabled = 0; > >> int old_tracing_enabled = 0; > >> + int old_buffer_size_kb = 0; > >> struct sigaction act; > >> struct sigaction old_sigterm; > >> struct sigaction old_sigint; > >> @@ -165,7 +166,7 @@ > >> > >> old_uselib_enabled = -1; > >> } > >> - if (set_value (dfd, "buffer_size_kb", 128000, NULL)< 0) > >> + if (set_value (dfd, "buffer_size_kb", 128000,&old_buffer_size_kb)< 0) > >> goto error; > >> if (set_value (dfd, "tracing_enabled", > >> TRUE,&old_tracing_enabled)< 0) > >> @@ -217,6 +218,9 @@ > >> if (set_value (dfd, "events/fs/do_sys_open/enable", > >> old_sys_open_enabled, NULL)< 0) > >> goto error; > >> + if (set_value (dfd, "buffer_size_kb", > >> + old_buffer_size_kb, NULL)< 0) > >> + goto error; > >> > >> /* Be nicer */ > >> if (nice (15)) > >> > > > > I'm guessing that set_value copies what would be the result of "cat > > buffer_size_kb" into old_buffer_size_kb. Unfortunately, that would end > > up with: > > > > 7 (expanded: 1408) > > > > Or something like that. This output signifies that at first we only have > > 7 KB/cpu of buffer, but as soon as a function trace is started we > > allocate 1408 KB/cpu. When you manually change the value, you > > automatically get pushed to the expanded state. This is a one-way > > operation; we can't go back to the original state once we bump the value > > in ureadahead. > > > > I think that the best thing is to just set it back to 1 (a minimum > > value). If someone wants to do a trace, they can bump this value up. I'm > > wary of even going back to the expanded value because in the vast > > majority of cases we're just wasting memory. > > > > -- Chase > > > > > > The first thing set_value() does is to perform atoi() on the buffer read > from /sys/kernel/debug/tracing/buffer_size_kb, the first token of which > appears to always be an integer. Isn't that the number we'd like to > preserve? I think the default in that case is 7 KB/cpu, which is reasonable. Since the set_value() implementation can handle what's read from the file, I think this is reasonable. -- Chase
On 07/08/2010 07:15 PM, Bryan Wu wrote: > Tim, > > This patch is quite on time for my hibernation bug in a customer's project. > > The root cause is ureadahead will set the tracing buffer size to 128MB, then do > profiling. But after that ureadahead never restores the old tracing buffer size > back. Due to the big chunk of memory occupied by ureadahead, hibernation will > fail when it preallocate memory to build hibernation image in memory. > > We tried to restore back the tracing buffer size to workaround this issue. > > So I just wanna say, please push this patch to ureadahead repo and upload for > us. I will test this patch on the machine. > > Thanks again, > -Bryan > Bryan, I've uploaded a package to http://ppa.launchpad.net/timg-tpi/ppa/ubuntu. I tested using 'sudo ureadahead --force-trace'. It appears to do the right thing. rtg
On 07/09/2010 09:49 AM, Tim Gardner wrote: > On 07/08/2010 07:15 PM, Bryan Wu wrote: >> Tim, >> >> This patch is quite on time for my hibernation bug in a customer's >> project. >> >> The root cause is ureadahead will set the tracing buffer size to >> 128MB, then do >> profiling. But after that ureadahead never restores the old tracing >> buffer size >> back. Due to the big chunk of memory occupied by ureadahead, >> hibernation will >> fail when it preallocate memory to build hibernation image in memory. >> >> We tried to restore back the tracing buffer size to workaround this >> issue. >> >> So I just wanna say, please push this patch to ureadahead repo and >> upload for >> us. I will test this patch on the machine. >> >> Thanks again, >> -Bryan >> > > Bryan, > > I've uploaded a package to http://ppa.launchpad.net/timg-tpi/ppa/ubuntu. > I tested using 'sudo ureadahead --force-trace'. It appears to do the > right thing. > Awesome. I noticed it is built for amd64 arch, shall we get a i386 version? The customer's machine is running i386 generice-pae kernel with 4G RAM. Thanks,
On 07/08/2010 08:05 PM, Bryan Wu wrote: > On 07/09/2010 09:49 AM, Tim Gardner wrote: >> On 07/08/2010 07:15 PM, Bryan Wu wrote: >>> Tim, >>> >>> This patch is quite on time for my hibernation bug in a customer's >>> project. >>> >>> The root cause is ureadahead will set the tracing buffer size to >>> 128MB, then do >>> profiling. But after that ureadahead never restores the old tracing >>> buffer size >>> back. Due to the big chunk of memory occupied by ureadahead, >>> hibernation will >>> fail when it preallocate memory to build hibernation image in memory. >>> >>> We tried to restore back the tracing buffer size to workaround this >>> issue. >>> >>> So I just wanna say, please push this patch to ureadahead repo and >>> upload for >>> us. I will test this patch on the machine. >>> >>> Thanks again, >>> -Bryan >>> >> >> Bryan, >> >> I've uploaded a package to http://ppa.launchpad.net/timg-tpi/ppa/ubuntu. >> I tested using 'sudo ureadahead --force-trace'. It appears to do the >> right thing. >> > > Awesome. I noticed it is built for amd64 arch, shall we get a i386 version? The > customer's machine is running i386 generice-pae kernel with 4G RAM. > > Thanks, https://edge.launchpad.net/~timg-tpi/+archive/ppa/+files/ureadahead_0.100.0-4.2~lucid1_i386.deb ?
On 07/09/2010 10:18 AM, Tim Gardner wrote: > On 07/08/2010 08:05 PM, Bryan Wu wrote: >> On 07/09/2010 09:49 AM, Tim Gardner wrote: >>> On 07/08/2010 07:15 PM, Bryan Wu wrote: >>>> Tim, >>>> >>>> This patch is quite on time for my hibernation bug in a customer's >>>> project. >>>> >>>> The root cause is ureadahead will set the tracing buffer size to >>>> 128MB, then do >>>> profiling. But after that ureadahead never restores the old tracing >>>> buffer size >>>> back. Due to the big chunk of memory occupied by ureadahead, >>>> hibernation will >>>> fail when it preallocate memory to build hibernation image in memory. >>>> >>>> We tried to restore back the tracing buffer size to workaround this >>>> issue. >>>> >>>> So I just wanna say, please push this patch to ureadahead repo and >>>> upload for >>>> us. I will test this patch on the machine. >>>> >>>> Thanks again, >>>> -Bryan >>>> >>> >>> Bryan, >>> >>> I've uploaded a package to http://ppa.launchpad.net/timg-tpi/ppa/ubuntu. >>> I tested using 'sudo ureadahead --force-trace'. It appears to do the >>> right thing. >>> >> >> Awesome. I noticed it is built for amd64 arch, shall we get a i386 >> version? The >> customer's machine is running i386 generice-pae kernel with 4G RAM. >> >> Thanks, > > https://edge.launchpad.net/~timg-tpi/+archive/ppa/+files/ureadahead_0.100.0-4.2~lucid1_i386.deb > ? > Cool, I just downloaded and will try it on our testing machine. btw, how can I found the i386 link you posted in your launchpad ppa page? -Bryan
On 07/08/2010 09:01 PM, Bryan Wu wrote: > On 07/09/2010 10:18 AM, Tim Gardner wrote: >> On 07/08/2010 08:05 PM, Bryan Wu wrote: >>> On 07/09/2010 09:49 AM, Tim Gardner wrote: >>>> On 07/08/2010 07:15 PM, Bryan Wu wrote: >>>>> Tim, >>>>> >>>>> This patch is quite on time for my hibernation bug in a customer's >>>>> project. >>>>> >>>>> The root cause is ureadahead will set the tracing buffer size to >>>>> 128MB, then do >>>>> profiling. But after that ureadahead never restores the old tracing >>>>> buffer size >>>>> back. Due to the big chunk of memory occupied by ureadahead, >>>>> hibernation will >>>>> fail when it preallocate memory to build hibernation image in memory. >>>>> >>>>> We tried to restore back the tracing buffer size to workaround this >>>>> issue. >>>>> >>>>> So I just wanna say, please push this patch to ureadahead repo and >>>>> upload for >>>>> us. I will test this patch on the machine. >>>>> >>>>> Thanks again, >>>>> -Bryan >>>>> >>>> >>>> Bryan, >>>> >>>> I've uploaded a package to http://ppa.launchpad.net/timg-tpi/ppa/ubuntu. >>>> I tested using 'sudo ureadahead --force-trace'. It appears to do the >>>> right thing. >>>> >>> >>> Awesome. I noticed it is built for amd64 arch, shall we get a i386 >>> version? The >>> customer's machine is running i386 generice-pae kernel with 4G RAM. >>> >>> Thanks, >> >> https://edge.launchpad.net/~timg-tpi/+archive/ppa/+files/ureadahead_0.100.0-4.2~lucid1_i386.deb >> ? >> > > Cool, I just downloaded and will try it on our testing machine. > > btw, how can I found the i386 link you posted in your launchpad ppa page? > > -Bryan Go to anyone's PPA page (like http://launchpad.net/~timg-tpi/+archive/ppa) and click on 'View Package Details', then click on the package of interest. rtg
hi, Am Donnerstag, den 08.07.2010, 10:24 -0600 schrieb Tim Gardner: > diff -u ureadahead-0.100.0/debian/changelog ureadahead-0.100.0/debian/changelog > --- ureadahead-0.100.0/debian/changelog > +++ ureadahead-0.100.0/debian/changelog > @@ -1,3 +1,9 @@ > +ureadahead (0.100.0-5ubuntu1) maverick; urgency=low > + > + * > + > + -- Tim Gardner <rtg@lochsa> Thu, 08 Jul 2010 16:04:39 +0000 > + > ureadahead (0.100.0-5) maverick; urgency=low > > * src/pack.c: Amend mount point detection logic to stat the mount point > only in patch2: > unchanged: > --- ureadahead-0.100.0.orig/src/trace.c > +++ ureadahead-0.100.0/src/trace.c > @@ -122,6 +122,7 @@ > int old_open_exec_enabled = 0; > int old_uselib_enabled = 0; > int old_tracing_enabled = 0; > + int old_buffer_size_kb = 0; > struct sigaction act; > struct sigaction old_sigterm; > struct sigaction old_sigint; > @@ -165,7 +166,7 @@ > > old_uselib_enabled = -1; > } > - if (set_value (dfd, "buffer_size_kb", 128000, NULL) < 0) > + if (set_value (dfd, "buffer_size_kb", 128000, &old_buffer_size_kb) < 0) > goto error; > if (set_value (dfd, "tracing_enabled", > TRUE, &old_tracing_enabled) < 0) > @@ -217,6 +218,9 @@ > if (set_value (dfd, "events/fs/do_sys_open/enable", > old_sys_open_enabled, NULL) < 0) > goto error; > + if (set_value (dfd, "buffer_size_kb", > + old_buffer_size_kb, NULL) < 0) > + goto error; > > /* Be nicer */ > if (nice (15)) > this looks like it might fix the OOM invocations we see on OMAP3 256M systems (beagleboard) ciao oli
diff -u ureadahead-0.100.0/debian/changelog ureadahead-0.100.0/debian/changelog --- ureadahead-0.100.0/debian/changelog +++ ureadahead-0.100.0/debian/changelog @@ -1,3 +1,9 @@ +ureadahead (0.100.0-5ubuntu1) maverick; urgency=low + + * + + -- Tim Gardner <rtg@lochsa> Thu, 08 Jul 2010 16:04:39 +0000 + ureadahead (0.100.0-5) maverick; urgency=low * src/pack.c: Amend mount point detection logic to stat the mount point only in patch2: unchanged: --- ureadahead-0.100.0.orig/src/trace.c +++ ureadahead-0.100.0/src/trace.c @@ -122,6 +122,7 @@ int old_open_exec_enabled = 0; int old_uselib_enabled = 0; int old_tracing_enabled = 0; + int old_buffer_size_kb = 0; struct sigaction act; struct sigaction old_sigterm; struct sigaction old_sigint; @@ -165,7 +166,7 @@ old_uselib_enabled = -1; } - if (set_value (dfd, "buffer_size_kb", 128000, NULL) < 0) + if (set_value (dfd, "buffer_size_kb", 128000, &old_buffer_size_kb) < 0) goto error; if (set_value (dfd, "tracing_enabled", TRUE, &old_tracing_enabled) < 0) @@ -217,6 +218,9 @@ if (set_value (dfd, "events/fs/do_sys_open/enable", old_sys_open_enabled, NULL) < 0) goto error; + if (set_value (dfd, "buffer_size_kb", + old_buffer_size_kb, NULL) < 0) + goto error; /* Be nicer */ if (nice (15))