Should suspend plug low-level devices?

Should suspend plug low-level devices?

[Alan Stern]

[Removed linux-usb and usb-storage from CC: and added linux-pm; changed 
$SUBJECT]

On Wed, 16 Mar 2016, Tejun Heo wrote:

> Hello, Jan, Alan.
> 
> On Tue, Mar 15, 2016 at 10:25:43AM +0100, Jan Kara wrote:
> > > The kernel does suspend device drivers; that is, it invokes their
> > > suspend callbacks.  But it doesn't "freeze" them in any sense.  Once a 
> > > driver has been suspended, it assumes it won't receive any I/O requests 
> > > until it has been resumed.  Therefore the kernel first has to prevent 
> > > all the upper layers from generating such requests and/or sending them 
> > > to the low-level drivers.
> > 
> > OK, so Tejun and you should talk together because you both seem to want
> > something else... If I understand it right, Tejun wants suspended devices
> > to just queue requests that have been submitted after these devices were
> > suspended and complete them once they are resumed...
> 
> Yeah, I suppose that's why we have the code base we do now.  I don't
> think freezing kernel threads is the right mechanism to plug IO
> devices during suspend.  It's way too error-prone and causes a
> dependency nightmare as it acts essentially as a system-wide lock.
> 
> More complex drivers already plug themselves which are necessary no
> matter what as upper layers or some kthreads aren't the only sources
> of commands to devices.  We can plug at block layer for IOs coming
> down from higher layers.  We can even provide a mechanism to plug
> certain kthreads if necessary but they should be contained in the
> driver - e.g. the suspend callback specifically blocking certain
> specific kthreads - instead of the vague "the system is generally
> stopped now and it seems to work most of the time" that we're doing
> now.

Doing what you suggest would require a tremendous effort.  There are
lots and lots of drivers that have no provisions for plugging; they
would all need to be modified.  Changing the higher layers instead (to
prevent them from generating I/O requests during suspend) seems much
easier.

The original design of the PM subsystem was intended to make life
easier for drivers by minimizing the amount of work needed to support
suspend/resume.  Maybe the decision to do it that way was wrong, but
it's kind of late to change the design now.

I foresee other kinds of problems, too.  Suppose after a suspend 
starts, some higher layer code creates an I/O request with a short 
timeout.  If the device weren't suspended the request would succeed -- 
should it time out and fail now that the system is going to sleep?  
That would make system sleep transitions non-transparent.

Alan Stern

Re: Should suspend plug low-level devices?

[Rafael J. Wysocki]

On Wed, Mar 16, 2016 at 4:37 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> [Removed linux-usb and usb-storage from CC: and added linux-pm; changed
> $SUBJECT]
>
> On Wed, 16 Mar 2016, Tejun Heo wrote:
>
>> Hello, Jan, Alan.
>>
>> On Tue, Mar 15, 2016 at 10:25:43AM +0100, Jan Kara wrote:
>> > > The kernel does suspend device drivers; that is, it invokes their
>> > > suspend callbacks.  But it doesn't "freeze" them in any sense.  Once a
>> > > driver has been suspended, it assumes it won't receive any I/O requests
>> > > until it has been resumed.  Therefore the kernel first has to prevent
>> > > all the upper layers from generating such requests and/or sending them
>> > > to the low-level drivers.
>> >
>> > OK, so Tejun and you should talk together because you both seem to want
>> > something else... If I understand it right, Tejun wants suspended devices
>> > to just queue requests that have been submitted after these devices were
>> > suspended and complete them once they are resumed...
>>
>> Yeah, I suppose that's why we have the code base we do now.  I don't
>> think freezing kernel threads is the right mechanism to plug IO
>> devices during suspend.  It's way too error-prone and causes a
>> dependency nightmare as it acts essentially as a system-wide lock.
>>
>> More complex drivers already plug themselves which are necessary no
>> matter what as upper layers or some kthreads aren't the only sources
>> of commands to devices.  We can plug at block layer for IOs coming
>> down from higher layers.  We can even provide a mechanism to plug
>> certain kthreads if necessary but they should be contained in the
>> driver - e.g. the suspend callback specifically blocking certain
>> specific kthreads - instead of the vague "the system is generally
>> stopped now and it seems to work most of the time" that we're doing
>> now.
>
> Doing what you suggest would require a tremendous effort.  There are
> lots and lots of drivers that have no provisions for plugging; they
> would all need to be modified.  Changing the higher layers instead (to
> prevent them from generating I/O requests during suspend) seems much
> easier.
>
> The original design of the PM subsystem was intended to make life
> easier for drivers by minimizing the amount of work needed to support
> suspend/resume.  Maybe the decision to do it that way was wrong, but
> it's kind of late to change the design now.
>
> I foresee other kinds of problems, too.  Suppose after a suspend
> starts, some higher layer code creates an I/O request with a short
> timeout.  If the device weren't suspended the request would succeed --
> should it time out and fail now that the system is going to sleep?
> That would make system sleep transitions non-transparent.

