Re: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to scsi_add_host()

Re: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to scsi_add_host()

[Alan Stern]

[Added linux-scsi to CC: list.  When discussing code in a particular 
subsystem, it's a good idea to include that subsystem's mailing list in 
the CC:.]

On Tue, Dec 01, 2020 at 03:01:56AM +0800, Tom Yan wrote:
> For the record,
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/scsi/scsi_host.h?h=v5.10-rc6#n753
> 
> On Tue, 1 Dec 2020 at 02:57, Tom Yan <tom.ty89@gmail.com> wrote:
> >
> > This maybe? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_lib.c?h=v5.10-rc6#n1816
> >
> > UAS:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/storage/uas.c?h=v5.10-rc6#n918
> > BOT (AFAICT):
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/hosts.c?h=v5.10-rc6#n466
> >
> > It would explain why the issue is only triggered with UAS drives.

In brief, a recent change -- calling scsi_add_host_with_dma rather than 
scsi_add_host -- in the USB uas driver has caused a regression in 
performance.  (Note that the shost->dma_dev value is set differently as 
a result of this change.)  Hans has determined that the problem seems 
to be related to permanent changes in the dma_dev's settings caused by 
scsi_add_host_with_dma.

Tom pointed out that __scsi_init_queue contains a couple of questionable 
assignments:

	dma_set_seg_boundary(dev, shost->dma_boundary);

and

	dma_set_max_seg_size(dev, queue_max_segment_size(q));

where dev = shost->dma_dev -- in this case, a USB host controller.

So an important question is why decisions related to a particular SCSI 
host should affect the DMA settings of a device somewhere else in the 
heirarchy?  Sure, the properties of the USB controller should constrain 
the settings available to the SCSI host, but there doesn't seem to be 
any good reason for restrictions to go in the other direction.

Doesn't the way we handle DMA permit a child device to impose additional 
restrictions (such as a smaller max segment size) beyond those of the 
parent device which actually performs the DMA transfer?

> > The questions (from me) are:
> > 1. From the scsi layer POV (as per what __scsi_init_queue() does),
> > what/which should we use as dma_dev?

We should be using the USB host controller, because it is the device 
which actually performs the DMA transfers.

> > 2. Do we really need to set dma_boundary in the UAS host template (to
> > PAGE_SIZE - 1)?

I don't know.  But in theory it should be possible to have settings 
(like this one) which affect only the transfers carried out by the SCSI 
host, not the transfers carred out by other drivers which might use the 
same USB controller.

> > 3. Kind of the same question as #1: when we clamp hw_max_sectors to
> > dma max mapping size, should the size actually be "the smaller one
> > among dev and sysdev"? Or is one of the two sizes *always* the smaller
> > one?

I assume you're referring to code in the uas driver.  There the value of 
dev is meaningless as far as DMA is concerned.  Only sysdev matters.

Alan Stern

> > On Tue, 1 Dec 2020 at 02:19, Hans de Goede <hdegoede@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On 11/30/20 6:20 PM, Alan Stern wrote:
> > > > On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote:
> > > >> Hi,
> > > >>
> > > >> On 11/30/20 2:30 PM, Greg KH wrote:
> > > >>> On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote:
> > > >>>> Hi,
> > > >>>>
> > > >>>> On 11/30/20 1:58 PM, Tom Yan wrote:
> > > >>>>> It's merely a moving of comment moving for/and a no-behavioral-change
> > > >>>>> adaptation for the reversion.>
> > > >>>>
> > > >>>> IMHO the revert of the troublesome commit and the other/new changes really
> > > >>>> should be 2 separate commits. But I will let Alan and Greg have the final
> > > >>>> verdict on this.
> > > >>>
> > > >>> I would prefer to just revert the commits and not do anything
> > > >>> different/special here so late in the release cycle.
> > > >>>
> > > >>> So, if Alan agrees, I'll be glad to do them on my end, I just need the
> > > >>> commit ids for them.
> > > >>
> > > >> The troublesome commit are (in reverse, so revert, order):
> > > >>
> > > >> 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives")
> > > >> 558033c2828f ("uas: fix sdev->host->dma_dev")
> > > >> 0154012f8018 ("usb-storage: fix sdev->host->dma_dev")
> > > >>
> > > >> Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the
> > > >> last 2 patches do, with the dmadev argument of that call pointing to the device
> > > >> for the XHCI controller is causing changes to the DMA settings of the XHCI controller
> > > >> itself which is causing regressions in 5.10, see this email thread:
> > > >>
> > > >> https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t
> > > >
> > > > It's hard to go wrong with reverting, so it's okay with me.
> > > >
> > > > Still, Hans, have you checked out the difference between the
> > > > scsi_add_host() and scsi_add_host_with_dma() calls?  It's just a matter
> > > > of using dev vs. sysdev.  In particular, have you checked to see what
> > > > those two devices are on your system?
> > >
> > > Its not just dev vs sysdev, its iface->dev vs bus->sysdev, and I assume
> > > that the latter is actually the XHCI controller.
> > >
> > > my vote goes to reverting to avoid the regression for 5.10, esp. since
> > > this is a clean revert of 3 patches with nothing depending / building
> > > on top of the reverted commits.
> > >
> > > Then for 5.11 we can retry to introduce similar changes. I would be happy
> > > to try a new patch-set for 5.11.
> > >
> > > > It seems likely that if one of those calls messes up some DMA settings,
> > > > the other one does too -- just maybe not settings that matter much.
> > >
> > > I'm not very familiar with all the DMA mapping / mask code, but AFAIK making
> > > changes to the DMA settings of a child will not influence the parent.
> > >
> > > Where as when passing bus->sysdev, then changes are made to a device
> > > which is shared with other devices on the bus, which is why we see
> > > a regression in an USB NIC driver being triggered by the UAS driver
> > > binding to a device (on the same bus).
> > >
> > > At least that is my interpretation of this. I bisected the regression
> > > and that pointed at the UAS DMA change and reverting it fixes things,
> > > confirming that I did not make any mistakes during the bisect.
> > >
> > > Regards,
> > >
> > > Hans
> > >

