Patchwork [1/6] mtd: nandsim: convert pages_written[] to bitmap

login
register
mail settings
Submitter Akinobu Mita
Date July 28, 2013, 2:21 a.m.
Message ID <1374978118-11418-1-git-send-email-akinobu.mita@gmail.com>
Download mbox | patch
Permalink /patch/262548/
State Accepted
Commit 08efe91a1befa67bcc3799e0d29b7194803e1538
Headers show

Comments

Akinobu Mita - July 28, 2013, 2:21 a.m.
nandsim.pages_written[] is the array of unsigned char which is indexed
by the page number and used for identifying which pages have been written
when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
so it can be converted to bitmap.  This reduces the allocation size of
pages_written[] by 1/8.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/nand/nandsim.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
Artem Bityutskiy - Aug. 2, 2013, 2:38 p.m.
On Sun, 2013-07-28 at 11:21 +0900, Akinobu Mita wrote:
> nandsim.pages_written[] is the array of unsigned char which is indexed
> by the page number and used for identifying which pages have been written
> when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
> so it can be converted to bitmap.  This reduces the allocation size of
> pages_written[] by 1/8.

Did you test this patch-set, if yes, how?
Artem Bityutskiy - Aug. 2, 2013, 3:46 p.m.
On Fri, 2013-08-02 at 17:38 +0300, Artem Bityutskiy wrote:
> On Sun, 2013-07-28 at 11:21 +0900, Akinobu Mita wrote:
> > nandsim.pages_written[] is the array of unsigned char which is indexed
> > by the page number and used for identifying which pages have been written
> > when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
> > so it can be converted to bitmap.  This reduces the allocation size of
> > pages_written[] by 1/8.
> 
> Did you test this patch-set, if yes, how?

Note, I am asking because this is an ancient and probably poor piece of
code, but quite important, so I am just trying to understand whether I
should test your patches before applying, and if yes, how.
Akinobu Mita - Aug. 2, 2013, 3:59 p.m.
2013/8/3 Artem Bityutskiy <dedekind1@gmail.com>:
> On Fri, 2013-08-02 at 17:38 +0300, Artem Bityutskiy wrote:
>> On Sun, 2013-07-28 at 11:21 +0900, Akinobu Mita wrote:
>> > nandsim.pages_written[] is the array of unsigned char which is indexed
>> > by the page number and used for identifying which pages have been written
>> > when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
>> > so it can be converted to bitmap.  This reduces the allocation size of
>> > pages_written[] by 1/8.
>>
>> Did you test this patch-set, if yes, how?
>
> Note, I am asking because this is an ancient and probably poor piece of
> code, but quite important, so I am just trying to understand whether I
> should test your patches before applying, and if yes, how.

I tested this with mtd/tests modules and mounting a filesystem through
mtdblock.  I also tried cache_file module parameter to test patch 1/6
because nandsim.pages_written[] is only used when cache_file is used.
Also I tried second_id_byte module parameter to check both 8-bit and
16-bit bus width are working.
Artem Bityutskiy - Aug. 5, 2013, 1:02 p.m.
On Sun, 2013-07-28 at 11:21 +0900, Akinobu Mita wrote:
> nandsim.pages_written[] is the array of unsigned char which is indexed
> by the page number and used for identifying which pages have been written
> when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
> so it can be converted to bitmap.  This reduces the allocation size of
> pages_written[] by 1/8.

Pushed all to l2-mtd.git, thanks.

I wonder, what is your motivation for doing this? Did you just notice
the imperfection and decided to fix it, or you really suffer from too
much memory allocated?

Thanks!
Akinobu Mita - Aug. 5, 2013, 11:02 p.m.
2013/8/5 Artem Bityutskiy <dedekind1@gmail.com>:
> On Sun, 2013-07-28 at 11:21 +0900, Akinobu Mita wrote:
>> nandsim.pages_written[] is the array of unsigned char which is indexed
>> by the page number and used for identifying which pages have been written
>> when cache_file is used.  Each entry holds 0 (not written) or 1 (written),
>> so it can be converted to bitmap.  This reduces the allocation size of
>> pages_written[] by 1/8.
>
> Pushed all to l2-mtd.git, thanks.

Thanks.

> I wonder, what is your motivation for doing this? Did you just notice
> the imperfection and decided to fix it, or you really suffer from too
> much memory allocated?

I found all in this patch set when I was reading nandsim.c.  So I did
not suffer any real problems.

Patch

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index cb38f3d..a73dce0 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -363,7 +363,7 @@  struct nandsim {
 
 	/* Fields needed when using a cache file */
 	struct file *cfile; /* Open file */
-	unsigned char *pages_written; /* Which pages have been written */
+	unsigned long *pages_written; /* Which pages have been written */
 	void *file_buf;
 	struct page *held_pages[NS_MAX_HELD_PAGES];
 	int held_cnt;
@@ -586,7 +586,8 @@  static int alloc_device(struct nandsim *ns)
 			err = -EINVAL;
 			goto err_close;
 		}
-		ns->pages_written = vzalloc(ns->geom.pgnum);
+		ns->pages_written = vzalloc(BITS_TO_LONGS(ns->geom.pgnum) *
+					    sizeof(unsigned long));
 		if (!ns->pages_written) {
 			NS_ERR("alloc_device: unable to allocate pages written array\n");
 			err = -ENOMEM;
@@ -1479,7 +1480,7 @@  static void read_page(struct nandsim *ns, int num)
 	union ns_mem *mypage;
 
 	if (ns->cfile) {
-		if (!ns->pages_written[ns->regs.row]) {
+		if (!test_bit(ns->regs.row, ns->pages_written)) {
 			NS_DBG("read_page: page %d not written\n", ns->regs.row);
 			memset(ns->buf.byte, 0xFF, num);
 		} else {
@@ -1525,9 +1526,9 @@  static void erase_sector(struct nandsim *ns)
 
 	if (ns->cfile) {
 		for (i = 0; i < ns->geom.pgsec; i++)
-			if (ns->pages_written[ns->regs.row + i]) {
+			if (__test_and_clear_bit(ns->regs.row + i,
+						 ns->pages_written)) {
 				NS_DBG("erase_sector: freeing page %d\n", ns->regs.row + i);
-				ns->pages_written[ns->regs.row + i] = 0;
 			}
 		return;
 	}
@@ -1560,7 +1561,7 @@  static int prog_page(struct nandsim *ns, int num)
 		NS_DBG("prog_page: writing page %d\n", ns->regs.row);
 		pg_off = ns->file_buf + ns->regs.column + ns->regs.off;
 		off = (loff_t)ns->regs.row * ns->geom.pgszoob + ns->regs.column + ns->regs.off;
-		if (!ns->pages_written[ns->regs.row]) {
+		if (!test_bit(ns->regs.row, ns->pages_written)) {
 			all = 1;
 			memset(ns->file_buf, 0xff, ns->geom.pgszoob);
 		} else {
@@ -1580,7 +1581,7 @@  static int prog_page(struct nandsim *ns, int num)
 				NS_ERR("prog_page: write error for page %d ret %ld\n", ns->regs.row, (long)tx);
 				return -1;
 			}
-			ns->pages_written[ns->regs.row] = 1;
+			__set_bit(ns->regs.row, ns->pages_written);
 		} else {
 			tx = write_file(ns, ns->cfile, pg_off, num, off);
 			if (tx != num) {