Right.

Essentially, every device interface that might be accessed by user
space would have to be intercepted somehow.

Thanks,
Rafael

Re: Should suspend plug low-level devices?

[Tejun Heo]

Hello, Rafael.

On Wed, Mar 16, 2016 at 05:31:23PM +0100, Rafael J. Wysocki wrote:
> Essentially, every device interface that might be accessed by user
> space would have to be intercepted somehow.

That's what we're doing now.  What I'm suggesting is that we should be
plugging the point where they come together instead of the sources of
which there can be many.  Freezing does block a lot of those sources
but not all.

Thanks.

-- 
tejun

Re: Should suspend plug low-level devices?

[Rafael J. Wysocki]

On Wed, Mar 16, 2016 at 5:35 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Rafael.
>
> On Wed, Mar 16, 2016 at 05:31:23PM +0100, Rafael J. Wysocki wrote:
>> Essentially, every device interface that might be accessed by user
>> space would have to be intercepted somehow.
>
> That's what we're doing now.  What I'm suggesting is that we should be
> plugging the point where they come together instead of the sources of
> which there can be many.  Freezing does block a lot of those sources
> but not all.

Well, is there any point where they come together?  Say somebody
communicates with user space over mmaped address range and writes to
that go to hardware.  What do we do then?

Thanks,
Rafael

Re: Should suspend plug low-level devices?

[Jiri Kosina]

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

> > That's what we're doing now.  What I'm suggesting is that we should be
> > plugging the point where they come together instead of the sources of
> > which there can be many.  Freezing does block a lot of those sources
> > but not all.
> 
> Well, is there any point where they come together?  Say somebody
> communicates with user space over mmaped address range and writes to
> that go to hardware.  What do we do then?

There are no more writes happening to that region after userspace has been 
frozen. And we're definitely not getting rid of userspace freezing.

Or have I missed the point of your question?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: Should suspend plug low-level devices?

[Rafael J. Wysocki]

On Thu, Mar 17, 2016 at 12:18 AM, Jiri Kosina <jikos@kernel.org> wrote:
> On Wed, 16 Mar 2016, Rafael J. Wysocki wrote:
>
>> > That's what we're doing now.  What I'm suggesting is that we should be
>> > plugging the point where they come together instead of the sources of
>> > which there can be many.  Freezing does block a lot of those sources
>> > but not all.
>>
>> Well, is there any point where they come together?  Say somebody
>> communicates with user space over mmaped address range and writes to
>> that go to hardware.  What do we do then?
>
> There are no more writes happening to that region after userspace has been
> frozen. And we're definitely not getting rid of userspace freezing.
>
> Or have I missed the point of your question?

I thought Tejun was talking about getting rid of userspace freezing.
Sorry if that was not the case.

As I've already said for multiple times, I think that the freezing
should really be limited to user space as a rule.  And in particular,
if freezing of any kernel threads or workqueues etc causes problems to
happen, the code using those freezable things is simply buggy and
needs to be fixed.  Most likely by getting rid of the freezing from
it.

Re: Should suspend plug low-level devices?

[Jiri Kosina]

On Thu, 17 Mar 2016, Rafael J. Wysocki wrote:

> >> Well, is there any point where they come together?  Say somebody
> >> communicates with user space over mmaped address range and writes to
> >> that go to hardware.  What do we do then?
> >
> > There are no more writes happening to that region after userspace has been
> > frozen. And we're definitely not getting rid of userspace freezing.
> >
> > Or have I missed the point of your question?
> 
> I thought Tejun was talking about getting rid of userspace freezing.
> Sorry if that was not the case.

