[RFC] usb: host: u132-hcd: Remove deprecated create_singlethread_workqueue

alloc_ordered_workqueue replaces the deprecated
create_singlethread_workqueue.

The workqueue "workqueue" has multiple workitems which may require
ordering. Hence, a dedicated ordered workqueue has been used.
Since the workqueue is not being used on a memory reclaim path,
WQ_MEM_RECLAIM has not been set.

Can the workitems run concurrently? Is the ordering among work items
necessary?

Thanks.

Signed-off-by: Bhaktipriya Shridhar <[email protected]>
---
drivers/usb/host/u132-hcd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/u132-hcd.c b/drivers/usb/host/u132-hcd.c
index 43d5293..cbde189 100644
--- a/drivers/usb/host/u132-hcd.c
+++ b/drivers/usb/host/u132-hcd.c
@@ -3206,7 +3206,7 @@ static int __init u132_hcd_init(void)
if (usb_disabled())
return -ENODEV;
printk(KERN_INFO "driver %s\n", hcd_name);
- workqueue = create_singlethread_workqueue("u132");
+ workqueue = alloc_ordered_workqueue("u132", 0);
retval = platform_driver_register(&u132_platform_driver);
return retval;
}
--
2.1.4

On Wed, 2016-07-27 at 14:50 +0530, Bhaktipriya Shridhar wrote:
> The workqueue "workqueue" has multiple workitems which may require
> ordering. Hence, a dedicated ordered workqueue has been used.
> Since the workqueue is not being used on a memory reclaim path,
> WQ_MEM_RECLAIM has not been set.

That is incorrect. The work queue is used by the HCD to handle
TDs, which are parts of basic IO. The HCD in turn is used by
usb-storage and uas, which are block drivers and those are obviously
used on the memory reclaim path.

HTH
Oliver

On Wed, 2016-07-27 at 14:50 +0530, Bhaktipriya Shridhar wrote:
> The workqueue "workqueue" has multiple workitems which may require
> ordering. Hence, a dedicated ordered workqueue has been used.
> Since the workqueue is not being used on a memory reclaim path,
> WQ_MEM_RECLAIM has not been set.

That is incorrect. The work queue is used by the HCD to handle
TDs, which are parts of basic IO. The HCD in turn is used by
usb-storage and uas, which are block drivers and those are obviously
used on the memory reclaim path.

HTH
Oliver

Hello, Oliver.

On Wed, Jul 27, 2016 at 11:29:56AM +0200, Oliver Neukum wrote:
> On Wed, 2016-07-27 at 14:50 +0530, Bhaktipriya Shridhar wrote:
> > The workqueue "workqueue" has multiple workitems which may require
> > ordering. Hence, a dedicated ordered workqueue has been used.
> > Since the workqueue is not being used on a memory reclaim path,
> > WQ_MEM_RECLAIM has not been set.
>
> That is incorrect. The work queue is used by the HCD to handle
> TDs, which are parts of basic IO. The HCD in turn is used by
> usb-storage and uas, which are block drivers and those are obviously
> used on the memory reclaim path.

Hmm... I didn't know the whole USB stack could operate without
allocating memory. Does usb stack have mempools and stuff all the way
through?

Thanks.

--
tejun

On Wed, 27 Jul 2016, Tejun Heo wrote:

> Hello, Oliver.
>
> On Wed, Jul 27, 2016 at 11:29:56AM +0200, Oliver Neukum wrote:
> > On Wed, 2016-07-27 at 14:50 +0530, Bhaktipriya Shridhar wrote:
> > > The workqueue "workqueue" has multiple workitems which may require
> > > ordering. Hence, a dedicated ordered workqueue has been used.
> > > Since the workqueue is not being used on a memory reclaim path,
> > > WQ_MEM_RECLAIM has not been set.
> >
> > That is incorrect. The work queue is used by the HCD to handle
> > TDs, which are parts of basic IO. The HCD in turn is used by
> > usb-storage and uas, which are block drivers and those are obviously
> > used on the memory reclaim path.
>
> Hmm... I didn't know the whole USB stack could operate without
> allocating memory. Does usb stack have mempools and stuff all the way
> through?

