[PATCH] RFC: mmc: block: replace semaphore with freezing

[PATCH] RFC: mmc: block: replace semaphore with freezing

[Linus Walleij]

The MMC/SD block core request processing thread is taking a
semaphore in the request processing section and the same
semaphore is taken around suspend/resume operations.

The purpose of the semaphore is to block any request from being
issued to the MMC/SD host controllers when the system is
suspended. A semaphore is used in place of a mutex since the
calls are coming from different threads.

This construction predates the kernel thread freezer mechanism:
we can easily replace the semaphore by instead activating the
freezer with set_freezable() and call try_to_freeze() instead
of the up(); schedule(); down(); construction that is devised
to let the suspend/resume calls get in and grab the semaphore.

Tested with a few suspend/resume to memory cycles on the Ux500
when doing intense dd operations in the background: the
thread thaws and resumes operation after resume.

Cc: linux-pm@vger.kernel.org
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I haven't seen any need to preserve the call to schedule()
in the request processing thread, but I want advice on whether
that has a point. I would guess the thread will just anyway
be preempted if needed anyway as it is marked interruptible?
---
 drivers/mmc/card/queue.c | 13 ++-----------
 drivers/mmc/card/queue.h |  1 -
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 8037f73a109a..09a932ffe46e 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -55,8 +55,8 @@ static int mmc_queue_thread(void *d)
 	struct request_queue *q = mq->queue;
 
 	current->flags |= PF_MEMALLOC;
+	set_freezable();
 
-	down(&mq->thread_sem);
 	do {
 		struct request *req = NULL;
 
@@ -95,12 +95,9 @@ static int mmc_queue_thread(void *d)
 				set_current_state(TASK_RUNNING);
 				break;
 			}
-			up(&mq->thread_sem);
-			schedule();
-			down(&mq->thread_sem);
+			try_to_freeze();
 		}
 	} while (1);
-	up(&mq->thread_sem);
 
 	return 0;
 }
@@ -289,8 +286,6 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
 			goto cleanup_queue;
 	}
 
-	sema_init(&mq->thread_sem, 1);
-
 	mq->thread = kthread_run(mmc_queue_thread, mq, "mmcqd/%d%s",
 		host->index, subname ? subname : "");
 
@@ -424,8 +419,6 @@ void mmc_queue_suspend(struct mmc_queue *mq)
 		spin_lock_irqsave(q->queue_lock, flags);
 		blk_stop_queue(q);
 		spin_unlock_irqrestore(q->queue_lock, flags);
-
-		down(&mq->thread_sem);
 	}
 }
 
@@ -441,8 +434,6 @@ void mmc_queue_resume(struct mmc_queue *mq)
 	if (mq->flags & MMC_QUEUE_SUSPENDED) {
 		mq->flags &= ~MMC_QUEUE_SUSPENDED;
 
-		up(&mq->thread_sem);
-
 		spin_lock_irqsave(q->queue_lock, flags);
 		blk_start_queue(q);
 		spin_unlock_irqrestore(q->queue_lock, flags);
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index 3c15a75bae86..fe10f94795de 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -53,7 +53,6 @@ struct mmc_queue_req {
 struct mmc_queue {
 	struct mmc_card		*card;
 	struct task_struct	*thread;
-	struct semaphore	thread_sem;
 	unsigned int		flags;
 #define MMC_QUEUE_SUSPENDED	(1 << 0)
 #define MMC_QUEUE_NEW_REQUEST	(1 << 1)
-- 
2.7.4

Re: [PATCH] RFC: mmc: block: replace semaphore with freezing

[Arnd Bergmann]

On Wednesday, November 16, 2016 11:51:04 AM CET Linus Walleij wrote:
> @@ -95,12 +95,9 @@ static int mmc_queue_thread(void *d)
>                                 set_current_state(TASK_RUNNING);
>                                 break;
>                         }
> -                       up(&mq->thread_sem);
> -                       schedule();
> -                       down(&mq->thread_sem);
> +                       try_to_freeze();
> 

The schedule() here is where we wait for new requests to come in
from mmc_request_fn(), you can't remove that or you end up spinning
continuously.

	Arnd

Re: [PATCH] RFC: mmc: block: replace semaphore with freezing

[Rafael J. Wysocki]

On Wed, Nov 16, 2016 at 11:51 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> The MMC/SD block core request processing thread is taking a
> semaphore in the request processing section and the same
> semaphore is taken around suspend/resume operations.
>
> The purpose of the semaphore is to block any request from being
> issued to the MMC/SD host controllers when the system is
> suspended. A semaphore is used in place of a mutex since the
> calls are coming from different threads.
>
> This construction predates the kernel thread freezer mechanism:
> we can easily replace the semaphore by instead activating the
> freezer with set_freezable() and call try_to_freeze() instead
> of the up(); schedule(); down(); construction that is devised
> to let the suspend/resume calls get in and grab the semaphore.
>
> Tested with a few suspend/resume to memory cycles on the Ux500
> when doing intense dd operations in the background: the
> thread thaws and resumes operation after resume.

Well, we had a session at the KS regarding usage of the freezer on
kernel threads and the conclusion was to get rid of that (as opposed
to freezing user space, which is necessary IMO).  So this change would
go in the opposite direction. :-)

Thanks,
Rafael

Re: [PATCH] RFC: mmc: block: replace semaphore with freezing

[Jiri Kosina]

On Wed, 16 Nov 2016, Rafael J. Wysocki wrote:

> > The MMC/SD block core request processing thread is taking a
> > semaphore in the request processing section and the same
> > semaphore is taken around suspend/resume operations.
> >
> > The purpose of the semaphore is to block any request from being
> > issued to the MMC/SD host controllers when the system is
> > suspended. A semaphore is used in place of a mutex since the
> > calls are coming from different threads.
> >
> > This construction predates the kernel thread freezer mechanism:
> > we can easily replace the semaphore by instead activating the
> > freezer with set_freezable() and call try_to_freeze() instead
> > of the up(); schedule(); down(); construction that is devised
> > to let the suspend/resume calls get in and grab the semaphore.
> >
> > Tested with a few suspend/resume to memory cycles on the Ux500
> > when doing intense dd operations in the background: the
> > thread thaws and resumes operation after resume.
> 
> Well, we had a session at the KS regarding usage of the freezer on
> kernel threads and the conclusion was to get rid of that (as opposed
> to freezing user space, which is necessary IMO).  So this change would
> go in the opposite direction. :-)

