From: Liu Aleaxander <[email protected]>
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 <[email protected]>
---
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);
+ if (error < 0)
+ goto out;
+
+ /*
+ * If we needed to expand the fs array we
+ * might have blocked - try again.
+ */
+ if (error)
+ goto repeat;
+ }
+
if (start <= files->next_fd)
files->next_fd = fd + 1;
--
1.6.2.5
--
regards
Liu Aleaxander
On Wed, Nov 18, 2009 at 1:54 PM, Liu Aleaxander <[email protected]> wrote:
> From: Liu Aleaxander <[email protected]>
> 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 <[email protected]>
> ---
> 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.
> + if (error < 0)
> + goto out;
> +
> + /*
> + * If we needed to expand the fs array we
> + * might have blocked - try again.
> + */
> + if (error)
> + goto repeat;
> + }
> +
> if (start <= files->next_fd)
> files->next_fd = fd + 1;
>
> --
> 1.6.2.5
>
> --
> regards
> Liu Aleaxander
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Wed, Nov 18, 2009 at 3:17 PM, Am?rico Wang <[email protected]> wrote:
>
> On Wed, Nov 18, 2009 at 1:54 PM, Liu Aleaxander <[email protected]> wrote:
> > From: Liu Aleaxander <[email protected]>
> > 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 <[email protected]>
> > ---
> > ?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?
>
> > + ? ? ? ? ? ? ? if (error < 0)
> > + ? ? ? ? ? ? ? ? ? ? ? goto out;
> > +
> > + ? ? ? ? ? ? ? /*
> > + ? ? ? ? ? ? ? ?* If we needed to expand the fs array we
> > + ? ? ? ? ? ? ? ?* might have blocked - try again.
> > + ? ? ? ? ? ? ? ?*/
> > + ? ? ? ? ? ? ? if (error)
> > + ? ? ? ? ? ? ? ? ? ? ? goto repeat;
> > + ? ? ? }
> > +
> > ? ? ? ?if (start <= files->next_fd)
> > ? ? ? ? ? ? ? ?files->next_fd = fd + 1;
> >
> > --
> > 1.6.2.5
> >
> > --
> > regards
> > Liu Aleaxander
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at ?http://www.tux.org/lkml/
> >
--
regards
Liu Aleaxander
On Wed, Nov 18, 2009 at 3:41 PM, Liu Aleaxander <[email protected]> wrote:
> On Wed, Nov 18, 2009 at 3:17 PM, Américo Wang <[email protected]> wrote:
>>
>> On Wed, Nov 18, 2009 at 1:54 PM, Liu Aleaxander <[email protected]> wrote:
>> > From: Liu Aleaxander <[email protected]>
>> > 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 <[email protected]>
>> > ---
>> > 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, it's trivial, not so much an
improvement, IMO.
On Wed, Nov 18, 2009 at 4:35 PM, Am?rico Wang <[email protected]> wrote:
> On Wed, Nov 18, 2009 at 3:41 PM, Liu Aleaxander <[email protected]> wrote:
>> On Wed, Nov 18, 2009 at 3:17 PM, Am?rico Wang <[email protected]> wrote:
>>>
>>> On Wed, Nov 18, 2009 at 1:54 PM, Liu Aleaxander <[email protected]> wrote:
>>> > From: Liu Aleaxander <[email protected]>
>>> > 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 <[email protected]>
>>> > ---
>>> > ?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
On Wed, Nov 18, 2009 at 4:54 PM, Liu Aleaxander <[email protected]> wrote:
<snip>
>
>>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.
I am not rejecting it, I said this is trivial, so accepting it or droping
it both are OK for me.
I don't think the orignal code is ugly, '< fdt->max_fds' is not checked
for expand_files(), but for find_next_zero_bit().
On Wed, Nov 18, 2009 at 5:22 PM, Am?rico Wang <[email protected]> wrote:
> On Wed, Nov 18, 2009 at 4:54 PM, Liu Aleaxander <[email protected]> wrote:
>
> <snip>
>
>>
>>>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.
>
>
> I am not rejecting it, I said this is trivial, so accepting it or droping
> it both are OK for me.
>
> I don't think the orignal code is ugly,
I didn't say the old code is ugly either. ;)
>'< fdt->max_fds' is not checked for expand_files(), but for find_next_zero_bit().
According to the old code, it's true, but it can also be applied to
expand_files checking. And just like what I said, we did rarely need
expand the file table as usual; even though we need, one-time expand
will be enough for a long while as it doubles the original size. (Am I
right?).
--
regards
Liu Aleaxander