linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Peter Zijlstra <peterz@infradead.org>
Cc: linux-mtd@lists.infradead.org,
	David Woodhouse <dwmw2@infradead.org>,
	Brian Norris <computersforpeace@gmail.com>,
	Artem Bityutskiy <dedekind1@gmail.com>,
	Richard Weinberger <richard@nod.at>,
	tglx@linutronix.de
Subject: [PATCH v2] mtd: nand: do FIFO processing in nand_get_device()
Date: Sun, 6 Dec 2015 15:23:18 +0100	[thread overview]
Message-ID: <20151206142318.GB5816@linutronix.de> (raw)
In-Reply-To: <20151206141757.GA5816@linutronix.de>

I have here a live lock in UBI doing
  ensure_wear_leveling() -> wear_leveling_worker() -> ubi_eba_copy_leb()
  MOVE_RETRY -> schedule_erase() -> ensure_wear_leveling()

on the same PEB over and over again. The reason for MOVE_RETRY is that
the LEB-Lock owner is stucked in nand_get_device() and does not get the
device lock. The PEB-lock owner is only scheduled on the CPU while the UBI
thread is idle during erase or read while (again) owning the device-lock
so the LEB-lock owner makes no progress.

To fix this live lock I ensure that there FIFO processing in
nand_get_device(). On release the first waiter is marked as the new
owner. If someone asks for the device and is not the waiter to which
nand device has been handed over then it will put itself on the
waitqueue.
The FIFO processing was suggested by Peter Zijlstra.

As a small optimization I use add_wait_queue_exclusive() instead
add_wait_queue() to make sure that only _one_ waiter is woken up and not
all of them.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: drop superfluously locking around list_first_entry()

 drivers/mtd/nand/nand_base.c  | 36 +++++++++++++++++++++++++++++-------
 include/linux/mtd/flashchip.h |  1 +
 include/linux/mtd/nand.h      |  1 +
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ece544efccc3..bd46b75b3540 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -137,8 +137,22 @@ static void nand_release_device(struct mtd_info *mtd)
 	/* Release the controller and the chip */
 	spin_lock(&chip->controller->lock);
 	chip->controller->active = NULL;
-	chip->state = FL_READY;
-	wake_up(&chip->controller->wq);
+
+	if (waitqueue_active(&chip->controller->wq)) {
+		wait_queue_head_t *q;
+		wait_queue_t *waiter;
+
+		q = &chip->controller->wq;
+		chip->state = FL_HANDOVER;
+
+		waiter = list_first_entry(&q->task_list, wait_queue_t,
+					  task_list);
+
+		chip->controller->handover_waiter = waiter;
+		wake_up(q);
+	} else {
+		chip->state = FL_READY;
+	}
 	spin_unlock(&chip->controller->lock);
 }
 
@@ -843,10 +857,18 @@ nand_get_device(struct mtd_info *mtd, int new_state)
 	if (!chip->controller->active)
 		chip->controller->active = chip;
 
-	if (chip->controller->active == chip && chip->state == FL_READY) {
-		chip->state = new_state;
-		spin_unlock(lock);
-		return 0;
+	if (chip->controller->active == chip) {
+		if (chip->state == FL_READY) {
+			chip->state = new_state;
+			spin_unlock(lock);
+			return 0;
+		}
+		if (chip->state == FL_HANDOVER &&
+		    chip->controller->handover_waiter == &wait) {
+			chip->state = new_state;
+			spin_unlock(lock);
+			return 0;
+		}
 	}
 	if (new_state == FL_PM_SUSPENDED) {
 		if (chip->controller->active->state == FL_PM_SUSPENDED) {
@@ -856,7 +878,7 @@ nand_get_device(struct mtd_info *mtd, int new_state)
 		}
 	}
 	set_current_state(TASK_UNINTERRUPTIBLE);
-	add_wait_queue(wq, &wait);
+	add_wait_queue_exclusive(wq, &wait);
 	spin_unlock(lock);
 	schedule();
 	remove_wait_queue(wq, &wait);
diff --git a/include/linux/mtd/flashchip.h b/include/linux/mtd/flashchip.h
index b63fa457febd..c855f4fd51b8 100644
--- a/include/linux/mtd/flashchip.h
+++ b/include/linux/mtd/flashchip.h
@@ -58,6 +58,7 @@ typedef enum {
 	FL_OTPING,
 	FL_PREPARING_ERASE,
 	FL_VERIFYING_ERASE,
+	FL_HANDOVER,
 
 	FL_UNKNOWN
 } flstate_t;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 5a9d1d4c2487..2686dc5dd51b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -439,6 +439,7 @@ struct nand_hw_control {
 	spinlock_t lock;
 	struct nand_chip *active;
 	wait_queue_head_t wq;
+	wait_queue_t *handover_waiter;
 };
 
 /**
-- 
2.6.2

  reply	other threads:[~2015-12-06 14:23 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-11-23 18:09 [RFC] avoid a live lock in wear_leveling_worker() Sebastian Andrzej Siewior
2015-11-23 18:09 ` [RFC PATCH 1/2] mtd: nand: schedule() after releasing the device Sebastian Andrzej Siewior
2015-11-23 18:18   ` Peter Zijlstra
2015-11-25 17:35     ` [PATCH] mtd: nand: do FIFO processing in nand_get_device() Sebastian Andrzej Siewior
2015-11-30 16:15       ` Peter Zijlstra
2015-12-06 14:17         ` Sebastian Andrzej Siewior
2015-12-06 14:23           ` Sebastian Andrzej Siewior [this message]
2015-12-02 18:52       ` Brian Norris
2015-12-02 20:41         ` Sebastian Andrzej Siewior
2015-11-23 18:09 ` [RFC PATCH 2/2] mtd: ubi: wl: avoid erasing a PEB which is empty Sebastian Andrzej Siewior
2015-11-23 21:30   ` Richard Weinberger
2015-11-23 21:50     ` Richard Weinberger
2015-11-24  8:26     ` Sebastian Andrzej Siewior
2015-11-24  8:39       ` Richard Weinberger
2015-11-24  8:42         ` Sebastian Andrzej Siewior
2015-11-24  9:02           ` Richard Weinberger
2015-11-24  9:07             ` Sebastian Andrzej Siewior
2015-11-24  9:16               ` Richard Weinberger
2015-11-24 12:58   ` Artem Bityutskiy
2015-11-24 13:33     ` Sebastian Andrzej Siewior
2015-11-24 13:40       ` Artem Bityutskiy
2015-11-24 13:57       ` Artem Bityutskiy
2015-11-26 14:56     ` Sebastian Andrzej Siewior

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=20151206142318.GB5816@linutronix.de \
    --to=bigeasy@linutronix.de \
    --cc=computersforpeace@gmail.com \
    --cc=dedekind1@gmail.com \
    --cc=dwmw2@infradead.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=peterz@infradead.org \
    --cc=richard@nod.at \
    --cc=tglx@linutronix.de \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).