[ thanks for CCing me, Rafael ]

Agreed. You already have PM callbacks handled properly, so the way this 
should be done is once you're in the PM-callback due to system going 
through power management change, you just stop generation of any new I/O, 
and tell the kthread that it should schedule itself out.

Plus the schedule() has to stay there anyway, as try_to_freeze() is not 
going to provide you with the schedule() semantics unless the system is 
actually undergoing a PM transition towards suspend.

So either semaphore, or some kind of atomic flag, is exactly the 
information that should be passed from the PM callback to the kthread.

(now, I agree, that this is one of the very rare cases where the kthread 
freezer is actually being used properly -- IOW to pause a kthread that is 
actually generating new I/O ... but given the fact that this is so rare, 
and the API is so heavily abused, I really tend to heavily prefer a 
semaphore / flag based aproach).

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] RFC: mmc: block: replace semaphore with freezing

[Linus Walleij]

On Wed, Nov 16, 2016 at 1:46 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:

> Well, we had a session at the KS regarding usage of the freezer on
> kernel threads and the conclusion was to get rid of that (as opposed
> to freezing user space, which is necessary IMO).  So this change would
> go in the opposite direction. :-)

Aha so I should not make this thread look like everyone else, instead
everyone else should look like this thread, haha :D

Ah well, I'll just drop it.

Linus

Re: [PATCH] RFC: mmc: block: replace semaphore with freezing

[Arnd Bergmann]

On Wednesday, November 16, 2016 4:20:47 PM CET Linus Walleij wrote:
> On Wed, Nov 16, 2016 at 1:46 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> 
> > Well, we had a session at the KS regarding usage of the freezer on
> > kernel threads and the conclusion was to get rid of that (as opposed
> > to freezing user space, which is necessary IMO).  So this change would
> > go in the opposite direction. 
> 
> Aha so I should not make this thread look like everyone else, instead
> everyone else should look like this thread, haha 
> 
> Ah well, I'll just drop it.

It would still be good to remove the semaphore and do something else,
as we also want to remove all semaphores. ;-)

We could check "mq->flags & MMC_QUEUE_SUSPENDED" in the kthread to see
if the queue is currently suspended, and otherwise go to sleep there,
and then call wake_up() in the resume function.

While looking at that code, I just noticed that access to 
mq->flags is racy and should be fixed as well.

	Arnd

Re: [PATCH] RFC: mmc: block: replace semaphore with freezing

[Linus Walleij]