No -- the USB stack does need to allocate memory in order to operate.
But it is careful to use GFP_NOIO or GFP_ATOMIC for allocations that
might be on the block-device path.

Alan Stern

Hello, Alan.

On Wed, Jul 27, 2016 at 02:54:51PM -0400, Alan Stern wrote:
> > Hmm... I didn't know the whole USB stack could operate without
> > allocating memory. Does usb stack have mempools and stuff all the way
> > through?
>
> No -- the USB stack does need to allocate memory in order to operate.
> But it is careful to use GFP_NOIO or GFP_ATOMIC for allocations that
> might be on the block-device path.

Hmm... That doesn't really make them dependable during memory reclaim.
What happens when those allocations fail?

Thanks.

--
tejun

On Wed, 27 Jul 2016, Tejun Heo wrote:

> Hello, Alan.
>
> On Wed, Jul 27, 2016 at 02:54:51PM -0400, Alan Stern wrote:
> > > Hmm... I didn't know the whole USB stack could operate without
> > > allocating memory. Does usb stack have mempools and stuff all the way
> > > through?
> >
> > No -- the USB stack does need to allocate memory in order to operate.
> > But it is careful to use GFP_NOIO or GFP_ATOMIC for allocations that
> > might be on the block-device path.
>
> Hmm... That doesn't really make them dependable during memory reclaim.

True. But it does mean that they can't cause a deadlock by waiting
indefinitely for some other memory to be paged out to the very device
they are on the access pathway for.

> What happens when those allocations fail?

The same thing that happens when any allocation fails -- the original
I/O request fails with -ENOMEM or the equivalent. In the case of
usb-storage, this is likely to trigger error recovery, which will need
to allocate memory of its own... A bad situation to get into.

Alan Stern

Hello, Alan.

On Wed, Jul 27, 2016 at 04:45:19PM -0400, Alan Stern wrote:
> > Hmm... That doesn't really make them dependable during memory reclaim.
>
> True. But it does mean that they can't cause a deadlock by waiting
> indefinitely for some other memory to be paged out to the very device
> they are on the access pathway for.
>
> > What happens when those allocations fail?
>
> The same thing that happens when any allocation fails -- the original
> I/O request fails with -ENOMEM or the equivalent. In the case of
> usb-storage, this is likely to trigger error recovery, which will need
> to allocate memory of its own... A bad situation to get into.

All that would do is deferring the deadlock, right? I'm not sure it
makes a lot of sense to protect an IO path against memory pressure
half-way. It either can be depended during memory reclaim or it
can't. The use of GFP_NOIO / ATOMIC is probably increases the chance
of IO errors under moderate memory pressure too when there are
dependable memory backing devices (and there better be) which can push
things forward if called upon.

Can MM people please chime in? The question is about USB stoage
devices and memory reclaim. USB doesn't guarantee forward progress
under memory pressure but tries a best-effort attempt with GFP_NOIO
and ATOMIC. Is this the right thing to do?

Thanks.

--
tejun

On Fri 29-07-16 09:11:47, Tejun Heo wrote:
> Hello, Alan.
>
> On Wed, Jul 27, 2016 at 04:45:19PM -0400, Alan Stern wrote:
> > > Hmm... That doesn't really make them dependable during memory reclaim.
> >
> > True. But it does mean that they can't cause a deadlock by waiting
> > indefinitely for some other memory to be paged out to the very device
> > they are on the access pathway for.
> >
> > > What happens when those allocations fail?
> >
> > The same thing that happens when any allocation fails -- the original
> > I/O request fails with -ENOMEM or the equivalent. In the case of
> > usb-storage, this is likely to trigger error recovery, which will need
> > to allocate memory of its own... A bad situation to get into.
>
> All that would do is deferring the deadlock, right? I'm not sure it
> makes a lot of sense to protect an IO path against memory pressure
> half-way. It either can be depended during memory reclaim or it
> can't.

