linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
@ 2019-05-17 16:02 Jaewon Kim
  2019-05-17 16:34 ` Matthew Wilcox
  2019-05-20  5:56 ` Christoph Hellwig
  0 siblings, 2 replies; 20+ messages in thread
From: Jaewon Kim @ 2019-05-17 16:02 UTC (permalink / raw)
  To: gregkh, m.szyprowski, linux-mm, linux-usb, linux-kernel
  Cc: Jaewon Kim, ytk.lee

Hello I don't have enough knowledge on USB core but I've wondered
why GFP_NOIO has been used in xhci_alloc_dev for
xhci_alloc_virt_device. I found commit ("a6d940dd759b xhci: Use
GFP_NOIO during device reset"). But can we just change GFP_NOIO
to __GFP_RECLAIM | __GFP_FS ?

Please refer to below case.

I got a report from Lee YongTaek <ytk.lee@samsung.com> that the
xhci_alloc_virt_device was too slow over 2 seconds only for one page
allocation.

1) It was because kernel version was v4.14 and DMA allocation was
done from CMA(Contiguous Memory Allocator) where CMA region was
almost filled with file page and  CMA passes GFP down to page
isolation. And the page isolation only allows file page isolation only to
requests having __GFP_FS.

2) Historically CMA was changed at v4.19 to use GFP_KERNEL
regardless of GFP passed to  DMA allocation through the
commit 6518202970c1 "(mm/cma: remove unsupported gfp_mask
parameter from cma_alloc()".

I think pre v4.19 the xhci_alloc_virt_device could be very slow
depending on CMA situation but free to USB deadlock issue. But as of
v4.19, I think, it will be fast but can face the deadlock issue.
Consequently I think to meet the both cases, I think USB can pass
__GFP_FS without __GFP_IO.

If __GFP_FS is passed from USB core, of course, the CMA patch also
need to be changed to pass GFP.

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 005e65922608..38abcd03a1a2 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3893,7 +3893,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct
usb_device *udev)
         * xhci_discover_or_reset_device(), which may be called as part of
         * mass storage driver error handling.
         */
