From: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
To: akpm@linux-foundation.org
Cc: andi@firstfloor.org, konishi.ryusuke@lab.ntt.co.jp,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH mmotm 1/5] nilfs2: fix problems of memory allocation in ioctl
Date: Sun, 14 Dec 2008 21:08:39 +0900 (JST) [thread overview]
Message-ID: <20081214.210839.09550698.ryusuke@osrg.net> (raw)
In-Reply-To: <20081213.172915.31069043.ryusuke@osrg.net>
This is the revised patch for fixing the following problems of a
memory copy function in nilfs2 ioctl.
(1) It tries to allocate 128KB size of memory even for small objects.
(2) Though the function repeatedly tries large memory allocations
while reducing the size, GFP_NOWAIT flag is not specified.
This increases the possibility of system memory shortage.
(3) During the retries of (2), verbose warnings are printed
because _GFP_NOWARN flag is not used for the kmalloc calls.
The first patch was still doing large allocations by kmalloc which are
repeatedly tried while reducing the size.
Andi Kleen has pointed out that just using vmalloc would be
easy/faster/more reliable, and he also told me that using
copy_from_user for large memory is not good from the viewpoint of
preempt latency:
On Fri, 12 Dec 2008 21:24:11 +0100, Andi Kleen <andi@firstfloor.org> wrote:
> > In the current interface, each data item is copied twice: one is to
> > the allocated memory from user space (via copy_from_user), and another
>
> For such large copies it is better to use multiple smaller (e.g. 4K)
> copy user, that gives better real time preempt latencies. Each cfu has a
> cond_resched(), but only one, not multiple times in the inner loop.
For the function in question, the size of buffer memory can be reduced
since the buffer is repeatedly used for a number of small objects. On
the other hand, it may incur large preempt latencies for larger buffer
because a copy_from_user (and a copy_to_user) was applied only once
each cycle.
So, this revision avoids the latency issue as well as fixes the
original problems merely by reducing allocation size of the buffer.
Cc: Andi Kleen <andi@firstfloor.org>
Signed-off-by: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
---
fs/nilfs2/ioctl.c | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
index 35ba60e..23378c3 100644
--- a/fs/nilfs2/ioctl.c
+++ b/fs/nilfs2/ioctl.c
@@ -34,8 +34,7 @@
#include "dat.h"
-#define KMALLOC_SIZE_MIN 4096 /* 4KB */
-#define KMALLOC_SIZE_MAX 131072 /* 128 KB */
+#define NILFS_IOCTL_KMALLOC_SIZE 8192 /* 8KB */
static int nilfs_ioctl_wrap_copy(struct the_nilfs *nilfs,
struct nilfs_argv *argv, int dir,
@@ -51,12 +50,9 @@ static int nilfs_ioctl_wrap_copy(struct the_nilfs *nilfs,
if (argv->v_nmembs == 0)
return 0;
- for (ksize = KMALLOC_SIZE_MAX; ksize >= KMALLOC_SIZE_MIN; ksize /= 2) {
- buf = kmalloc(ksize, GFP_NOFS);
- if (buf != NULL)
- break;
- }
- if (ksize < KMALLOC_SIZE_MIN)
+ ksize = max_t(size_t, NILFS_IOCTL_KMALLOC_SIZE, argv->v_size);
+ buf = kmalloc(ksize, GFP_NOFS);
+ if (unlikely(!buf))
return -ENOMEM;
maxmembs = ksize / argv->v_size;
--
1.5.6.5
next prev parent reply other threads:[~2008-12-14 12:09 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-12-12 5:16 [PATCH mmotm 0/5] nilfs2 filesystem updates Ryusuke Konishi
2008-12-12 5:16 ` [PATCH mmotm 1/5] nilfs2: fix problems of memory allocation in ioctl Ryusuke Konishi
2008-12-12 5:16 ` [PATCH mmotm 2/5] nilfs2: cleanup nilfs_clear_inode Ryusuke Konishi
2008-12-12 5:16 ` [PATCH mmotm 3/5] nilfs2: avoid double error caused by nilfs_transaction_end Ryusuke Konishi
2008-12-12 5:17 ` [PATCH mmotm 4/5] nilfs2: insert explanations in gcinode file Ryusuke Konishi
2008-12-12 5:17 ` [PATCH mmotm 5/5] nilfs2: add maintainer Ryusuke Konishi
2008-12-12 7:50 ` [PATCH mmotm 3/5] nilfs2: avoid double error caused by nilfs_transaction_end Pekka Enberg
2008-12-12 13:47 ` [PATCH mmotm 1/5] nilfs2: fix problems of memory allocation in ioctl Andi Kleen
2008-12-12 18:48 ` Ryusuke Konishi
2008-12-12 20:24 ` Andi Kleen
2008-12-13 8:29 ` Ryusuke Konishi
2008-12-14 12:08 ` Ryusuke Konishi [this message]
2008-12-14 15:13 ` Andi Kleen
2008-12-15 6:58 ` Ryusuke Konishi
2008-12-15 10:17 ` Andi Kleen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20081214.210839.09550698.ryusuke@osrg.net \
--to=konishi.ryusuke@lab.ntt.co.jp \
--cc=akpm@linux-foundation.org \
--cc=andi@firstfloor.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.