On Wed, Nov 16, 2016 at 5:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, November 16, 2016 4:20:47 PM CET Linus Walleij wrote:
>> On Wed, Nov 16, 2016 at 1:46 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> > Well, we had a session at the KS regarding usage of the freezer on
>> > kernel threads and the conclusion was to get rid of that (as opposed
>> > to freezing user space, which is necessary IMO).  So this change would
>> > go in the opposite direction.
>>
>> Aha so I should not make this thread look like everyone else, instead
>> everyone else should look like this thread, haha
>>
>> Ah well, I'll just drop it.
>
> It would still be good to remove the semaphore and do something else,
> as we also want to remove all semaphores. ;-)
>
> We could check "mq->flags & MMC_QUEUE_SUSPENDED" in the kthread to see
> if the queue is currently suspended, and otherwise go to sleep there,
> and then call wake_up() in the resume function.

Hm... so simply:

if (mq->flags & MMC_QUEUE_SUSPENDED)
  schedule();

?

This whole kthread business is pretty messy. I would prefer if I could
just convert it to a workqueue. Just that it's not very simple the way
it speculates around in the request queue from the block layer.

> While looking at that code, I just noticed that access to
> mq->flags is racy and should be fixed as well.

I guess we should take the queue lock &q->lock around accessing
the flags.

Yours,
Linus Walleij

Re: [PATCH] RFC: mmc: block: replace semaphore with freezing

[Russell King - ARM Linux]

On Tue, Nov 22, 2016 at 09:54:18AM +0100, Linus Walleij wrote:
> On Wed, Nov 16, 2016 at 5:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday, November 16, 2016 4:20:47 PM CET Linus Walleij wrote:
> >> On Wed, Nov 16, 2016 at 1:46 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>
> >> > Well, we had a session at the KS regarding usage of the freezer on
> >> > kernel threads and the conclusion was to get rid of that (as opposed
> >> > to freezing user space, which is necessary IMO).  So this change would
> >> > go in the opposite direction.
> >>
> >> Aha so I should not make this thread look like everyone else, instead
> >> everyone else should look like this thread, haha
> >>
> >> Ah well, I'll just drop it.
> >
> > It would still be good to remove the semaphore and do something else,
> > as we also want to remove all semaphores. ;-)
> >
> > We could check "mq->flags & MMC_QUEUE_SUSPENDED" in the kthread to see
> > if the queue is currently suspended, and otherwise go to sleep there,
> > and then call wake_up() in the resume function.
> 
> Hm... so simply:
> 
> if (mq->flags & MMC_QUEUE_SUSPENDED)
>   schedule();
> 
> ?

No, the schedule() is required when there are no more requests to
process, so the thread doesn't spin waiting for the next request to
appear.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] RFC: mmc: block: replace semaphore with freezing

[Arnd Bergmann]

On Tuesday, November 22, 2016 9:54:18 AM CET Linus Walleij wrote:
> On Wed, Nov 16, 2016 at 5:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday, November 16, 2016 4:20:47 PM CET Linus Walleij wrote:
> >> On Wed, Nov 16, 2016 at 1:46 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>
> >> > Well, we had a session at the KS regarding usage of the freezer on
> >> > kernel threads and the conclusion was to get rid of that (as opposed
> >> > to freezing user space, which is necessary IMO).  So this change would
> >> > go in the opposite direction.
> >>
> >> Aha so I should not make this thread look like everyone else, instead
> >> everyone else should look like this thread, haha
> >>
> >> Ah well, I'll just drop it.
> >
> > It would still be good to remove the semaphore and do something else,
> > as we also want to remove all semaphores. 
> >
> > We could check "mq->flags & MMC_QUEUE_SUSPENDED" in the kthread to see
> > if the queue is currently suspended, and otherwise go to sleep there,
> > and then call wake_up() in the resume function.
> 
> Hm... so simply:
> 
> if (mq->flags & MMC_QUEUE_SUSPENDED)
>   schedule();
> 
> ?

Something like that.

> This whole kthread business is pretty messy. I would prefer if I could
> just convert it to a workqueue. Just that it's not very simple the way
> it speculates around in the request queue from the block layer.

I don't see how that would work, but might be worth trying.
After doing that, a simple flush_workqueue() might be enough
to take care of the suspend operation.

> > While looking at that code, I just noticed that access to
> > mq->flags is racy and should be fixed as well.
> 
> I guess we should take the queue lock &q->lock around accessing
> the flags.

Yes, either that, or use set_bit/test_bit/test_and_set_bit for
atomic access. For instance, this one

        if (mq->flags & MMC_QUEUE_NEW_REQUEST) {
                mq->flags &= ~MMC_QUEUE_NEW_REQUEST;

Could be

	if (test_and_clear(MMC_QUEUE_NEW_REQUEST_BIT, &mq->flags))
			...


	Arnd