-       if (!xhci_alloc_virt_device(xhci, slot_id, udev, GFP_NOIO)) {
+       if (!xhci_alloc_virt_device(xhci, slot_id, udev, __GFP_RECLAIM
| __GFP_FS)) {
                xhci_warn(xhci, "Could not allocate xHCI USB device
data structures\n");
                goto disable_slot;
        }


Thank you

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-17 16:02 [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation Jaewon Kim
@ 2019-05-17 16:34 ` Matthew Wilcox
  2019-05-18  1:53   ` Jaewon Kim
  2019-05-20  5:56 ` Christoph Hellwig
  1 sibling, 1 reply; 20+ messages in thread
From: Matthew Wilcox @ 2019-05-17 16:34 UTC (permalink / raw)
  To: Jaewon Kim
  Cc: gregkh, m.szyprowski, linux-mm, linux-usb, linux-kernel,
	Jaewon Kim, ytk.lee

On Sat, May 18, 2019 at 01:02:28AM +0900, Jaewon Kim wrote:
> Hello I don't have enough knowledge on USB core but I've wondered
> why GFP_NOIO has been used in xhci_alloc_dev for
> xhci_alloc_virt_device. I found commit ("a6d940dd759b xhci: Use
> GFP_NOIO during device reset"). But can we just change GFP_NOIO
> to __GFP_RECLAIM | __GFP_FS ?

No.  __GFP_FS implies __GFP_IO; you can't set __GFP_FS and clear __GFP_IO.

It seems like the problem you have is using the CMA to do DMA allocation.
Why would you do such a thing?

> Please refer to below case.
> 
> I got a report from Lee YongTaek <ytk.lee@samsung.com> that the
> xhci_alloc_virt_device was too slow over 2 seconds only for one page
> allocation.
> 
> 1) It was because kernel version was v4.14 and DMA allocation was
> done from CMA(Contiguous Memory Allocator) where CMA region was
> almost filled with file page and  CMA passes GFP down to page
> isolation. And the page isolation only allows file page isolation only to
> requests having __GFP_FS.
> 
> 2) Historically CMA was changed at v4.19 to use GFP_KERNEL
> regardless of GFP passed to  DMA allocation through the
> commit 6518202970c1 "(mm/cma: remove unsupported gfp_mask
> parameter from cma_alloc()".
> 
> I think pre v4.19 the xhci_alloc_virt_device could be very slow
> depending on CMA situation but free to USB deadlock issue. But as of
> v4.19, I think, it will be fast but can face the deadlock issue.
> Consequently I think to meet the both cases, I think USB can pass
> __GFP_FS without __GFP_IO.
> 
> If __GFP_FS is passed from USB core, of course, the CMA patch also
> need to be changed to pass GFP.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-17 16:34 ` Matthew Wilcox
@ 2019-05-18  1:53   ` Jaewon Kim
  0 siblings, 0 replies; 20+ messages in thread
From: Jaewon Kim @ 2019-05-18  1:53 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: gregkh, m.szyprowski, linux-mm, linux-usb, linux-kernel,
	Jaewon Kim, ytk.lee

Thank you for your comment.

In ARM64 architecture, default CMA region is commonly activated and it
could be used
if no specific memory region is defined. The USB driver in my platform
also uses the
default CMA region as DMA allocation. If using the CMA to do DMA
allocation is improper,
then do you think that the USB driver for my platform should be
changed not to use CMA?

According to my understanding, in CONFIG_DMA_CMA on both v4.14 and v5.0,
__GFP_DIRECT_RECLAIM will try CMA allocation first instead of normal
buddy allocation.
Then it will get default CMA region through dev_get_cma_area API.
Please refer to
dev_get_cma_area code below though I am using v4.14 for my platform.

git show v5.0:include/linux/dma-contiguous.h
 61 #ifdef CONFIG_DMA_CMA
 62
 63 extern struct cma *dma_contiguous_default_area;
 64
 65 static inline struct cma *dev_get_cma_area(struct device *dev)
 66 {
 67         if (dev && dev->cma_area)
 68                 return dev->cma_area;
 69         return dma_contiguous_default_area;
 70 }

Thank you

2019년 5월 18일 (토) 오전 1:34, Matthew Wilcox <willy@infradead.org>님이 작성:
>
> On Sat, May 18, 2019 at 01:02:28AM +0900, Jaewon Kim wrote:
> > Hello I don't have enough knowledge on USB core but I've wondered
> > why GFP_NOIO has been used in xhci_alloc_dev for
> > xhci_alloc_virt_device. I found commit ("a6d940dd759b xhci: Use
> > GFP_NOIO during device reset"). But can we just change GFP_NOIO
> > to __GFP_RECLAIM | __GFP_FS ?
>
> No.  __GFP_FS implies __GFP_IO; you can't set __GFP_FS and clear __GFP_IO.
>
> It seems like the problem you have is using the CMA to do DMA allocation.
> Why would you do such a thing?
>
> > Please refer to below case.
> >
> > I got a report from Lee YongTaek <ytk.lee@samsung.com> that the
> > xhci_alloc_virt_device was too slow over 2 seconds only for one page
> > allocation.
> >
> > 1) It was because kernel version was v4.14 and DMA allocation was
> > done from CMA(Contiguous Memory Allocator) where CMA region was
> > almost filled with file page and  CMA passes GFP down to page
> > isolation. And the page isolation only allows file page isolation only to
> > requests having __GFP_FS.
> >
> > 2) Historically CMA was changed at v4.19 to use GFP_KERNEL
> > regardless of GFP passed to  DMA allocation through the
> > commit 6518202970c1 "(mm/cma: remove unsupported gfp_mask
> > parameter from cma_alloc()".
> >
> > I think pre v4.19 the xhci_alloc_virt_device could be very slow
> > depending on CMA situation but free to USB deadlock issue. But as of
> > v4.19, I think, it will be fast but can face the deadlock issue.
> > Consequently I think to meet the both cases, I think USB can pass
> > __GFP_FS without __GFP_IO.
> >
> > If __GFP_FS is passed from USB core, of course, the CMA patch also
> > need to be changed to pass GFP.
>
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-17 16:02 [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation Jaewon Kim
  2019-05-17 16:34 ` Matthew Wilcox
@ 2019-05-20  5:56 ` Christoph Hellwig
  2019-05-20  9:09   ` Oliver Neukum
  2019-05-23 12:32   ` Oliver Neukum
  1 sibling, 2 replies; 20+ messages in thread
From: Christoph Hellwig @ 2019-05-20  5:56 UTC (permalink / raw)
  To: Jaewon Kim
  Cc: gregkh, m.szyprowski, linux-mm, linux-usb, linux-kernel,
	Jaewon Kim, ytk.lee

Folks, you can't just pass arbitary GFP_ flags to dma allocation
routines, beause very often they are not just wrappers around
the page allocator.

So no, you can't just fine grained control the flags, but the
existing code is just as buggy.

Please switch to use memalloc_noio_save() instead.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-20  5:56 ` Christoph Hellwig
@ 2019-05-20  9:09   ` Oliver Neukum
  2019-05-20 10:12     ` Christoph Hellwig
  2019-05-23 12:32   ` Oliver Neukum
  1 sibling, 1 reply; 20+ messages in thread
From: Oliver Neukum @ 2019-05-20  9:09 UTC (permalink / raw)
  To: Christoph Hellwig, Jaewon Kim
  Cc: linux-mm, gregkh, Jaewon Kim, m.szyprowski, ytk.lee,
	linux-kernel, linux-usb

On So, 2019-05-19 at 22:56 -0700, Christoph Hellwig wrote:
> Folks, you can't just pass arbitary GFP_ flags to dma allocation
> routines, beause very often they are not just wrappers around
> the page allocator.
> 
> So no, you can't just fine grained control the flags, but the
> existing code is just as buggy.
> 
> Please switch to use memalloc_noio_save() instead.
> 

Hi,

we actually do. It is just higher up in the calling path:

int usb_reset_device(struct usb_device *udev)
{
        int ret;
        int i;
        unsigned int noio_flag;
        struct usb_port *port_dev;
        struct usb_host_config *config = udev->actconfig;
        struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent);

        if (udev->state == USB_STATE_NOTATTACHED ||
                        udev->state == USB_STATE_SUSPENDED) {
                dev_dbg(&udev->dev, "device reset not allowed in state %d\n",
                                udev->state);
                return -EINVAL;
        }

        if (!udev->parent) {
                /* this requires hcd-specific logic; see ohci_restart() */
                dev_dbg(&udev->dev, "%s for root hub!\n", __func__);
                return -EISDIR;
        }

        port_dev = hub->ports[udev->portnum - 1];

        /*
         * Don't allocate memory with GFP_KERNEL in current
         * context to avoid possible deadlock if usb mass
         * storage interface or usbnet interface(iSCSI case)
         * is included in current configuration. The easist
         * approach is to do it for every device reset,
         * because the device 'memalloc_noio' flag may have
         * not been set before reseting the usb device.
         */
        noio_flag = memalloc_noio_save();

So, do we need to audit the mem_flags again?
What are we supposed to use? GFP_KERNEL?

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-20  9:09   ` Oliver Neukum
@ 2019-05-20 10:12     ` Christoph Hellwig
  2019-05-20 14:16       ` Alan Stern
  0 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2019-05-20 10:12 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Christoph Hellwig, Jaewon Kim, linux-mm, gregkh, Jaewon Kim,
	m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Mon, May 20, 2019 at 11:09:25AM +0200, Oliver Neukum wrote:
> we actually do. It is just higher up in the calling path:

Perfect!

> So, do we need to audit the mem_flags again?
> What are we supposed to use? GFP_KERNEL?

GFP_KERNEL if you can block, GFP_ATOMIC if you can't for a good reason,
that is the allocation is from irq context or under a spinlock.  If you
think you have a case where you think you don't want to block, but it
is not because of the above reasons we need to have a chat about the
details.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-20 10:12     ` Christoph Hellwig
@ 2019-05-20 14:16       ` Alan Stern
  2019-05-20 14:23         ` Christoph Hellwig
  2019-05-21 13:11         ` Oliver Neukum
  0 siblings, 2 replies; 20+ messages in thread
From: Alan Stern @ 2019-05-20 14:16 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Oliver Neukum, Jaewon Kim, linux-mm, gregkh, Jaewon Kim,
	m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Mon, 20 May 2019, Christoph Hellwig wrote:

> On Mon, May 20, 2019 at 11:09:25AM +0200, Oliver Neukum wrote:
> > we actually do. It is just higher up in the calling path:
> 
> Perfect!
> 
> > So, do we need to audit the mem_flags again?
> > What are we supposed to use? GFP_KERNEL?
> 
> GFP_KERNEL if you can block, GFP_ATOMIC if you can't for a good reason,
> that is the allocation is from irq context or under a spinlock.  If you
> think you have a case where you think you don't want to block, but it
> is not because of the above reasons we need to have a chat about the
> details.

What if the allocation requires the kernel to swap some old pages out 
to the backing store, but the backing store is on the device that the 
driver is managing?  The swap can't take place until the current I/O 
operation is complete (assuming the driver can handle only one I/O 
operation at a time), and the current operation can't complete until 
the old pages are swapped out.  Result: deadlock.

Isn't that the whole reason for using GFP_NOIO in the first place?

Alan Stern


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-20 14:16       ` Alan Stern
@ 2019-05-20 14:23         ` Christoph Hellwig
  2019-05-21  8:54           ` Oliver Neukum
  2019-05-21 13:11         ` Oliver Neukum
  1 sibling, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2019-05-20 14:23 UTC (permalink / raw)
  To: Alan Stern
  Cc: Christoph Hellwig, Oliver Neukum, Jaewon Kim, linux-mm, gregkh,
	Jaewon Kim, m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Mon, May 20, 2019 at 10:16:57AM -0400, Alan Stern wrote:
> What if the allocation requires the kernel to swap some old pages out 
> to the backing store, but the backing store is on the device that the 
> driver is managing?  The swap can't take place until the current I/O 
> operation is complete (assuming the driver can handle only one I/O 
> operation at a time), and the current operation can't complete until 
> the old pages are swapped out.  Result: deadlock.
> 
> Isn't that the whole reason for using GFP_NOIO in the first place?

It is, or rather was.  As it has been incredibly painful to wire
up the gfp_t argument through some callstacks, most notably the
vmalloc allocator which is used by a lot of the DMA allocators on
non-coherent platforms, we now have the memalloc_noio_save and
memalloc_nofs_save functions that mark a thread as not beeing to
go into I/O / FS reclaim.  So even if you use GFP_KERNEL you will
not dip into reclaim with those flags set on the thread.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-20 14:23         ` Christoph Hellwig
@ 2019-05-21  8:54           ` Oliver Neukum
  2019-05-21 13:27             ` Christoph Hellwig
  0 siblings, 1 reply; 20+ messages in thread
From: Oliver Neukum @ 2019-05-21  8:54 UTC (permalink / raw)
  To: Christoph Hellwig, Alan Stern
  Cc: Jaewon Kim, linux-mm, gregkh, Jaewon Kim, m.szyprowski, ytk.lee,
	linux-kernel, linux-usb

On Mo, 2019-05-20 at 07:23 -0700, Christoph Hellwig wrote:
> On Mon, May 20, 2019 at 10:16:57AM -0400, Alan Stern wrote:
> > What if the allocation requires the kernel to swap some old pages out 
> > to the backing store, but the backing store is on the device that the 
> > driver is managing?  The swap can't take place until the current I/O 
> > operation is complete (assuming the driver can handle only one I/O 
> > operation at a time), and the current operation can't complete until 
> > the old pages are swapped out.  Result: deadlock.
> > 
> > Isn't that the whole reason for using GFP_NOIO in the first place?
> 
> It is, or rather was.  As it has been incredibly painful to wire
> up the gfp_t argument through some callstacks, most notably the
> vmalloc allocator which is used by a lot of the DMA allocators on
> non-coherent platforms, we now have the memalloc_noio_save and
> memalloc_nofs_save functions that mark a thread as not beeing to
> go into I/O / FS reclaim.  So even if you use GFP_KERNEL you will
> not dip into reclaim with those flags set on the thread.

OK, but this leaves a question open. Will the GFP_NOIO actually
hurt, if it is used after memalloc_noio_save()?

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-20 14:16       ` Alan Stern
  2019-05-20 14:23         ` Christoph Hellwig
@ 2019-05-21 13:11         ` Oliver Neukum
  2019-05-21 14:00           ` Alan Stern
  1 sibling, 1 reply; 20+ messages in thread
From: Oliver Neukum @ 2019-05-21 13:11 UTC (permalink / raw)
  To: Alan Stern, Christoph Hellwig
  Cc: Jaewon Kim, linux-mm, gregkh, Jaewon Kim, m.szyprowski, ytk.lee,
	linux-kernel, linux-usb

On Mo, 2019-05-20 at 10:16 -0400, Alan Stern wrote:
> On Mon, 20 May 2019, Christoph Hellwig wrote:
> 
> > GFP_KERNEL if you can block, GFP_ATOMIC if you can't for a good reason,
> > that is the allocation is from irq context or under a spinlock.  If you
> > think you have a case where you think you don't want to block, but it
> > is not because of the above reasons we need to have a chat about the
> > details.
> 
> What if the allocation requires the kernel to swap some old pages out 
> to the backing store, but the backing store is on the device that the 
> driver is managing?  The swap can't take place until the current I/O 
> operation is complete (assuming the driver can handle only one I/O 
> operation at a time), and the current operation can't complete until 
> the old pages are swapped out.  Result: deadlock.
> 
> Isn't that the whole reason for using GFP_NOIO in the first place?

Hi,

lookig at this it seems to me that we are in danger of a deadlock

- during reset - devices cannot do IO while being reset
	covered by the USB layer in usb_reset_device
- resume & restore - devices cannot do IO while suspended
	covered by driver core in rpm_callback
- disconnect - a disconnected device cannot do IO
	is this a theoretical case or should I do something to
	the driver core?

How about changing configurations on USB?

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-21  8:54           ` Oliver Neukum
@ 2019-05-21 13:27             ` Christoph Hellwig
  0 siblings, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2019-05-21 13:27 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Christoph Hellwig, Alan Stern, Jaewon Kim, linux-mm, gregkh,
	Jaewon Kim, m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Tue, May 21, 2019 at 10:54:37AM +0200, Oliver Neukum wrote:
> OK, but this leaves a question open. Will the GFP_NOIO actually
> hurt, if it is used after memalloc_noio_save()?

Unless we have a bug somewhere it should not make any difference,
neither positively nor negatively.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-21 13:11         ` Oliver Neukum
@ 2019-05-21 14:00           ` Alan Stern
  2019-05-22  6:31             ` Oliver Neukum
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Stern @ 2019-05-21 14:00 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Christoph Hellwig, Jaewon Kim, linux-mm, gregkh, Jaewon Kim,
	m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Tue, 21 May 2019, Oliver Neukum wrote:

> On Mo, 2019-05-20 at 10:16 -0400, Alan Stern wrote:
> > On Mon, 20 May 2019, Christoph Hellwig wrote:
> > 
> > > GFP_KERNEL if you can block, GFP_ATOMIC if you can't for a good reason,
> > > that is the allocation is from irq context or under a spinlock.  If you
> > > think you have a case where you think you don't want to block, but it
> > > is not because of the above reasons we need to have a chat about the
> > > details.
> > 
> > What if the allocation requires the kernel to swap some old pages out 
> > to the backing store, but the backing store is on the device that the 
> > driver is managing?  The swap can't take place until the current I/O 
> > operation is complete (assuming the driver can handle only one I/O 
> > operation at a time), and the current operation can't complete until 
> > the old pages are swapped out.  Result: deadlock.
> > 
> > Isn't that the whole reason for using GFP_NOIO in the first place?
> 
> Hi,
> 
> lookig at this it seems to me that we are in danger of a deadlock
> 
> - during reset - devices cannot do IO while being reset
> 	covered by the USB layer in usb_reset_device
> - resume & restore - devices cannot do IO while suspended
> 	covered by driver core in rpm_callback
> - disconnect - a disconnected device cannot do IO
> 	is this a theoretical case or should I do something to
> 	the driver core?
> 
> How about changing configurations on USB?

Changing configurations amounts to much the same as disconnecting,
because both operations destroy all the existing interfaces.

Disconnect can arise in two different ways.

	Physical hot-unplug: All I/O operations will fail.

	Rmmod or unbind: I/O operations will succeed.

The second case is probably okay.  The first we can do nothing about.  
However, in either case we do need to make sure that memory allocations
do not require any writebacks.  This suggests that we need to call
memalloc_noio_save() from within usb_unbind_interface().

Alan Stern


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-21 14:00           ` Alan Stern
@ 2019-05-22  6:31             ` Oliver Neukum
  2019-05-22 14:56               ` Alan Stern
  0 siblings, 1 reply; 20+ messages in thread
From: Oliver Neukum @ 2019-05-22  6:31 UTC (permalink / raw)
  To: Alan Stern
  Cc: Jaewon Kim, Christoph Hellwig, linux-mm, gregkh, Jaewon Kim,
	m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Di, 2019-05-21 at 10:00 -0400, Alan Stern wrote:
> 
> Changing configurations amounts to much the same as disconnecting,
> because both operations destroy all the existing interfaces.
> 
> Disconnect can arise in two different ways.
> 
>         Physical hot-unplug: All I/O operations will fail.
> 
>         Rmmod or unbind: I/O operations will succeed.
> 
> The second case is probably okay.  The first we can do nothing about.  
> However, in either case we do need to make sure that memory allocations
> do not require any writebacks.  This suggests that we need to call
> memalloc_noio_save() from within usb_unbind_interface().

I agree with the problem, but I fail to see why this issue would be
specific to USB. Shouldn't this be done in the device core layer?

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-22  6:31             ` Oliver Neukum
@ 2019-05-22 14:56               ` Alan Stern
  2019-05-22 20:47                 ` Oliver Neukum
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Stern @ 2019-05-22 14:56 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Jaewon Kim, Christoph Hellwig, linux-mm, gregkh, Jaewon Kim,
	m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Wed, 22 May 2019, Oliver Neukum wrote:

> On Di, 2019-05-21 at 10:00 -0400, Alan Stern wrote:
> > 
> > Changing configurations amounts to much the same as disconnecting,
> > because both operations destroy all the existing interfaces.
> > 
> > Disconnect can arise in two different ways.
> > 
> >         Physical hot-unplug: All I/O operations will fail.
> > 
> >         Rmmod or unbind: I/O operations will succeed.
> > 
> > The second case is probably okay.  The first we can do nothing about.  
> > However, in either case we do need to make sure that memory allocations
> > do not require any writebacks.  This suggests that we need to call
> > memalloc_noio_save() from within usb_unbind_interface().
> 
> I agree with the problem, but I fail to see why this issue would be
> specific to USB. Shouldn't this be done in the device core layer?

Only for drivers that are on the block-device writeback path.  The 
device core doesn't know which drivers these are.

Alan Stern


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-22 14:56               ` Alan Stern
@ 2019-05-22 20:47                 ` Oliver Neukum
  2019-05-23 14:01                   ` Alan Stern
  0 siblings, 1 reply; 20+ messages in thread
From: Oliver Neukum @ 2019-05-22 20:47 UTC (permalink / raw)
  To: Alan Stern
  Cc: Jaewon Kim, Christoph Hellwig, linux-mm, gregkh, Jaewon Kim,
	m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Mi, 2019-05-22 at 10:56 -0400, Alan Stern wrote:
> On Wed, 22 May 2019, Oliver Neukum wrote:
> 
> > I agree with the problem, but I fail to see why this issue would be
> > specific to USB. Shouldn't this be done in the device core layer?
> 
> Only for drivers that are on the block-device writeback path.  The 
> device core doesn't know which drivers these are.

Neither does USB know. It is very hard to predict or even tell which
devices are block device drivers. I think we must assume that
any device may be affected.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-20  5:56 ` Christoph Hellwig
  2019-05-20  9:09   ` Oliver Neukum
@ 2019-05-23 12:32   ` Oliver Neukum
  2019-05-23 16:35     ` Christoph Hellwig
  1 sibling, 1 reply; 20+ messages in thread
From: Oliver Neukum @ 2019-05-23 12:32 UTC (permalink / raw)
  To: Christoph Hellwig, Jaewon Kim
  Cc: linux-mm, gregkh, Jaewon Kim, m.szyprowski, ytk.lee,
	linux-kernel, linux-usb

On So, 2019-05-19 at 22:56 -0700, Christoph Hellwig wrote:
> Folks, you can't just pass arbitary GFP_ flags to dma allocation
> routines, beause very often they are not just wrappers around
> the page allocator.
> 
> So no, you can't just fine grained control the flags, but the
> existing code is just as buggy.
> 
> Please switch to use memalloc_noio_save() instead.

Thinking about this again, we have a problem. We introduced
memalloc_noio_save() in 3.10 . Hence the code should have been
correct in v4.14. Which means that either
6518202970c1 "(mm/cma: remove unsupported gfp_mask
parameter from cma_alloc()"
is buggy, or the original issue with a delay of 2 seconds
still exist.

Do we need to do something?

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-22 20:47                 ` Oliver Neukum
@ 2019-05-23 14:01                   ` Alan Stern
  2019-05-28 12:34                     ` Oliver Neukum
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Stern @ 2019-05-23 14:01 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Jaewon Kim, Christoph Hellwig, linux-mm, gregkh, Jaewon Kim,
	m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Wed, 22 May 2019, Oliver Neukum wrote:

> On Mi, 2019-05-22 at 10:56 -0400, Alan Stern wrote:
> > On Wed, 22 May 2019, Oliver Neukum wrote:
> > 
> > > I agree with the problem, but I fail to see why this issue would be
> > > specific to USB. Shouldn't this be done in the device core layer?
> > 
> > Only for drivers that are on the block-device writeback path.  The 
> > device core doesn't know which drivers these are.
> 
> Neither does USB know. It is very hard to predict or even tell which
> devices are block device drivers. I think we must assume that
> any device may be affected.

All right.  Would you like to submit a patch?

Alan Stern


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-23 12:32   ` Oliver Neukum
@ 2019-05-23 16:35     ` Christoph Hellwig
  0 siblings, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2019-05-23 16:35 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Christoph Hellwig, Jaewon Kim, linux-mm, gregkh, Jaewon Kim,
	m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Thu, May 23, 2019 at 02:32:09PM +0200, Oliver Neukum wrote:
> > Please switch to use memalloc_noio_save() instead.
> 
> Thinking about this again, we have a problem. We introduced
> memalloc_noio_save() in 3.10 . Hence the code should have been
> correct in v4.14. Which means that either
> 6518202970c1 "(mm/cma: remove unsupported gfp_mask
> parameter from cma_alloc()"
> is buggy, or the original issue with a delay of 2 seconds
> still exist.
> 
> Do we need to do something?

cma_alloc calls into alloc_contig_range to do the actual allocation,
which then calls current_gfp_context() to pick up the adjustments
from memalloc_noio_save and friends.  So at least in current mainline
we should be fine.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-23 14:01                   ` Alan Stern
@ 2019-05-28 12:34                     ` Oliver Neukum
  2019-05-28 14:25                       ` Alan Stern
  0 siblings, 1 reply; 20+ messages in thread
From: Oliver Neukum @ 2019-05-28 12:34 UTC (permalink / raw)
  To: Alan Stern
  Cc: Jaewon Kim, Christoph Hellwig, linux-mm, gregkh, Jaewon Kim,
	m.szyprowski, ytk.lee, linux-kernel, linux-usb

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

Am Donnerstag, den 23.05.2019, 10:01 -0400 schrieb Alan Stern:
> On Wed, 22 May 2019, Oliver Neukum wrote:
> 
> > On Mi, 2019-05-22 at 10:56 -0400, Alan Stern wrote:
> > > On Wed, 22 May 2019, Oliver Neukum wrote:
> > > 
> > > > I agree with the problem, but I fail to see why this issue would be
> > > > specific to USB. Shouldn't this be done in the device core layer?
> > > 
> > > Only for drivers that are on the block-device writeback path.  The 
> > > device core doesn't know which drivers these are.
> > 
> > Neither does USB know. It is very hard to predict or even tell which
> > devices are block device drivers. I think we must assume that
> > any device may be affected.
> 
> All right.  Would you like to submit a patch?

Do you like this one?

	Regards
		Oliver

[-- Attachment #2: 0001-base-force-NOIO-allocations-during-unplug.patch --]
[-- Type: text/x-patch, Size: 1522 bytes --]

From 0dc9c7dfe994fc9c28a63ba283e4442c237f6989 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Tue, 28 May 2019 11:43:02 +0200
Subject: [PATCH] base: force NOIO allocations during unplug

There is one overlooked situation under which a driver
must not do IO to allocate memory. You cannot do that
while disconnecting a device. A device being disconnected
is no longer functional in most cases, yet IO may fail
only when the handler runs.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/base/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index fd7511e04e62..a7f5f45bd761 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2229,6 +2229,7 @@ void device_del(struct device *dev)
 	struct device *parent = dev->parent;
 	struct kobject *glue_dir = NULL;
 	struct class_interface *class_intf;
+	unsigned int noio_flag;
 
 	/*
 	 * Hold the device lock and set the "dead" flag to guarantee that
@@ -2256,6 +2257,7 @@ void device_del(struct device *dev)
 		device_remove_sys_dev_entry(dev);
 		device_remove_file(dev, &dev_attr_dev);
 	}
+	noio_flag = memalloc_noio_save();
 	if (dev->class) {
 		device_remove_class_symlinks(dev);
 
@@ -2277,6 +2279,8 @@ void device_del(struct device *dev)
 	device_platform_notify(dev, KOBJ_REMOVE);
 	device_remove_properties(dev);
 	device_links_purge(dev);
+	memalloc_noio_restore(noio_flag);
+
 
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation
  2019-05-28 12:34                     ` Oliver Neukum
@ 2019-05-28 14:25                       ` Alan Stern
  0 siblings, 0 replies; 20+ messages in thread
From: Alan Stern @ 2019-05-28 14:25 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Jaewon Kim, Christoph Hellwig, linux-mm, gregkh, Jaewon Kim,
	m.szyprowski, ytk.lee, linux-kernel, linux-usb

On Tue, 28 May 2019, Oliver Neukum wrote:

> Am Donnerstag, den 23.05.2019, 10:01 -0400 schrieb Alan Stern:
> > On Wed, 22 May 2019, Oliver Neukum wrote:
> > 
> > > On Mi, 2019-05-22 at 10:56 -0400, Alan Stern wrote:
> > > > On Wed, 22 May 2019, Oliver Neukum wrote:
> > > > 
> > > > > I agree with the problem, but I fail to see why this issue would be
> > > > > specific to USB. Shouldn't this be done in the device core layer?
> > > > 
> > > > Only for drivers that are on the block-device writeback path.  The 
> > > > device core doesn't know which drivers these are.
> > > 
> > > Neither does USB know. It is very hard to predict or even tell which
> > > devices are block device drivers. I think we must assume that
> > > any device may be affected.
> > 
> > All right.  Would you like to submit a patch?
> 
> Do you like this one?

Hmmm.  I might be inclined to move the start of the I/O-protected
region a little earlier.  For example, the first
blocking_notifier_call_chain() might result in some memory allocations.

The end is okay; once bus_remove_device() has returned the driver will 
be completely unbound, so there shouldn't be any pending I/O through 
the device.

Alan Stern


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-05-28 14:25 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-17 16:02 [RFC PATCH] usb: host: xhci: allow __GFP_FS in dma allocation Jaewon Kim
2019-05-17 16:34 ` Matthew Wilcox
2019-05-18  1:53   ` Jaewon Kim
2019-05-20  5:56 ` Christoph Hellwig
2019-05-20  9:09   ` Oliver Neukum
2019-05-20 10:12     ` Christoph Hellwig
2019-05-20 14:16       ` Alan Stern
2019-05-20 14:23         ` Christoph Hellwig
2019-05-21  8:54           ` Oliver Neukum
2019-05-21 13:27             ` Christoph Hellwig
2019-05-21 13:11         ` Oliver Neukum
2019-05-21 14:00           ` Alan Stern
2019-05-22  6:31             ` Oliver Neukum
2019-05-22 14:56               ` Alan Stern
2019-05-22 20:47                 ` Oliver Neukum
2019-05-23 14:01                   ` Alan Stern
2019-05-28 12:34                     ` Oliver Neukum
2019-05-28 14:25                       ` Alan Stern
2019-05-23 12:32   ` Oliver Neukum
2019-05-23 16:35     ` Christoph Hellwig

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