Re: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to scsi_add_host()

[Alan Stern]

This thread seems to have fallen through the cracks.  Maybe now would be 
a good time to address the problem (since we originally planned to fix 
it in 5.11!).

The questions listed below are pretty self-contained, although the rest
of the discussion isn't.  But I never received any answers.

Alan Stern

On Mon, Nov 30, 2020 at 03:36:18PM -0500, Alan Stern wrote:
> [Added linux-scsi to CC: list.  When discussing code in a particular 
> subsystem, it's a good idea to include that subsystem's mailing list in 
> the CC:.]
> 
> On Tue, Dec 01, 2020 at 03:01:56AM +0800, Tom Yan wrote:
> > For the record,
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/scsi/scsi_host.h?h=v5.10-rc6#n753
> > 
> > On Tue, 1 Dec 2020 at 02:57, Tom Yan <tom.ty89@gmail.com> wrote:
> > >
> > > This maybe? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_lib.c?h=v5.10-rc6#n1816
> > >
> > > UAS:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/storage/uas.c?h=v5.10-rc6#n918
> > > BOT (AFAICT):
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/hosts.c?h=v5.10-rc6#n466
> > >
> > > It would explain why the issue is only triggered with UAS drives.
> 
> In brief, a recent change -- calling scsi_add_host_with_dma rather than 
> scsi_add_host -- in the USB uas driver has caused a regression in 
> performance.  (Note that the shost->dma_dev value is set differently as 
> a result of this change.)  Hans has determined that the problem seems 
> to be related to permanent changes in the dma_dev's settings caused by 
> scsi_add_host_with_dma.
> 
> Tom pointed out that __scsi_init_queue contains a couple of questionable 
> assignments:
> 
> 	dma_set_seg_boundary(dev, shost->dma_boundary);
> 
> and
> 
> 	dma_set_max_seg_size(dev, queue_max_segment_size(q));
> 
> where dev = shost->dma_dev -- in this case, a USB host controller.
> 
> So an important question is why decisions related to a particular SCSI 
> host should affect the DMA settings of a device somewhere else in the 
> heirarchy?  Sure, the properties of the USB controller should constrain 
> the settings available to the SCSI host, but there doesn't seem to be 
> any good reason for restrictions to go in the other direction.
> 
> Doesn't the way we handle DMA permit a child device to impose additional 
> restrictions (such as a smaller max segment size) beyond those of the 
> parent device which actually performs the DMA transfer?
> 
> > > The questions (from me) are:
> > > 1. From the scsi layer POV (as per what __scsi_init_queue() does),
> > > what/which should we use as dma_dev?
> 
> We should be using the USB host controller, because it is the device 
> which actually performs the DMA transfers.
> 
> > > 2. Do we really need to set dma_boundary in the UAS host template (to
> > > PAGE_SIZE - 1)?
> 
> I don't know.  But in theory it should be possible to have settings 
> (like this one) which affect only the transfers carried out by the SCSI 
> host, not the transfers carred out by other drivers which might use the 
> same USB controller.
> 
> > > 3. Kind of the same question as #1: when we clamp hw_max_sectors to
> > > dma max mapping size, should the size actually be "the smaller one
> > > among dev and sysdev"? Or is one of the two sizes *always* the smaller
> > > one?
> 
> I assume you're referring to code in the uas driver.  There the value of 
> dev is meaningless as far as DMA is concerned.  Only sysdev matters.
> 
> Alan Stern
> 
> > > On Tue, 1 Dec 2020 at 02:19, Hans de Goede <hdegoede@redhat.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 11/30/20 6:20 PM, Alan Stern wrote:
> > > > > On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote:
> > > > >> Hi,
> > > > >>
> > > > >> On 11/30/20 2:30 PM, Greg KH wrote:
> > > > >>> On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote:
> > > > >>>> Hi,
> > > > >>>>
> > > > >>>> On 11/30/20 1:58 PM, Tom Yan wrote:
> > > > >>>>> It's merely a moving of comment moving for/and a no-behavioral-change
> > > > >>>>> adaptation for the reversion.>
> > > > >>>>
> > > > >>>> IMHO the revert of the troublesome commit and the other/new changes really
> > > > >>>> should be 2 separate commits. But I will let Alan and Greg have the final
> > > > >>>> verdict on this.
> > > > >>>
> > > > >>> I would prefer to just revert the commits and not do anything
> > > > >>> different/special here so late in the release cycle.
> > > > >>>
> > > > >>> So, if Alan agrees, I'll be glad to do them on my end, I just need the
> > > > >>> commit ids for them.
> > > > >>
> > > > >> The troublesome commit are (in reverse, so revert, order):
> > > > >>
> > > > >> 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives")
> > > > >> 558033c2828f ("uas: fix sdev->host->dma_dev")
> > > > >> 0154012f8018 ("usb-storage: fix sdev->host->dma_dev")
> > > > >>
> > > > >> Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the
> > > > >> last 2 patches do, with the dmadev argument of that call pointing to the device
> > > > >> for the XHCI controller is causing changes to the DMA settings of the XHCI controller
> > > > >> itself which is causing regressions in 5.10, see this email thread:
> > > > >>
> > > > >> https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t
> > > > >
> > > > > It's hard to go wrong with reverting, so it's okay with me.
> > > > >
> > > > > Still, Hans, have you checked out the difference between the
> > > > > scsi_add_host() and scsi_add_host_with_dma() calls?  It's just a matter
> > > > > of using dev vs. sysdev.  In particular, have you checked to see what
> > > > > those two devices are on your system?
> > > >
> > > > Its not just dev vs sysdev, its iface->dev vs bus->sysdev, and I assume
> > > > that the latter is actually the XHCI controller.
> > > >
> > > > my vote goes to reverting to avoid the regression for 5.10, esp. since
> > > > this is a clean revert of 3 patches with nothing depending / building
> > > > on top of the reverted commits.
> > > >
> > > > Then for 5.11 we can retry to introduce similar changes. I would be happy
> > > > to try a new patch-set for 5.11.
> > > >
> > > > > It seems likely that if one of those calls messes up some DMA settings,
> > > > > the other one does too -- just maybe not settings that matter much.
> > > >
> > > > I'm not very familiar with all the DMA mapping / mask code, but AFAIK making
> > > > changes to the DMA settings of a child will not influence the parent.
> > > >
> > > > Where as when passing bus->sysdev, then changes are made to a device
> > > > which is shared with other devices on the bus, which is why we see
> > > > a regression in an USB NIC driver being triggered by the UAS driver
> > > > binding to a device (on the same bus).
> > > >
> > > > At least that is my interpretation of this. I bisected the regression
> > > > and that pointed at the UAS DMA change and reverting it fixes things,
> > > > confirming that I did not make any mistakes during the bisect.
> > > >
> > > > Regards,
> > > >
> > > > Hans
> > > >