Completely agreed! If the rescuer thread can block on a memory
allocation be it GFP_NOIO or others it is basically useless.

> The use of GFP_NOIO / ATOMIC is probably increases the chance
> of IO errors under moderate memory pressure too when there are
> dependable memory backing devices (and there better be) which can push
> things forward if called upon.
>
> Can MM people please chime in? The question is about USB stoage
> devices and memory reclaim. USB doesn't guarantee forward progress
> under memory pressure but tries a best-effort attempt with GFP_NOIO
> and ATOMIC. Is this the right thing to do?

If any real IO depends on those devices then this is not sufficient and
they need some form of guarantee for progress (aka mempool).
--
Michal Hocko
SUSE Labs

Hello,

On Mon, Aug 01, 2016 at 03:50:36PM +0200, Michal Hocko wrote:
> > All that would do is deferring the deadlock, right? I'm not sure it
> > makes a lot of sense to protect an IO path against memory pressure
> > half-way. It either can be depended during memory reclaim or it
> > can't.
>
> Completely agreed! If the rescuer thread can block on a memory
> allocation be it GFP_NOIO or others it is basically useless.
....
> > Can MM people please chime in? The question is about USB stoage
> > devices and memory reclaim. USB doesn't guarantee forward progress
> > under memory pressure but tries a best-effort attempt with GFP_NOIO
> > and ATOMIC. Is this the right thing to do?
>
> If any real IO depends on those devices then this is not sufficient and
> they need some form of guarantee for progress (aka mempool).

Oliver, Alan, what do you think? If USB itself can't operate without
allocating memory during transactions, whatever USB storage drivers
are doing isn't all that meaningful. Can we proceed with the
workqueue patches? Also, it could be that the only thing GFP_NOIO and
GFP_ATOMIC are doing is increasing the chance of IO failures under
memory pressure. Maybe it'd be a good idea to reconsider the
approach?

Thanks.

--
tejun

On Mon, 1 Aug 2016, Tejun Heo wrote:

> Hello,
>
> On Mon, Aug 01, 2016 at 03:50:36PM +0200, Michal Hocko wrote:
> > > All that would do is deferring the deadlock, right? I'm not sure it
> > > makes a lot of sense to protect an IO path against memory pressure
> > > half-way. It either can be depended during memory reclaim or it
> > > can't.
> >
> > Completely agreed! If the rescuer thread can block on a memory
> > allocation be it GFP_NOIO or others it is basically useless.
> ...
> > > Can MM people please chime in? The question is about USB stoage
> > > devices and memory reclaim. USB doesn't guarantee forward progress
> > > under memory pressure but tries a best-effort attempt with GFP_NOIO
> > > and ATOMIC. Is this the right thing to do?
> >
> > If any real IO depends on those devices then this is not sufficient and
> > they need some form of guarantee for progress (aka mempool).
>
> Oliver, Alan, what do you think? If USB itself can't operate without
> allocating memory during transactions, whatever USB storage drivers
> are doing isn't all that meaningful. Can we proceed with the
> workqueue patches? Also, it could be that the only thing GFP_NOIO and
> GFP_ATOMIC are doing is increasing the chance of IO failures under
> memory pressure. Maybe it'd be a good idea to reconsider the
> approach?

I agree that USB's approach to memory allocation won't prevent failures
when there is severe pressure.

However, is it possible that the _type_ of failure might be more
transparent? With GFP_NOIO, you end up with a cascading series of
allocation errors and it's obvious that something has gone very wrong.
If we were to switch to GFP_KERNEL, page-out could lead to deadlock
with no diagnostics (except perhaps for a watchdog warning).

Regardless, I would like to hear Oliver's thoughts.

Alan Stern

On Mon 01-08-16 14:00:57, Alan Stern wrote:
> On Mon, 1 Aug 2016, Tejun Heo wrote:
>
> > Hello,
> >
> > On Mon, Aug 01, 2016 at 03:50:36PM +0200, Michal Hocko wrote:
> > > > All that would do is deferring the deadlock, right? I'm not sure it
> > > > makes a lot of sense to protect an IO path against memory pressure
> > > > half-way. It either can be depended during memory reclaim or it
> > > > can't.
> > >
> > > Completely agreed! If the rescuer thread can block on a memory
> > > allocation be it GFP_NOIO or others it is basically useless.
> > ...
> > > > Can MM people please chime in? The question is about USB stoage
> > > > devices and memory reclaim. USB doesn't guarantee forward progress
> > > > under memory pressure but tries a best-effort attempt with GFP_NOIO
> > > > and ATOMIC. Is this the right thing to do?
> > >
> > > If any real IO depends on those devices then this is not sufficient and
> > > they need some form of guarantee for progress (aka mempool).
> >
> > Oliver, Alan, what do you think? If USB itself can't operate without
> > allocating memory during transactions, whatever USB storage drivers
> > are doing isn't all that meaningful. Can we proceed with the
> > workqueue patches? Also, it could be that the only thing GFP_NOIO and
> > GFP_ATOMIC are doing is increasing the chance of IO failures under
> > memory pressure. Maybe it'd be a good idea to reconsider the
> > approach?
>
> I agree that USB's approach to memory allocation won't prevent failures
> when there is severe pressure.

Or even worse, silent hangs for GFP_NOIO requests. If the allocation
size that is issued from that context is not large (basically < order-4)
then the allocation would be retried basically for ever without invoking
the OOM killer. Now, this is rather unlikely to become a real problem
unless there is a serious flood of these GFP_NOIO allocation requests.
But the main point remains. GFP_NOIO doesn't guanratee a forward
progress. Success of such an allocation depends on on a different
context with the full reclaim capabilities (including the OOM killer).

--
Michal Hocko
SUSE Labs

On Mon, 2016-08-01 at 10:20 -0400, Tejun Heo wrote:
> Hello,

> > If any real IO depends on those devices then this is not sufficient and
> > they need some form of guarantee for progress (aka mempool).
>
> Oliver, Alan, what do you think? If USB itself can't operate without
> allocating memory during transactions, whatever USB storage drivers

It cannot. The IO must be described to the hardware with a data
structure in memory.

> are doing isn't all that meaningful. Can we proceed with the
> workqueue patches? Also, it could be that the only thing GFP_NOIO and
> GFP_ATOMIC are doing is increasing the chance of IO failures under
> memory pressure. Maybe it'd be a good idea to reconsider the
> approach?

We had actual deadlocks with GFP_KERNEL. It seems to me that the SCSI
layer can deal with IO that cannot be completed due to a lack of memory
at least somewhat, but a deadlock within a driver would obviously be
deadly. So I don't think that mempools would remove the need for
GFP_NOIO as there are places in usbcore we cannot enter the page
laundering path from. They are an additional need.

Regards
Oliver

On Tue 02-08-16 10:06:12, Oliver Neukum wrote:
> On Mon, 2016-08-01 at 10:20 -0400, Tejun Heo wrote:
> > Hello,
>
> > > If any real IO depends on those devices then this is not sufficient and
> > > they need some form of guarantee for progress (aka mempool).
> >
> > Oliver, Alan, what do you think? If USB itself can't operate without
> > allocating memory during transactions, whatever USB storage drivers
>
> It cannot. The IO must be described to the hardware with a data
> structure in memory.
>
> > are doing isn't all that meaningful. Can we proceed with the
> > workqueue patches? Also, it could be that the only thing GFP_NOIO and
> > GFP_ATOMIC are doing is increasing the chance of IO failures under
> > memory pressure. Maybe it'd be a good idea to reconsider the
> > approach?
>
> We had actual deadlocks with GFP_KERNEL. It seems to me that the SCSI
> layer can deal with IO that cannot be completed due to a lack of memory
> at least somewhat, but a deadlock within a driver would obviously be
> deadly. So I don't think that mempools would remove the need for
> GFP_NOIO as there are places in usbcore we cannot enter the page
> laundering path from. They are an additional need.

