Patchwork ureadahead patch

login
register
mail settings
Submitter Tim Gardner
Date July 8, 2010, 4:24 p.m.
Message ID <20100708162442.ADD40F891E@sepang.rtg.net>
Download mbox | patch
Permalink /patch/58256/
State Superseded
Headers show

Comments

Tim Gardner - July 8, 2010, 4:24 p.m.

Chase Douglas - July 8, 2010, 7:46 p.m.
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
Bryan Wu - July 9, 2010, 1:15 a.m.
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))
>
Tim Gardner - July 9, 2010, 1:19 a.m.
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
Chase Douglas - July 9, 2010, 1:31 a.m.
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
Tim Gardner - July 9, 2010, 1:49 a.m.
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
Bryan Wu - July 9, 2010, 2:05 a.m.
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,
Tim Gardner - July 9, 2010, 2:18 a.m.
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 
?
Bryan Wu - July 9, 2010, 3:01 a.m.
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
Tim Gardner - July 9, 2010, 3:07 a.m.
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
Oliver Grawert - July 9, 2010, 9:48 a.m.
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

Patch

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))