Re: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to scsi_add_host()

[Christoph Hellwig]

On Thu, Feb 25, 2021 at 11:35:57AM -0500, Alan Stern wrote:
> This thread seems to have fallen through the cracks.  Maybe now would be 
> a good time to address the problem (since we originally planned to fix 
> it in 5.11!).
> 
> The questions listed below are pretty self-contained, although the rest
> of the discussion isn't.  But I never received any answers.

usb-storage must use scsi_add_host_with_dma to use the right device
for DMA mapping and parameters.  The calls to set the DMA options
on the device are needed so that IOMMU merging doesn't change the
imposed requirements.  If these requirements slow you down you need
to relax them, as apparently the hardware is able to handle bigger
limits.

Re: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to scsi_add_host()

[Alan Stern]

On Fri, Feb 26, 2021 at 06:53:52AM +0100, Christoph Hellwig wrote:
> On Thu, Feb 25, 2021 at 11:35:57AM -0500, Alan Stern wrote:
> > This thread seems to have fallen through the cracks.  Maybe now would be 
> > a good time to address the problem (since we originally planned to fix 
> > it in 5.11!).
> > 
> > The questions listed below are pretty self-contained, although the rest
> > of the discussion isn't.  But I never received any answers.
> 
> usb-storage must use scsi_add_host_with_dma to use the right device
> for DMA mapping and parameters.  The calls to set the DMA options
> on the device are needed so that IOMMU merging doesn't change the
> imposed requirements.  If these requirements slow you down you need
> to relax them, as apparently the hardware is able to handle bigger
> limits.

Hans, don't you have the right equipment to test this approach?

Alan Stern