linux-kernel - Re: [PATCH] vfs: does call expand

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 18 Nov 2009 16:54:04 +0800
From:	Liu Aleaxander <aleaxander@...il.com>
To:	Américo Wang <xiyou.wangcong@...il.com>
Cc:	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] vfs: does call expand_files when needed

On Wed, Nov 18, 2009 at 4:35 PM, Américo Wang <xiyou.wangcong@...il.com> wrote:
> On Wed, Nov 18, 2009 at 3:41 PM, Liu Aleaxander <aleaxander@...il.com> wrote:
>> On Wed, Nov 18, 2009 at 3:17 PM, Américo Wang <xiyou.wangcong@...il.com> wrote:
>>>
>>> On Wed, Nov 18, 2009 at 1:54 PM, Liu Aleaxander <aleaxander@...il.com> wrote:
>>> > From: Liu Aleaxander <Aleaxander@...il.com>
>>> > Date: Wed, 18 Nov 2009 10:59:09 +0800
>>> > Subject: [PATCH] vfs: does call expand_files when needed
>>> >
>>> > I don't think we should call expand_files every time we open a
>>> > file for a new unused fd, so does the expand when necessary.
>>> >
>>> > Signed-off-by: Liu Aleaxander <Aleaxander@...il.com>
>>> > ---
>>> >  fs/file.c |   27 ++++++++++++++-------------
>>> >  1 files changed, 14 insertions(+), 13 deletions(-)
>>> >
>>> > diff --git a/fs/file.c b/fs/file.c
>>> > index 87e1290..3f3d0fc 100644
>>> > --- a/fs/file.c
>>> > +++ b/fs/file.c
>>> > @@ -452,22 +452,22 @@ repeat:
>>> >        if (fd < files->next_fd)
>>> >                fd = files->next_fd;
>>> >
>>> > -       if (fd < fdt->max_fds)
>>> > +       if (likely(fd < fdt->max_fds)) {
>>> >                fd = find_next_zero_bit(fdt->open_fds->fds_bits,
>>> >                                           fdt->max_fds, fd);
>>> > -
>>> > -       error = expand_files(files, fd);
>>> > -       if (error < 0)
>>> > -               goto out;
>>> > -
>>> > -       /*
>>> > -        * If we needed to expand the fs array we
>>> > -        * might have blocked - try again.
>>> > -        */
>>> > -       if (error)
>>> > -               goto repeat;
>>> > -
>>> > +       } else {
>>> > +               error = expand_files(files, fd);
>>>
>>>
>>> In expand_files(), it has the check for
>>> ' < fdt->max_fds', so this change is not necessary.
>>
>> Yeah, indeed. But why we should go into an another function to do a
>> _double_ check especially we mostly don't need to do that?
>
> You only optimized one call path,
Yes, and that's the intent of this patch.

>it's trivial, not so much an improvement, IMO.
So, shouldn't we do the optimize when there is a way to do that?

While, I don't think so. And BTW, it's not just a problem of
optimization, but also make it be more sense: JUST call expand when
need. I don't know why you are rejecting about this, especially it did
optimized one call path(as you said), and it doesn't make the code
uglier than before but making it be more sense, and, in fact, a kind
of more readable.

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