[patch]fastboot: remove duplicate unpack_to_rootfs() - Kernel

This is a discussion on [patch]fastboot: remove duplicate unpack_to_rootfs() - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 8 of 8

Thread: [patch]fastboot: remove duplicate unpack_to_rootfs()

  1. [patch]fastboot: remove duplicate unpack_to_rootfs()

    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

    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
    #include
    #include
    +#include
    #include

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


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [patch]fastboot: remove duplicate unpack_to_rootfs()


    * Shaohua Li 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [patch]fastboot: remove duplicate unpack_to_rootfs()


    * Ingo Molnar 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. RE: [patch]fastboot: remove duplicate unpack_to_rootfs()



    >-----Original Message-----
    >From: Ingo Molnar [mailto:mingo@elte.hu]
    >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 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [patch]fastboot: remove duplicate unpack_to_rootfs()

    On Wed, 13 Aug 2008 09:52:36 +0200 Ingo Molnar 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?)
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [patch]fastboot: remove duplicate unpack_to_rootfs()

    2008/8/13, Ingo Molnar :
    >
    > * Ingo Molnar 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.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [patch]fastboot: remove duplicate unpack_to_rootfs()


    * Andrew Morton wrote:

    > On Wed, 13 Aug 2008 09:52:36 +0200 Ingo Molnar 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [patch]fastboot: remove duplicate unpack_to_rootfs()


    * Frans Meulenbroeks wrote:

    > 2008/8/13, Ingo Molnar :
    > >
    > > * Ingo Molnar 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread