Re: [PATCH v2] MTD: Retry Read/Write Transfer Buffer Allocations

[Grant Erickson <marathon96@gmail.com>]

Artem:

Thanks for the quick turnaround in feedback. Please see inline below.

On 4/4/11 9:39 PM, Artem Bityutskiy wrote:
> On Mon, 2011-04-04 at 11:19 -0700, Grant Erickson wrote:
>> + * is highly fragmented at the cost of reducing the performance of the
>> + * requested transfer due to a smaller buffer size.
>> + *
>> + * A more complex but more memory-efficient implementation based on
>> + * get_user_pages and iovecs to cover extents of those pages is a
>> + * longer-term goal, as intimated by Linus above. However, for the
>> + * write case, this requires yet more complex head and tail transfer
>> + * handling when those head and tail offsets and sizes are such that
>> + * alignment requirements are not met in the NAND subdriver.
>> + */
>>  #define MAX_KMALLOC_SIZE 0x20000
>>  
>> +static void *mtd_try_alloc(size_t *size)
>> +{
>> + const gfp_t flags = (GFP_KERNEL | __GFP_NOWARN);
> 
> I still think you'll damage the performance when you try to do
> 
> kmalloc(128KiB, flags)
> 
> because as I wrote in my previous e-mail your system will start doing
> the following to free memory for you:
> 
> 1. write-back dirty FS data = overall slowdown = e.g., background mp3
>    playback glitches
> 2. drop FS caches = slow down later because the system will have to
>    re-read the dropped data from the media later.
> 3. not really sure, needs checking if this is the case, but I think
>    the kernel may start swapping out apps.
> 
> This is why I suggested to use the following flags here:
> 
> gfp_t flags = __GFP_NOWARN | __GFP_WAIT | __GFP_NORETRY;

On my system (64 MiB RAM, 256 MiB Flash), there is no swap and under these
allocation conditions for jffs2_scan_medium, mtd_read or mtd_write, I don't
see the kernel doing (1), (2) or (3).

My impression is that the above behaviors are only activated when a swap
store exists and, in general, most systems using JFFS2 and MTD do not have
swap.

Regardless, adding the additional flags should not be detrimental for
systems with no swap and, it sounds like, helpful for systems with it.

Regarding the suggestion of mtd_alloc_upto() or mtd_alloc_as_much(), are you
OK exporting these from mtdchar.c or would you rather they be moved to and
exported from mtdcore.c?

Stay tuned for v3. 

-Grant