we check if initrd is initramfs first and then do real unpack. The check
isn't required, we can directly do unpack. If initrd isn't initramfs, we
can remove garbage. In my laptop, this saves 0.1s boot time. This
penalizes non-initramfs case, but now initramfs is mostly widely used.
Signed-off-by: Shaohua Li <[email protected]>
diff --git a/init/initramfs.c b/init/initramfs.c
index 644fc01..e51c92b 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -5,6 +5,7 @@
#include <linux/fcntl.h>
#include <linux/delay.h>
#include <linux/string.h>
+#include <linux/dirent.h>
#include <linux/syscalls.h>
static __initdata char *message;
@@ -520,6 +521,37 @@ skip:
initrd_end = 0;
}
+static void __init clean_rootfs(void)
+{
+ int fd = sys_open("/", O_RDONLY, 0);
+ void *buf = malloc(1024);
+ struct linux_dirent64 *dirp = buf;
+ int count;
+
+ memset(buf, 0, PAGE_SIZE);
+ count = sys_getdents64(fd, dirp, PAGE_SIZE);
+ while (count > 0) {
+ while (count > 0) {
+ struct stat st;
+
+ sys_newlstat(dirp->d_name, &st);
+ if (S_ISDIR(st.st_mode))
+ sys_rmdir(dirp->d_name);
+ else
+ sys_unlink(dirp->d_name);
+
+ count -= dirp->d_reclen;
+ dirp = (void *)dirp + dirp->d_reclen;
+ }
+ dirp = buf;
+ memset(buf, 0, 1024);
+ count = sys_getdents64(fd, dirp, PAGE_SIZE);
+ }
+
+ sys_close(fd);
+ free(buf);
+}
+
static int __init populate_rootfs(void)
{
char *err = unpack_to_rootfs(__initramfs_start,
@@ -531,13 +563,15 @@ static int __init populate_rootfs(void)
int fd;
printk(KERN_INFO "checking if image is initramfs...");
err = unpack_to_rootfs((char *)initrd_start,
- initrd_end - initrd_start, 1);
+ initrd_end - initrd_start, 0);
if (!err) {
printk(" it is\n");
- unpack_to_rootfs((char *)initrd_start,
- initrd_end - initrd_start, 0);
free_initrd();
return 0;
+ } else {
+ clean_rootfs();
+ unpack_to_rootfs(__initramfs_start,
+ __initramfs_end - __initramfs_start, 0);
}
printk("it isn't (%s); looks like an initrd\n", err);
fd = sys_open("/initrd.image", O_WRONLY|O_CREAT, 0700);
* Shaohua Li <[email protected]> wrote:
> we check if initrd is initramfs first and then do real unpack. The
> check isn't required, we can directly do unpack. If initrd isn't
> initramfs, we can remove garbage. In my laptop, this saves 0.1s boot
> time. This penalizes non-initramfs case, but now initramfs is mostly
> widely used.
clever concept!
a few observations about the cleanup function:
> +static void __init clean_rootfs(void)
> +{
> + int fd = sys_open("/", O_RDONLY, 0);
can this ever fail?
> + void *buf = malloc(1024);
no error checking for buf==NULL.
> + struct linux_dirent64 *dirp = buf;
> + int count;
> +
> + memset(buf, 0, PAGE_SIZE);
overflow: clearly allocating a 1024 bytes buffer and then clearing 4096
bytes isnt that good?
you could introduce a default-off CONFIG_DEBUG_ROOTFS_CLEANUP option
that does two runs of unpack_to_rootfs() and inserts an artificial
clean_rootfs() inbetween? Even if that debug patch doesnt get integrated
its a good test for the cleanup function.
> + count = sys_getdents64(fd, dirp, PAGE_SIZE);
... and then doing an up to 4096 bytes getdents into the buffer. A large
enough initramfs will overflow this.
> + while (count > 0) {
> + while (count > 0) {
> + struct stat st;
> +
> + sys_newlstat(dirp->d_name, &st);
can this ever fail? If yes we should at least WARN_ON_ONCE().
> + if (S_ISDIR(st.st_mode))
> + sys_rmdir(dirp->d_name);
> + else
> + sys_unlink(dirp->d_name);
> +
> + count -= dirp->d_reclen;
can this ever zero-underflow, with a sufficiently corrupted initramfs?
We should check for 0 underflow to be sure.
> + dirp = (void *)dirp + dirp->d_reclen;
likewise, we should size-overflow check this pointer. Failure modes of
overrunning the buffer are subtle and hard to notice/track down.
> + }
> + dirp = buf;
> + memset(buf, 0, 1024);
> + count = sys_getdents64(fd, dirp, PAGE_SIZE);
overflow: we do a 4096 bytes getdents into a 1K buffer.
> + }
> +
> + sys_close(fd);
> + free(buf);
> +}
> +
> static int __init populate_rootfs(void)
> {
> char *err = unpack_to_rootfs(__initramfs_start,
> @@ -531,13 +563,15 @@ static int __init populate_rootfs(void)
> int fd;
> printk(KERN_INFO "checking if image is initramfs...");
> err = unpack_to_rootfs((char *)initrd_start,
> - initrd_end - initrd_start, 1);
> + initrd_end - initrd_start, 0);
> if (!err) {
> printk(" it is\n");
> - unpack_to_rootfs((char *)initrd_start,
> - initrd_end - initrd_start, 0);
> free_initrd();
> return 0;
> + } else {
> + clean_rootfs();
> + unpack_to_rootfs(__initramfs_start,
> + __initramfs_end - __initramfs_start, 0);
> }
the dry_run variable is now unused in unpack_to_rootfs() and could be
eliminated.
> printk("it isn't (%s); looks like an initrd\n", err);
> fd = sys_open("/initrd.image", O_WRONLY|O_CREAT, 0700);
Ingo
* Ingo Molnar <[email protected]> wrote:
> the dry_run variable is now unused in unpack_to_rootfs() and could be
> eliminated.
also, while we are materially touching init/initramfs.c, that file has
collected a few uglies in the past few years, checkpatch --file says:
total: 7 errors, 7 warnings, 3 checks, 562 lines checked
it has a few other problems as well that can be seen if you look at the
file. Unused macros:
/* Diagnostic functions (stubbed out) */
#define Assert(cond,msg)
#define Trace(x)
#define Tracev(x)
#define Tracevv(x)
#define Tracec(c,x)
#define Tracecv(c,x)
#define STATIC static
#define INIT __init
lots of no-newline-after-variable-definitions instances:
{
int written;
dry_run = check_only;
no-newline-before-return:
kfree(header_buf);
return message;
}
so it would be nice to start off with a cleanup [strictly no code
changed] patch.
then it also has an absolutely crazy turn-error-printouts-off hack:
static __initdata char *message;
static void __init error(char *x)
{
if (!message)
message = x;
}
which is unobvious, 100% unused and should be removed.
those error()s should be pr_debug() perhaps. [this should be a second
cleanup patch as it changes the code]
Hm?
Ingo
>-----Original Message-----
>From: Ingo Molnar [mailto:[email protected]]
>Sent: Wednesday, August 13, 2008 3:45 PM
>To: Li, Shaohua
>Cc: lkml; Andrew Morton; Arjan van de Ven
>Subject: Re: [patch]fastboot: remove duplicate unpack_to_rootfs()
>
>
>* Shaohua Li <[email protected]> wrote:
>
>> we check if initrd is initramfs first and then do real unpack. The
>> check isn't required, we can directly do unpack. If initrd isn't
>> initramfs, we can remove garbage. In my laptop, this saves 0.1s boot
>> time. This penalizes non-initramfs case, but now initramfs is mostly
>> widely used.
>
>clever concept!
>
>a few observations about the cleanup function:
>
>> +static void __init clean_rootfs(void)
>> +{
>> + int fd = sys_open("/", O_RDONLY, 0);
>
>can this ever fail?
I thought there will be a panic if it fails, so I didn't explicitly add a check here. I can add one.
>> + struct linux_dirent64 *dirp = buf;
>> + int count;
>> +
>> + memset(buf, 0, PAGE_SIZE);
>
>overflow: clearly allocating a 1024 bytes buffer and then clearing 4096
>bytes isnt that good?
Oops, I use 4096 first and found it's too big, so changed to 1024, but forgot change all. My bad.
>you could introduce a default-off CONFIG_DEBUG_ROOTFS_CLEANUP option
>that does two runs of unpack_to_rootfs() and inserts an artificial
>clean_rootfs() inbetween? Even if that debug patch doesnt get integrated
>its a good test for the cleanup function.
Actually I did the test already, just forgot change all size.
>> + while (count > 0) {
>> + while (count > 0) {
>> + struct stat st;
>> +
>> + sys_newlstat(dirp->d_name, &st);
>
>can this ever fail? If yes we should at least WARN_ON_ONCE().
I'll add check.
>> + if (S_ISDIR(st.st_mode))
>> + sys_rmdir(dirp->d_name);
>> + else
>> + sys_unlink(dirp->d_name);
>> +
>> + count -= dirp->d_reclen;
>
>can this ever zero-underflow, with a sufficiently corrupted initramfs?
>We should check for 0 underflow to be sure.
>> + dirp = (void *)dirp + dirp->d_reclen;
>
>likewise, we should size-overflow check this pointer. Failure modes of
>overrunning the buffer are subtle and hard to notice/track down.
I'm not quite sure here. Do you think the .d_reclen can be a incorrect value?
>> static int __init populate_rootfs(void)
>> {
>> char *err = unpack_to_rootfs(__initramfs_start,
>> @@ -531,13 +563,15 @@ static int __init populate_rootfs(void)
>> int fd;
>> printk(KERN_INFO "checking if image is initramfs...");
>> err = unpack_to_rootfs((char *)initrd_start,
>> - initrd_end - initrd_start, 1);
>> + initrd_end - initrd_start, 0);
>> if (!err) {
>> printk(" it is\n");
>> - unpack_to_rootfs((char *)initrd_start,
>> - initrd_end - initrd_start, 0);
>> free_initrd();
>> return 0;
>> + } else {
>> + clean_rootfs();
>> + unpack_to_rootfs(__initramfs_start,
>> + __initramfs_end - __initramfs_start, 0);
>> }
>
>the dry_run variable is now unused in unpack_to_rootfs() and could be
>eliminated.
Ok, I can cleanup this.
Thanks,
Shaohua
On Wed, 13 Aug 2008 09:52:36 +0200 Ingo Molnar <[email protected]> wrote:
> no-newline-before-return:
>
> kfree(header_buf);
> return message;
> }
I accidentally delete those newlines when nobody is looking. They don't seem
worth the space they consume.
(what do we do with a function which has multiple `return's?)
2008/8/13, Ingo Molnar <[email protected]>:
>
> * Ingo Molnar <[email protected]> wrote:
>
> > the dry_run variable is now unused in unpack_to_rootfs() and could be
> > eliminated.
>
> also, while we are materially touching init/initramfs.c, that file has
> collected a few uglies in the past few years, checkpatch --file says:
>
> total: 7 errors, 7 warnings, 3 checks, 562 lines checked
>
> it has a few other problems as well that can be seen if you look at the
> file. Unused macros:
>
> /* Diagnostic functions (stubbed out) */
> #define Assert(cond,msg)
> #define Trace(x)
> #define Tracev(x)
> #define Tracevv(x)
> #define Tracec(c,x)
> #define Tracecv(c,x)
>
> #define STATIC static
> #define INIT __init
>
These are not really unused. A few lines later it reads:
#include "../src/inflate.c"
These macros are used within inflate.c
(and perhaps the inclusion of inflate.c is not a good idea, maybe this
should be in lib.a
note that inflate.c is also included in init/do_mounts_rd.c;
fortunately this is all init code (which is probably why include was
used in the first place))
Frans.
* Andrew Morton <[email protected]> wrote:
> On Wed, 13 Aug 2008 09:52:36 +0200 Ingo Molnar <[email protected]> wrote:
>
> > no-newline-before-return:
> >
> > kfree(header_buf);
> > return message;
> > }
>
> I accidentally delete those newlines when nobody is looking. They
> don't seem worth the space they consume.
yeah - for me it's case-dependent. My benchmark for it is absolutely
objective and easy to describe: i add a newline when it looks nicer and
more maintainable that way ;-)
> (what do we do with a function which has multiple `return's?)
i really didnt want to make a full scale style discussion out of this.
Lets ignore my suggestion. The valid case when i use a newline is for
example when the return obscures what happens:
if (something) {
do_one();
repeat_this();
return;
}
as visually it's easy to miss the return - especially if the lines above
it look similar. So i use:
if (something) {
do_one();
repeat_this();
return;
}
because way too often do i miss a stray return somewhere and
misunderstand the code flow of a function if it does not stand out, even
with syntax highlighting.
Another case is when there's a long linear block of cleanup statements
followed by a return:
q->mode = mode;
strcpy(q->name, name);
q->next = NULL;
*p = q;
return NULL;
}
i usually add a newline:
q->mode = mode;
strcpy(q->name, name);
q->next = NULL;
*p = q;
return NULL;
}
as the 'return NULL' is a separate concept from the preceding
activities.
So in this case it is not really because the return is specialy, this is
because i like to separate groups of statements per type of activity. So
i'd do the same if there were two groups of statements, i'd turn this:
q->mode = mode;
strcpy(q->name, name);
q->next = NULL;
*p = q;
other_stuff = 2;
some_other_stuff(other_stuff)
into this:
q->mode = mode;
strcpy(q->name, name);
q->next = NULL;
*p = q;
other_stuff = 2;
some_other_stuff(other_stuff)
to make sure the two groups of statements stand out. (Sometimes a pure
newline does a better job at inserting the right kind of visual
structure than a comment line.)
but again ... these are nuances where reasonable people might disagree,
and i only made them because this topic lives, is developed and tested
in tip/fastboot at the moment.
Ingo
* Frans Meulenbroeks <[email protected]> wrote:
> 2008/8/13, Ingo Molnar <[email protected]>:
> >
> > * Ingo Molnar <[email protected]> wrote:
> >
> > > the dry_run variable is now unused in unpack_to_rootfs() and could be
> > > eliminated.
> >
> > also, while we are materially touching init/initramfs.c, that file has
> > collected a few uglies in the past few years, checkpatch --file says:
> >
> > total: 7 errors, 7 warnings, 3 checks, 562 lines checked
> >
> > it has a few other problems as well that can be seen if you look at the
> > file. Unused macros:
> >
> > /* Diagnostic functions (stubbed out) */
> > #define Assert(cond,msg)
> > #define Trace(x)
> > #define Tracev(x)
> > #define Tracevv(x)
> > #define Tracec(c,x)
> > #define Tracecv(c,x)
> >
> > #define STATIC static
> > #define INIT __init
> >
> These are not really unused. A few lines later it reads:
>
> #include "../src/inflate.c"
>
> These macros are used within inflate.c
oh, i _knew_ i saw this zlib ugliness sometime in the past.
> (and perhaps the inclusion of inflate.c is not a good idea, maybe this
> should be in lib.a note that inflate.c is also included in
> init/do_mounts_rd.c; fortunately this is all init code (which is
> probably why include was used in the first place))
definitely. It's a different patch though.
Ingo
>-----Original Message-----
>From: Ingo Molnar [mailto:[email protected]]
>Sent: Wednesday, August 13, 2008 3:45 PM
>To: Li, Shaohua
>Cc: lkml; Andrew Morton; Arjan van de Ven
>Subject: Re: [patch]fastboot: remove duplicate unpack_to_rootfs()
>
>
>* Shaohua Li <[email protected]> wrote:
>
>> we check if initrd is initramfs first and then do real unpack. The
>> check isn't required, we can directly do unpack. If initrd isn't
>> initramfs, we can remove garbage. In my laptop, this saves 0.1s boot
>> time. This penalizes non-initramfs case, but now initramfs is mostly
>> widely used.
Updated patch. Sorry for the attachment, my email client is broken.
Thanks,
Shaohua