OK, I guess there is some misunderstanding here. I believe that Tejun
wasn't arguing to drop GFP_NOIO. It might be really needed for the dead
lock avoidance. No question about that. The whole point is that
WQ_RECLAIM might be completely pointless because a rescuer wouldn't help
much if the work item would do GFP_NOIO and get stuck in the page
allocator.
--
Michal Hocko
SUSE Labs

On Tue, 2016-08-02 at 10:18 +0200, Michal Hocko wrote:
> On Tue 02-08-16 10:06:12, Oliver Neukum wrote:
> > On Mon, 2016-08-01 at 10:20 -0400, Tejun Heo wrote:
> > > Hello,
> >
> > > > If any real IO depends on those devices then this is not sufficient and
> > > > they need some form of guarantee for progress (aka mempool).
> > >
> > > Oliver, Alan, what do you think? If USB itself can't operate without
> > > allocating memory during transactions, whatever USB storage drivers
> >
> > It cannot. The IO must be described to the hardware with a data
> > structure in memory.
> >
> > > are doing isn't all that meaningful. Can we proceed with the
> > > workqueue patches? Also, it could be that the only thing GFP_NOIO and
> > > GFP_ATOMIC are doing is increasing the chance of IO failures under
> > > memory pressure. Maybe it'd be a good idea to reconsider the
> > > approach?
> >
> > We had actual deadlocks with GFP_KERNEL. It seems to me that the SCSI
> > layer can deal with IO that cannot be completed due to a lack of memory
> > at least somewhat, but a deadlock within a driver would obviously be
> > deadly. So I don't think that mempools would remove the need for
> > GFP_NOIO as there are places in usbcore we cannot enter the page
> > laundering path from. They are an additional need.
>
> OK, I guess there is some misunderstanding here. I believe that Tejun
> wasn't arguing to drop GFP_NOIO. It might be really needed for the dead
> lock avoidance. No question about that. The whole point is that
> WQ_RECLAIM might be completely pointless because a rescuer wouldn't help
> much if the work item would do GFP_NOIO and get stuck in the page
> allocator.

But that can be a problem only if the items on the work queue are
actually run and without WQ_MEM_RECLAIM that guarantee cannot be made.
We can deal with failures of memory allocation. But the requests
must actually terminate.

Regards
Oliver

On Tue 02-08-16 12:03:23, Oliver Neukum wrote:
> On Tue, 2016-08-02 at 10:18 +0200, Michal Hocko wrote:
> > On Tue 02-08-16 10:06:12, Oliver Neukum wrote:
> > > On Mon, 2016-08-01 at 10:20 -0400, Tejun Heo wrote:
> > > > Hello,
> > >
> > > > > If any real IO depends on those devices then this is not sufficient and
> > > > > they need some form of guarantee for progress (aka mempool).
> > > >
> > > > Oliver, Alan, what do you think? If USB itself can't operate without
> > > > allocating memory during transactions, whatever USB storage drivers
> > >
> > > It cannot. The IO must be described to the hardware with a data
> > > structure in memory.
> > >
> > > > are doing isn't all that meaningful. Can we proceed with the
> > > > workqueue patches? Also, it could be that the only thing GFP_NOIO and
> > > > GFP_ATOMIC are doing is increasing the chance of IO failures under
> > > > memory pressure. Maybe it'd be a good idea to reconsider the
> > > > approach?
> > >
> > > We had actual deadlocks with GFP_KERNEL. It seems to me that the SCSI
> > > layer can deal with IO that cannot be completed due to a lack of memory
> > > at least somewhat, but a deadlock within a driver would obviously be
> > > deadly. So I don't think that mempools would remove the need for
> > > GFP_NOIO as there are places in usbcore we cannot enter the page
> > > laundering path from. They are an additional need.
> >
> > OK, I guess there is some misunderstanding here. I believe that Tejun
> > wasn't arguing to drop GFP_NOIO. It might be really needed for the dead
> > lock avoidance. No question about that. The whole point is that
> > WQ_RECLAIM might be completely pointless because a rescuer wouldn't help
> > much if the work item would do GFP_NOIO and get stuck in the page
> > allocator.
>
> But that can be a problem only if the items on the work queue are
> actually run and without WQ_MEM_RECLAIM that guarantee cannot be made.
> We can deal with failures of memory allocation. But the requests
> must actually terminate.

