All of lore.kernel.org
 help / color / mirror / Atom feed
From: Grant Erickson <marathon96@gmail.com>
To: Artem Bityutskiy <dedekind1@gmail.com>
Cc: Jarkko Lavinen <jarkko.lavinen@nokia.com>,
	linux-mtd@lists.infradead.org,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] MTD: Retry Read/Write Transfer Buffer Allocations
Date: Mon, 04 Apr 2011 09:05:16 -0700	[thread overview]
Message-ID: <C9BF3A4C.27686%marathon96@gmail.com> (raw)
In-Reply-To: <1301902050.2760.23.camel@localhost>

On 4/4/11 12:27 AM, Artem Bityutskiy wrote:
> [CCing LKML in a hope to get good suggestions]
> [The patch: 
> http://lists.infradead.org/pipermail/linux-mtd/2011-April/034645.html]
> 
> Hi Grant,
> 
> Just in case, Jarkko was trying to address the same issue recently:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2011-March/034416.html
> 
> This should be a bit more complex I think. First of all, I think it is
> better to make this a separate function. Second, you should make sure
> the system does not print scary warnings when the allocation fails - use
> __GFP_NOWARN flag, just like Jarkko did.
> 
> An third, as I wrote in my answer to Jarkko, allocating large contiguous
> buffers is bad for performance: if the system memory is fragmented and
> there is no such large contiguous areas, the kernel will start writing
> back dirty FS data, killing FS caches, shrinking caches and buggers,
> probably even swapping out applications. We do not want MTD to cause
> this at all.
> 
> Probably we can mitigate this with kmalloc flags. Now, I'm not sure what
> flags are the optimal, but I'd do:
> 
> __GFP_NOWARN | __GFP_WAIT | __GFP_NORETRY
> 
> May be even __GFP_WAIT flag could be kicked out.

Artem:

Thanks for the feedback and the link to Jarkko's very similar patch. Your
suggestions will be incorporated into a subsequent patch.

For reference, I pursued a second uses-less-memory-but-is-more-complex
approach that does get_user_pages, builds up a series of iovecs for the page
extents. This worked well for all read cases I could test; however, for the
write case, the approach required yet more refinement and overhead since the
head and tail of the transfer need to be deblocked with read-modify-write
due to the NOTALIGNED checks in nand_base.c:nand_do_write_ops. I am happy to
share the work-in-progress with the list if anyone is interested.

I propose a two-stage approach. This issue has been in the kernel for about
six years. 

Can we take a modified version of Jarkko's or my simpler fixes for the first
pass and then iterate toward the get_user_pages scatter/gather approach
later?

Best,

Grant

WARNING: multiple messages have this Message-ID (diff)
From: Grant Erickson <marathon96@gmail.com>
To: Artem Bityutskiy <dedekind1@gmail.com>
Cc: <linux-mtd@lists.infradead.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Jarkko Lavinen <jarkko.lavinen@nokia.com>
Subject: Re: [PATCH] MTD: Retry Read/Write Transfer Buffer Allocations
Date: Mon, 04 Apr 2011 09:05:16 -0700	[thread overview]
Message-ID: <C9BF3A4C.27686%marathon96@gmail.com> (raw)
In-Reply-To: <1301902050.2760.23.camel@localhost>

On 4/4/11 12:27 AM, Artem Bityutskiy wrote:
> [CCing LKML in a hope to get good suggestions]
> [The patch: 
> http://lists.infradead.org/pipermail/linux-mtd/2011-April/034645.html]
> 
> Hi Grant,
> 
> Just in case, Jarkko was trying to address the same issue recently:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2011-March/034416.html
> 
> This should be a bit more complex I think. First of all, I think it is
> better to make this a separate function. Second, you should make sure
> the system does not print scary warnings when the allocation fails - use
> __GFP_NOWARN flag, just like Jarkko did.
> 
> An third, as I wrote in my answer to Jarkko, allocating large contiguous
> buffers is bad for performance: if the system memory is fragmented and
> there is no such large contiguous areas, the kernel will start writing
> back dirty FS data, killing FS caches, shrinking caches and buggers,
> probably even swapping out applications. We do not want MTD to cause
> this at all.
> 
> Probably we can mitigate this with kmalloc flags. Now, I'm not sure what
> flags are the optimal, but I'd do:
> 
> __GFP_NOWARN | __GFP_WAIT | __GFP_NORETRY
> 
> May be even __GFP_WAIT flag could be kicked out.

Artem:

Thanks for the feedback and the link to Jarkko's very similar patch. Your
suggestions will be incorporated into a subsequent patch.

For reference, I pursued a second uses-less-memory-but-is-more-complex
approach that does get_user_pages, builds up a series of iovecs for the page
extents. This worked well for all read cases I could test; however, for the
write case, the approach required yet more refinement and overhead since the
head and tail of the transfer need to be deblocked with read-modify-write
due to the NOTALIGNED checks in nand_base.c:nand_do_write_ops. I am happy to
share the work-in-progress with the list if anyone is interested.

I propose a two-stage approach. This issue has been in the kernel for about
six years. 

Can we take a modified version of Jarkko's or my simpler fixes for the first
pass and then iterate toward the get_user_pages scatter/gather approach
later?

Best,

Grant



  parent reply	other threads:[~2011-04-04 16:05 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-02  0:44 [PATCH] MTD: Retry Read/Write Transfer Buffer Allocations Grant Erickson
2011-04-04  7:27 ` Artem Bityutskiy
2011-04-04  7:41   ` Artem Bityutskiy
2011-04-04 16:05   ` Grant Erickson [this message]
2011-04-04 16:05     ` Grant Erickson
2011-04-05  4:39     ` Artem Bityutskiy
2011-04-05  4:39       ` Artem Bityutskiy
2011-04-04 18:19 ` [PATCH v2] " Grant Erickson
2011-04-05  4:39   ` Artem Bityutskiy
2011-04-05 15:54     ` Grant Erickson
2011-04-05 16:54       ` Artem Bityutskiy
2011-04-05  4:48   ` Artem Bityutskiy
  -- strict thread matches above, loose matches on Subject: below --
2011-04-02  1:29 [PATCH] JFFS2: Retry Medium Scan " Grant Erickson
2011-04-04 18:39 ` [PATCH] MTD: Retry Read/Write Transfer " Grant Erickson

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=C9BF3A4C.27686%marathon96@gmail.com \
    --to=marathon96@gmail.com \
    --cc=dedekind1@gmail.com \
    --cc=jarkko.lavinen@nokia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.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.