Oh, I definitely belive that the current "holy grail" is to get rid of 
ktrehad / kernel code freezing. Userspace is a completely different story, 
I agree.

> As I've already said for multiple times, I think that the freezing 
> should really be limited to user space as a rule.  And in particular, if 
> freezing of any kernel threads or workqueues etc causes problems to 
> happen, the code using those freezable things is simply buggy and needs 
> to be fixed.  Most likely by getting rid of the freezing from it.

I am (slowly) working towards this goal. First I am trying to make sure 
that all the kernel is using the kthread freezing API in a well-defined 
way (which is currently not the case at all).

Before this is done, it is more or less impossible to analyse the current 
users and make any decisions on top of that.

Once some kind of analysis is possible, I am pretty sure that we'll be 
able to get rid of most kthread freezing by introducing fs freezing during 
suspend (and fixing a lot of currently existing bugs as a nice 
side-effect), and perhaps eventually ditch the kthread freezer altogether.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: Should suspend plug low-level devices?

[Rafael J. Wysocki]

On Thu, Mar 17, 2016 at 12:42 AM, Jiri Kosina <jikos@kernel.org> wrote:
> On Thu, 17 Mar 2016, Rafael J. Wysocki wrote:
>
>> >> Well, is there any point where they come together?  Say somebody
>> >> communicates with user space over mmaped address range and writes to
>> >> that go to hardware.  What do we do then?
>> >
>> > There are no more writes happening to that region after userspace has been
>> > frozen. And we're definitely not getting rid of userspace freezing.
>> >
>> > Or have I missed the point of your question?
>>
>> I thought Tejun was talking about getting rid of userspace freezing.
>> Sorry if that was not the case.
>
> Oh, I definitely belive that the current "holy grail" is to get rid of
> ktrehad / kernel code freezing. Userspace is a completely different story,
> I agree.
>
>> As I've already said for multiple times, I think that the freezing
>> should really be limited to user space as a rule.  And in particular, if
>> freezing of any kernel threads or workqueues etc causes problems to
>> happen, the code using those freezable things is simply buggy and needs
>> to be fixed.  Most likely by getting rid of the freezing from it.
>
> I am (slowly) working towards this goal. First I am trying to make sure
> that all the kernel is using the kthread freezing API in a well-defined
> way (which is currently not the case at all).
>
> Before this is done, it is more or less impossible to analyse the current
> users and make any decisions on top of that.
>
> Once some kind of analysis is possible, I am pretty sure that we'll be
> able to get rid of most kthread freezing by introducing fs freezing during
> suspend (and fixing a lot of currently existing bugs as a nice
> side-effect), and perhaps eventually ditch the kthread freezer altogether.

Sounds good! :-)

Re: Should suspend plug low-level devices?

[Oliver Neukum]

On Thu, 2016-03-17 at 00:42 +0100, Jiri Kosina wrote:
> I am (slowly) working towards this goal. First I am trying to make
> sure 
> that all the kernel is using the kthread freezing API in a
> well-defined 
> way (which is currently not the case at all).
> 
> Before this is done, it is more or less impossible to analyse the
> current 
> users and make any decisions on top of that.
> 
> Once some kind of analysis is possible, I am pretty sure that we'll
> be 
> able to get rid of most kthread freezing by introducing fs freezing
> during 
> suspend (and fixing a lot of currently existing bugs as a nice 
> side-effect), and perhaps eventually ditch the kthread freezer
> altogether.

But what about character devices?

	Regards
		Oliver

Re: Should suspend plug low-level devices?

[Jiri Kosina]

On Thu, 17 Mar 2016, Oliver Neukum wrote:

> But what about character devices?

What makes the special in this context?

-- 
Jiri Kosina
SUSE Labs

Re: Should suspend plug low-level devices?

[Oliver Neukum]

On Thu, 2016-03-17 at 09:02 +0100, Jiri Kosina wrote:
> On Thu, 17 Mar 2016, Oliver Neukum wrote:
> 
> > But what about character devices?
> 
> What makes the special in this context?

Actually nothing, but they don't dpend on file systems.
So I see a basic problem, how does a device driver
know which kernel threads cause IO? Are you assuming
that only those a driver starts itself are important?

	Regards
		Oliver

Re: Should suspend plug low-level devices?

[Jiri Kosina]

