Re: [PATCH] Retry Large Buffer Allocations

[Artem Bityutskiy <dedekind1@gmail.com>]

Just partial review before I go to bed, to keep you busy :-)))

On Tue, 2011-04-05 at 11:34 -0700, Grant Erickson wrote:
> 			kfree(kbuf);
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index da69bc8..d102b96 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -638,6 +638,36 @@ int default_mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
>  	return ret;
>  }
>  
> +/* mtd_alloc_up_to - called, for example by mtd_{read,write} and
> + *			jffs2_scan_medium, to handle smaller (i.e. degraded) buffer
> + *			allocations under low- or fragmented-memory situations where
> + *			such reduced allocations, from a requested ideal, are allowed.
> + */

Please, make the comment to be of proper style. See the CodingStyle
file. Or even better - make it proper kerneldoc comment, if you can. I'm
sure you'll find the rules in the "Documentation" directory, but this is
optional. Also, please, put the comment just above the function. 
> +#define MAX_KMALLOC_SIZE 0x20000
> +
> +void *mtd_alloc_up_to(size_t *size)
> +{
> +	gfp_t flags;
> +	size_t try;
> +	void *kbuf;
> +
> +	try = min_t(size_t, *size, MAX_KMALLOC_SIZE);
> +
> +	do {
> +		flags = ((size == PAGE_SIZE) ?

s/size/try/

> +			 GFP_KERNEL :
> +			 (__GFP_NOWARN | __GFP_WAIT |
> +			   __GFP_NORETRY | __GFP_NO_KSWAPD));

Sorry, but this construct is not beautiful and difficult to read. I'd
call it abuse of ?: operators :-) I think it is much more readable to
simply:

1. initialize gfp_t flags to (__GFP_NOWARN .... ) when you initialize it
or just before the loop.
2. Do
	if (try == PAGE_SIZE)
		flags = GFP_KERNEL;

Simple and readable.

And do not forget to check the code with checkpatch.pl before sending
out :-)

Good night!

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