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

Submitted by Akinobu Mita on July 28, 2013, 2:21 a.m.

Details

Message ID 1374978118-11418-1-git-send-email-akinobu.mita@gmail.com
State Accepted
Commit 08efe91a1befa67bcc3799e0d29b7194803e1538
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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) {