On Thu, 17 Mar 2016, Oliver Neukum wrote:

> > What makes the special in this context?
> 
> Actually nothing, but they don't dpend on file systems. 

Sure. And userspace is frozen, so nothing new is happening with them from 
the process side.

> So I see a basic problem, how does a device driver know which kernel 
> threads cause IO? Are you assuming that only those a driver starts 
> itself are important?

Every driver should know what the kthreads it is spawning are doing, and 
whether they should be handled in a special way during suspend (in most 
cases, they don't, I believe).

Most kthreads don't generate any IO by themselves; usually they are 
actualy I/O helpers, which are threads you in fact want to be running 
during suspend.

-- 
Jiri Kosina
SUSE Labs

Re: Should suspend plug low-level devices?

[Oliver Neukum]

On Thu, 2016-03-17 at 09:35 +0100, Jiri Kosina wrote:
> On Thu, 17 Mar 2016, Oliver Neukum wrote:
> 
> > > What makes the special in this context?
> > 
> > Actually nothing, but they don't dpend on file systems. 
> 
> Sure. And userspace is frozen, so nothing new is happening with them from 
> the process side.

Well, to the same extent as to block devices.

> > So I see a basic problem, how does a device driver know which kernel 
> > threads cause IO? Are you assuming that only those a driver starts 
> > itself are important?
> 
> Every driver should know what the kthreads it is spawning are doing, and 
> whether they should be handled in a special way during suspend (in most 
> cases, they don't, I believe).
> 
> Most kthreads don't generate any IO by themselves; usually they are 
> actualy I/O helpers, which are threads you in fact want to be running 
> during suspend.

Yes, but you are making the assumption that only the kthreads a driver
started are causing IO to it. That is a tall one.

- logging
- network
- device discovery on busses
- detection of media
- knfsd

All these things cause IO and I probably forgot some.
Defined semantics for the threads a driver starts are a good idea,
but it leaves out the really hard cases.

	Regards
		Oliver

Re: Should suspend plug low-level devices?

[Jiri Kosina]

On Thu, 17 Mar 2016, Oliver Neukum wrote:

> Yes, but you are making the assumption that only the kthreads a driver
> started are causing IO to it. That is a tall one.
> 
> - logging
> - network
> - device discovery on busses
> - detection of media
> - knfsd
> 
> All these things cause IO and I probably forgot some. Defined semantics 
> for the threads a driver starts are a good idea, but it leaves out the 
> really hard cases.

So you named only one specific kthread here (knfsd), so let's focus on 
that one. Where is that one currently being frozen during suspend?

-- 
Jiri Kosina
SUSE Labs

Re: Should suspend plug low-level devices?

[Oliver Neukum]

On Thu, 2016-03-17 at 11:20 +0100, Jiri Kosina wrote:
> On Thu, 17 Mar 2016, Oliver Neukum wrote:
> 
> > Yes, but you are making the assumption that only the kthreads a driver
> > started are causing IO to it. That is a tall one.
> > 
> > - logging
> > - network
> > - device discovery on busses
> > - detection of media
> > - knfsd
> > 
> > All these things cause IO and I probably forgot some. Defined semantics 
> > for the threads a driver starts are a good idea, but it leaves out the 
> > really hard cases.
> 
> So you named only one specific kthread here (knfsd), so let's focus on 
> that one. Where is that one currently being frozen during suspend?

In svc_recv()

That concentration, however, is problematic. We can target anything
specific, but the point is, do we know what is running?

	Regards
		Oliver

Re: Should suspend plug low-level devices?

[Jiri Kosina]

On Thu, 17 Mar 2016, Oliver Neukum wrote:

> > > - logging
> > > - network
> > > - device discovery on busses
> > > - detection of media
> > > - knfsd
> > > 
> > > All these things cause IO and I probably forgot some. Defined semantics 
> > > for the threads a driver starts are a good idea, but it leaves out the 
> > > really hard cases.
> > 
> > So you named only one specific kthread here (knfsd), so let's focus on 
> > that one. Where is that one currently being frozen during suspend?
> 
> In svc_recv()
> 
> That concentration, however, is problematic. We can target anything
> specific, but the point is, do we know what is running?

The important question here is (and I don't have answer to that yet, 
unfortunately), whether filesystem freezing works properly on NFS.

Properly working filesystem freezing is of course hard pre-requisity.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: Should suspend plug low-level devices?

[Alan Stern]

On Thu, 17 Mar 2016, Jiri Kosina wrote:

> On Thu, 17 Mar 2016, Rafael J. Wysocki wrote:
> 
> > >> Well, is there any point where they come together?  Say somebody
> > >> communicates with user space over mmaped address range and writes to
> > >> that go to hardware.  What do we do then?
> > >
> > > There are no more writes happening to that region after userspace has been
> > > frozen. And we're definitely not getting rid of userspace freezing.
> > >
> > > Or have I missed the point of your question?
> > 
> > I thought Tejun was talking about getting rid of userspace freezing.
> > Sorry if that was not the case.
> 
> Oh, I definitely belive that the current "holy grail" is to get rid of 
> ktrehad / kernel code freezing. Userspace is a completely different story, 
> I agree.

Why get rid of kernel code freezing?  It is the perfect solution for 
some problems.

> > As I've already said for multiple times, I think that the freezing 
> > should really be limited to user space as a rule.  And in particular, if 
> > freezing of any kernel threads or workqueues etc causes problems to 
> > happen, the code using those freezable things is simply buggy and needs 
> > to be fixed.  Most likely by getting rid of the freezing from it.
> 
> I am (slowly) working towards this goal. First I am trying to make sure 
> that all the kernel is using the kthread freezing API in a well-defined 
> way (which is currently not the case at all).
> 
> Before this is done, it is more or less impossible to analyse the current 
> users and make any decisions on top of that.
> 
> Once some kind of analysis is possible, I am pretty sure that we'll be 
> able to get rid of most kthread freezing by introducing fs freezing during 
> suspend (and fixing a lot of currently existing bugs as a nice 
> side-effect), and perhaps eventually ditch the kthread freezer altogether.

While fs freezing would undoubtedly be a good thing, I don't think it 
will allow you to eliminate kthread (or more specifically, workqueue) 
freezing.

Alan Stern

Re: Should suspend plug low-level devices?

[Tejun Heo]

Hello,

On Thu, Mar 17, 2016 at 02:55:47PM -0400, Alan Stern wrote:
> > Oh, I definitely belive that the current "holy grail" is to get rid of 
> > ktrehad / kernel code freezing. Userspace is a completely different story, 
> > I agree.
> 
> Why get rid of kernel code freezing?  It is the perfect solution for 
> some problems.

with pretty severe generic problems which are difficult to root out.
It's more about making it finer grained rather than removing it.

Thanks.

-- 
tejun

Re: Should suspend plug low-level devices?

[Tejun Heo]

Hello, Rafael, Jiri.

On Thu, Mar 17, 2016 at 12:24:57AM +0100, Rafael J. Wysocki wrote:
> > There are no more writes happening to that region after userspace has been
> > frozen. And we're definitely not getting rid of userspace freezing.
> >
> > Or have I missed the point of your question?
> 
> I thought Tejun was talking about getting rid of userspace freezing.
> Sorry if that was not the case.

Oh, kthreads, definitely.  Userland freezing doesn't cause any
dependency issues in the kernel and I don't see any reason for
changing userland part.

> As I've already said for multiple times, I think that the freezing
> should really be limited to user space as a rule.  And in particular,
> if freezing of any kernel threads or workqueues etc causes problems to
> happen, the code using those freezable things is simply buggy and
> needs to be fixed.  Most likely by getting rid of the freezing from
> it.

We're in full agreement.

Thanks.

-- 
tejun

Re: Should suspend plug low-level devices?

[Alan Stern]

On Wed, 16 Mar 2016, Tejun Heo wrote:

> Hello, Rafael, Jiri.
> 
> On Thu, Mar 17, 2016 at 12:24:57AM +0100, Rafael J. Wysocki wrote:
> > > There are no more writes happening to that region after userspace has been
> > > frozen. And we're definitely not getting rid of userspace freezing.
> > >
> > > Or have I missed the point of your question?
> > 
> > I thought Tejun was talking about getting rid of userspace freezing.
> > Sorry if that was not the case.
> 
> Oh, kthreads, definitely.  Userland freezing doesn't cause any
> dependency issues in the kernel and I don't see any reason for
> changing userland part.

Actually, it does cause dependency issues.  The prototype example is
fuse, when one process can't be frozen because it is waiting in the
kernel for file I/O to complete, but the I/O involves a fuse filesystem
and the userspace driver is already frozen.

Alan Stern

Re: Should suspend plug low-level devices?

[Tejun Heo]

Hello, Alan.

On Thu, Mar 17, 2016 at 02:58:31PM -0400, Alan Stern wrote:
> Actually, it does cause dependency issues.  The prototype example is
> fuse, when one process can't be frozen because it is waiting in the
> kernel for file I/O to complete, but the I/O involves a fuse filesystem
> and the userspace driver is already frozen.

Fuse is a clearly exceptional case where kernel internals depend on
userland for forward-progress.  I don't think this has much bearing on
the overall design.

Thanks.

-- 
tejun

Re: Should suspend plug low-level devices?

[Alan Stern]

On Thu, 17 Mar 2016, Rafael J. Wysocki wrote:

> I thought Tejun was talking about getting rid of userspace freezing.
> Sorry if that was not the case.
> 
> As I've already said for multiple times, I think that the freezing
> should really be limited to user space as a rule.  And in particular,
> if freezing of any kernel threads or workqueues etc causes problems to
> happen, the code using those freezable things is simply buggy and
> needs to be fixed.  Most likely by getting rid of the freezing from
> it.

I agree, except that I think the most likely solution is to delay the 
operations that require a freezable thing until after the system is 
resumed.

Alan Stern

Re: Should suspend plug low-level devices?

[Tejun Heo]

On Thu, Mar 17, 2016 at 02:53:42PM -0400, Alan Stern wrote:
> I agree, except that I think the most likely solution is to delay the 
> operations that require a freezable thing until after the system is 
> resumed.

Yes, by following the existing and well-established device suspend
order.

Thanks.

-- 
tejun

Re: Should suspend plug low-level devices?

[Tejun Heo]

Hello, Alan.

On Wed, Mar 16, 2016 at 11:37:35AM -0400, Alan Stern wrote:
> Doing what you suggest would require a tremendous effort.  There are

I don't know.  Is it?

> lots and lots of drivers that have no provisions for plugging; they
> would all need to be modified.  Changing the higher layers instead (to
> prevent them from generating I/O requests during suspend) seems much
> easier.

Big lock and "much easier" usually don't go together.  I get why we
started out this way - there wasn't anything working and we didn't
quite understand what the actual requirements were and wanted
something more or less working with manageable amount of effort, but I
think we're past the point where we need to address the issue
properly.  The more complex drivers are already doing most of what's
necessary and even for drivers which depend on the freezer the changes
don't need to be too difficult.

Stopping kthreads in itself is fine but the problem is that we
currently don't know who's stopping what.  Just making each freezing
explicit, say, an explicit invocation of kthread_freeze_kthread(),
wouldn't be that difficult and would go a long way.  e.g. It would
immediately solve the problem arising from kthread freezing order not
matching device dependency hierarchy.

> The original design of the PM subsystem was intended to make life
> easier for drivers by minimizing the amount of work needed to support
> suspend/resume.  Maybe the decision to do it that way was wrong, but
> it's kind of late to change the design now.

Hmmm... but that's how most subsystems evolve.  We start out with
something coarse and not quite optimal or even correct and then as we
learn more, the code and design evolve.  I frankly don't think
shifting to driver-side plugging would be *that* difficult.  It's
gonna be quite a bit of work, for sure, but things can be gradual.
For example,

* Make block layer plug bio's from upper layers.

* Visit each freezable workqueue or kthread users and convert them to
  plug explicitly.

> I foresee other kinds of problems, too.  Suppose after a suspend 
> starts, some higher layer code creates an I/O request with a short 
> timeout.  If the device weren't suspended the request would succeed -- 
> should it time out and fail now that the system is going to sleep?  
> That would make system sleep transitions non-transparent.

I can't see how freezing or not freezing would make any difference
there.  What if a kthread gets frozen with with IOs in flight?  The
timeouts would still trigger whether the issuer is frozen or not.  IO
timeouts are mostly implemented in block layer and block layer just
needs to handle it sensibly.

Thanks.

-- 
tejun

Re: Should suspend plug low-level devices?

[Alan Stern]

On Wed, 16 Mar 2016, Tejun Heo wrote:

> Hello, Alan.
> 
> On Wed, Mar 16, 2016 at 11:37:35AM -0400, Alan Stern wrote:
> > Doing what you suggest would require a tremendous effort.  There are
> 
> I don't know.  Is it?
> 
> > lots and lots of drivers that have no provisions for plugging; they
> > would all need to be modified.  Changing the higher layers instead (to
> > prevent them from generating I/O requests during suspend) seems much
> > easier.
> 
> Big lock and "much easier" usually don't go together.  I get why we
> started out this way - there wasn't anything working and we didn't
> quite understand what the actual requirements were and wanted
> something more or less working with manageable amount of effort, but I
> think we're past the point where we need to address the issue
> properly.  The more complex drivers are already doing most of what's
> necessary and even for drivers which depend on the freezer the changes
> don't need to be too difficult.
> 
> Stopping kthreads in itself is fine but the problem is that we
> currently don't know who's stopping what.  Just making each freezing
> explicit, say, an explicit invocation of kthread_freeze_kthread(),
> wouldn't be that difficult and would go a long way.  e.g. It would
> immediately solve the problem arising from kthread freezing order not
> matching device dependency hierarchy.

We're talking about workqueues in particular.  Freezing order is not 
the issue.

The problem that started this discussion was that a block device can be
hot-unplugged while the system is asleep.  The device driver's resume
routine realizes the device is gone and unregisters it.  As part of the
unregistration, the block layer (bdi_unregister) does a
flush_delayed_work.

I'm not sure exactly what work it's trying to flush (the code is
difficult to follow), but evidently it can't go forward while the
system is still resuming.  Maybe the workqueue thread is frozen.

> > The original design of the PM subsystem was intended to make life
> > easier for drivers by minimizing the amount of work needed to support
> > suspend/resume.  Maybe the decision to do it that way was wrong, but
> > it's kind of late to change the design now.
> 
> Hmmm... but that's how most subsystems evolve.  We start out with
> something coarse and not quite optimal or even correct and then as we
> learn more, the code and design evolve.  I frankly don't think
> shifting to driver-side plugging would be *that* difficult.  It's
> gonna be quite a bit of work, for sure, but things can be gradual.
> For example,
> 
> * Make block layer plug bio's from upper layers.
> 
> * Visit each freezable workqueue or kthread users and convert them to
>   plug explicitly.

How would this help the case in question?  If a workqueue is plugged 
instead of frozen, won't flush_delayed_work still deadlock?

> > I foresee other kinds of problems, too.  Suppose after a suspend 
> > starts, some higher layer code creates an I/O request with a short 
> > timeout.  If the device weren't suspended the request would succeed -- 
> > should it time out and fail now that the system is going to sleep?  
> > That would make system sleep transitions non-transparent.
> 
> I can't see how freezing or not freezing would make any difference
> there.  What if a kthread gets frozen with with IOs in flight?  The
> timeouts would still trigger whether the issuer is frozen or not.  IO
> timeouts are mostly implemented in block layer and block layer just
> needs to handle it sensibly.

kthreads (including workqueues) get frozen at controlled places.  
Presumably they are smart enough not to allow themselves to be frozen
while any I/Os are in flight.

Alan Stern

Re: Should suspend plug low-level devices?

[Tejun Heo]

Hello,

On Thu, Mar 17, 2016 at 02:51:36PM -0400, Alan Stern wrote:
> > Stopping kthreads in itself is fine but the problem is that we
> > currently don't know who's stopping what.  Just making each freezing
> > explicit, say, an explicit invocation of kthread_freeze_kthread(),
> > wouldn't be that difficult and would go a long way.  e.g. It would
> > immediately solve the problem arising from kthread freezing order not
> > matching device dependency hierarchy.
> 
> We're talking about workqueues in particular.  Freezing order is not 
> the issue.

Whether something is running on kthread or workqueue doesn't make any
difference.  Why would it?  It's about dependency chain, not what
something is running on.

> > * Make block layer plug bio's from upper layers.
> > 
> > * Visit each freezable workqueue or kthread users and convert them to
> >   plug explicitly.
> 
> How would this help the case in question?  If a workqueue is plugged 
> instead of frozen, won't flush_delayed_work still deadlock?

By plugging and unplugging in a well defined order rather than
"whatever comes first in workqueue or task lists".

> kthreads (including workqueues) get frozen at controlled places.  
> Presumably they are smart enough not to allow themselves to be frozen
> while any I/Os are in flight.

I don't think that's the main problem here.

Thanks.

-- 
tejun

Re: Should suspend plug low-level devices?

[Alan Stern]

On Fri, 18 Mar 2016, Tejun Heo wrote:

> Whether something is running on kthread or workqueue doesn't make any
> difference.  Why would it?  It's about dependency chain, not what
> something is running on.
> 
> > > * Make block layer plug bio's from upper layers.
> > > 
> > > * Visit each freezable workqueue or kthread users and convert them to
> > >   plug explicitly.
> > 
> > How would this help the case in question?  If a workqueue is plugged 
> > instead of frozen, won't flush_delayed_work still deadlock?
> 
> By plugging and unplugging in a well defined order rather than
> "whatever comes first in workqueue or task lists".

The original problem here was that some unfreezable code was waiting
(via flush_delayed_work) on frozen code, and it was blocking the
routine that would unfreeze things.  Therefore the order of freezing or
thawing has no bearing on this particular problem.

This is an example of a dependency chain where finer-grained control of
freezing will not help.  And even if we moved the I/O plugging from the
high-level routines to the low-level drivers, we could still end up
with situations where something waiting for a plugged device driver was
blocking the code that would unplug it.

Now, I have nothing against finer-grained control of freezing, although
I'm not sure how you would implement it in cases where a kthread is not
per-device but rather is per-driver or per-subsystem.  But we will 
still have to handle inversions where unfreezable code is waiting for 
frozen code.

Alan Stern

Re: Should suspend plug low-level devices?

[Tejun Heo]

Hello,

On Fri, Mar 18, 2016 at 05:06:01PM -0400, Alan Stern wrote:
> The original problem here was that some unfreezable code was waiting
> (via flush_delayed_work) on frozen code, and it was blocking the
> routine that would unfreeze things.  Therefore the order of freezing or
> thawing has no bearing on this particular problem.

Yeah, this one is caused by a high-level kernel construct being frozen
and device resuming as a whole taking place before unfreezing.  The
eventual goal would be avoiding freezing any high-level kernel
constructs as it can lead to circular dependencies as shown here.

> Now, I have nothing against finer-grained control of freezing, although
> I'm not sure how you would implement it in cases where a kthread is not
> per-device but rather is per-driver or per-subsystem.  But we will 
> still have to handle inversions where unfreezable code is waiting for 
> frozen code.

It won't be simple one-to-one conversion for all cases.  It's about
terminating suspend handling where the endpoints actually are after
all.

thanks.

-- 
tejun

Re: Should suspend plug low-level devices?

[Alan Stern]

On Sun, 20 Mar 2016, Tejun Heo wrote:

> Hello,
> 
> On Fri, Mar 18, 2016 at 05:06:01PM -0400, Alan Stern wrote:
> > The original problem here was that some unfreezable code was waiting
> > (via flush_delayed_work) on frozen code, and it was blocking the
> > routine that would unfreeze things.  Therefore the order of freezing or
> > thawing has no bearing on this particular problem.
> 
> Yeah, this one is caused by a high-level kernel construct being frozen
> and device resuming as a whole taking place before unfreezing.  The
> eventual goal would be avoiding freezing any high-level kernel
> constructs as it can lead to circular dependencies as shown here.

That's not the point I was trying to make.

Even if we avoid freezing any high-level kernel constructs, there will 
_still_ be problems if a high-level construct blocks waiting for a 
low-level driver that is plugged... which is exactly the sort of thing 
that can happen when you call flush_delayed_work().

Alan Stern

Re: Should suspend plug low-level devices?

[Tejun Heo]

Hello, Alan.

On Mon, Mar 21, 2016 at 10:38:55AM -0400, Alan Stern wrote:
> Even if we avoid freezing any high-level kernel constructs, there will 
> _still_ be problems if a high-level construct blocks waiting for a 
> low-level driver that is plugged... which is exactly the sort of thing 
> that can happen when you call flush_delayed_work().

The problem is caused by or at least made difficult to solve by having
these dependencies lumped up together.  If a device is detected to
have been removed across suspend, the device should be marked as gone
and further IO requests should be failed immediately instead of
getting blocked on filesystem layer generally blocked on global
freezer state.

Thanks.

-- 
tejun