[PATCH] mtd: Use fallback in memory allocation for mtd_{read,write}

[PATCH] mtd: Use fallback in memory allocation for mtd_{read,write}

[Jarkko Lavinen]

Kmalloc used in mtd_read() and mtd_write() can fail if the request
size is large and memory is fragmented.  Use fall-back mechanism which
will quietly retry the allocation by halving the allocation size in each
retry.

Signed-off-by: Jarkko Lavinen <jarkko.lavinen@nokia.com>
---
 drivers/mtd/mtdchar.c |   44 ++++++++++++++++++++++++++++++--------------
 1 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 145b3d0d..18263d8 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -171,12 +171,33 @@ static int mtd_close(struct inode *inode, struct file *file)
 */
 #define MAX_KMALLOC_SIZE 0x20000
 
+static void *mtd_buf_alloc(size_t *size)
+{
+	void *kbuf;
+	size_t next;
+
+	if (*size > MAX_KMALLOC_SIZE)
+		*size = MAX_KMALLOC_SIZE;
+
+	kbuf = kmalloc(*size, GFP_KERNEL | __GFP_NOWARN);
+	next = 1 << (fls(*size - 1) - 1);
+
+	while (!kbuf && next >= PAGE_SIZE) {
+		*size = next;
+		next /= 2;
+		kbuf = kmalloc(*size, GFP_KERNEL | __GFP_NOWARN);
+	}
+
+	return kbuf;
+}
+
 static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t *ppos)
 {
 	struct mtd_file_info *mfi = file->private_data;
 	struct mtd_info *mtd = mfi->mtd;
 	size_t retlen=0;
 	size_t total_retlen=0;
+	size_t alloc;
 	int ret=0;
 	int len;
 	char *kbuf;
@@ -192,18 +213,14 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
 	/* FIXME: Use kiovec in 2.5 to lock down the user's buffers
 	   and pass them directly to the MTD functions */
 
-	if (count > MAX_KMALLOC_SIZE)
-		kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
-	else
-		kbuf=kmalloc(count, GFP_KERNEL);
-
+	alloc = count;
+	kbuf = mtd_buf_alloc(&alloc);
 	if (!kbuf)
 		return -ENOMEM;
 
 	while (count) {
-
-		if (count > MAX_KMALLOC_SIZE)
-			len = MAX_KMALLOC_SIZE;
+		if (count > alloc)
+			len = alloc;
 		else
 			len = count;
 
@@ -271,6 +288,7 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 	char *kbuf;
 	size_t retlen;
 	size_t total_retlen=0;
+	size_t alloc;
 	int ret=0;
 	int len;
 
@@ -285,18 +303,16 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 	if (!count)
 		return 0;
 
-	if (count > MAX_KMALLOC_SIZE)
-		kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
-	else
-		kbuf=kmalloc(count, GFP_KERNEL);
+	alloc = count;
+	kbuf = mtd_buf_alloc(&alloc);
 
 	if (!kbuf)
 		return -ENOMEM;
 
 	while (count) {
 
-		if (count > MAX_KMALLOC_SIZE)
-			len = MAX_KMALLOC_SIZE;
+		if (count > alloc)
+			len = alloc;
 		else
 			len = count;
 
-- 
1.7.2.5

Re: [PATCH] mtd: Use fallback in memory allocation for mtd_{read,write}

[Artem Bityutskiy]

On Tue, 2011-03-22 at 14:13 +0200, Jarkko Lavinen wrote:
> Kmalloc used in mtd_read() and mtd_write() can fail if the request
> size is large and memory is fragmented.  Use fall-back mechanism which
> will quietly retry the allocation by halving the allocation size in each
> retry.
> 
> Signed-off-by: Jarkko Lavinen <jarkko.lavinen@nokia.com>
> ---
>  drivers/mtd/mtdchar.c |   44 ++++++++++++++++++++++++++++++--------------
>  1 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 145b3d0d..18263d8 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -171,12 +171,33 @@ static int mtd_close(struct inode *inode, struct file *file)
>  */
>  #define MAX_KMALLOC_SIZE 0x20000
>  
> +static void *mtd_buf_alloc(size_t *size)
> +{
> +	void *kbuf;
> +	size_t next;
> +
> +	if (*size > MAX_KMALLOC_SIZE)
> +		*size = MAX_KMALLOC_SIZE;
> +
> +	kbuf = kmalloc(*size, GFP_KERNEL | __GFP_NOWARN);
> +	next = 1 << (fls(*size - 1) - 1);
> +
> +	while (!kbuf && next >= PAGE_SIZE) {
> +		*size = next;
> +		next /= 2;
> +		kbuf = kmalloc(*size, GFP_KERNEL | __GFP_NOWARN);
> +	}
> +
> +	return kbuf;
> +}

Instead of improving bad code we need re-work it and allocate multiple
small buffers and use vector-based read/write functions.

Indeed, kmalloc()'ing large buffers is a horrible things to do for
performance because it causes a lot of activities, write-back, kills
caches, etc. This patch tries to kmalloc a lot, then less, even less,
etc, until it succeeds. And the first kmalloc will put the device on the
knees.

So we should not even try to kmalloc that much. And also, we should use
GFP_NOIO flag when kmalloc-ing a lot, I think.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)