I think you have missed my point. So let me ask differently. What is the
difference between your work item not running at all or looping
endlessly with GFP_NOIO inside the page allocator? If that particular
work item is necessary for the further progress then the system is
screwed one way or another.
--
Michal Hocko
SUSE Labs

On Tue, 2016-08-02 at 14:48 +0200, Michal Hocko wrote:
> On Tue 02-08-16 13:44:48, Oliver Neukum wrote:
> > On Tue, 2016-08-02 at 13:34 +0200, Michal Hocko wrote:
> > > On Tue 02-08-16 12:03:23, Oliver Neukum wrote:
> > > > On Tue, 2016-08-02 at 10:18 +0200, Michal Hocko wrote:
> > > > > On Tue 02-08-16 10:06:12, Oliver Neukum wrote:
> > > > > > On Mon, 2016-08-01 at 10:20 -0400, Tejun Heo wrote:
> > > > > > > Hello,
> > > > > >
> > > > > > > > If any real IO depends on those devices then this is not sufficient and
> > > > > > > > they need some form of guarantee for progress (aka mempool).
> > > > > > >
> > > > > > > Oliver, Alan, what do you think? If USB itself can't operate without
> > > > > > > allocating memory during transactions, whatever USB storage drivers
> > > > > >
> > > > > > It cannot. The IO must be described to the hardware with a data
> > > > > > structure in memory.
> > > > > >
> > > > > > > are doing isn't all that meaningful. Can we proceed with the
> > > > > > > workqueue patches? Also, it could be that the only thing GFP_NOIO and
> > > > > > > GFP_ATOMIC are doing is increasing the chance of IO failures under
> > > > > > > memory pressure. Maybe it'd be a good idea to reconsider the
> > > > > > > approach?
> > > > > >
> > > > > > We had actual deadlocks with GFP_KERNEL. It seems to me that the SCSI
> > > > > > layer can deal with IO that cannot be completed due to a lack of memory
> > > > > > at least somewhat, but a deadlock within a driver would obviously be
> > > > > > deadly. So I don't think that mempools would remove the need for
> > > > > > GFP_NOIO as there are places in usbcore we cannot enter the page
> > > > > > laundering path from. They are an additional need.
> > > > >
> > > > > OK, I guess there is some misunderstanding here. I believe that Tejun
> > > > > wasn't arguing to drop GFP_NOIO. It might be really needed for the dead
> > > > > lock avoidance. No question about that. The whole point is that
> > > > > WQ_RECLAIM might be completely pointless because a rescuer wouldn't help
> > > > > much if the work item would do GFP_NOIO and get stuck in the page
> > > > > allocator.
> > > >
> > > > But that can be a problem only if the items on the work queue are
> > > > actually run and without WQ_MEM_RECLAIM that guarantee cannot be made.
> > > > We can deal with failures of memory allocation. But the requests
> > > > must actually terminate.
> > >
> > > I think you have missed my point. So let me ask differently. What is the
> > > difference between your work item not running at all or looping
> > > endlessly with GFP_NOIO inside the page allocator? If that particular
> > > work item is necessary for the further progress then the system is
> > > screwed one way or another.
> >
> > The key difference is that I could give the right parameters to the
> > kmalloc() call. If it doesn't run, I am surely screwed. Thus I conclude
> > that WQ_RECLAIM needs to be set.
>
> Just to make sure I understand. So you will never call kmalloc with
> __GFP_DIRECT_RECLAIM from the rescuer context, right?

Apparently if that is the requirement USB will have to define its own
set of flags to use in such contexts. But still the calls as currently
executed work. Taking away WQ_MEM_RECLAIM would create danger of
introducing a regression. The issue with __GFP_DIRECT_RECLAIM already
exists and can be fixed.

Regards
Oliver