Hi,
somebody reported frequent "Out of SW-IOMMU space" failures when copying
between a FireWire and a USB disk. swiotlb=512M on the kernel command
line fixes it for him. https://bugzilla.redhat.com/show_bug.cgi?id=478567
Should the default be raised?
--
Stefan Richter
-=====-==--= ---= =-=--
http://arcgraph.de/sr/
On Tue, 20 Jan 2009 21:10:48 +0100
Stefan Richter <[email protected]> wrote:
> somebody reported frequent "Out of SW-IOMMU space" failures when copying
> between a FireWire and a USB disk. swiotlb=512M on the kernel command
> line fixes it for him. https://bugzilla.redhat.com/show_bug.cgi?id=478567
>
> Should the default be raised?
I don't think so. Reserving large memory like 512MB for swiotlb? It
means that you waste 512MB if you use decent HBAs (having no dma mask
limit) or fast storage, or when there is few I/O activity.
The bug reporter said that copying stooped but it should not
happen. It doesn't happen with SCSI (copying can continue a bit
slowly). dma mapping errors are transient so SCSI retries.
I guess that it would be better to remove the misleading error
message. Sane IOMMUs don't do that.
If the bug report is true, then the FireWire stack or the driver (or
both) has problems. Make sure that FireWire can work even with dma
mapping failures. My dma mapping debug patchset might be useful:
git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git dmafault
You can inject dma mapping failures per device:
vine:/debug/pci/0000:00:1d.1# ls
interval probability space task-filter times verbose
FUJITA Tomonori wrote:
> The bug reporter said that copying stooped but it should not
> happen. It doesn't happen with SCSI (copying can continue a bit
> slowly). dma mapping errors are transient so SCSI retries.
...
> If the bug report is true, then the FireWire stack or the driver (or
> both) has problems. Make sure that FireWire can work even with dma
> mapping failures.
sbp2_scsi_queuecommand returns SCSI_MLQUEUE_HOST_BUSY if DMA mapping
failed. Isn't this what should happen?
However, both usb-storage and firewire-sbp2 currently have a queudepth
of only 1; if there are no DMA resources to map just this one SCSI
request, how should the system be able to recover? It can wait, but it
can't lower the part of the workload which is related to this particular
copying operation (which, as the reporter wrote, ultimately stopped). I
suppose there is something else* on the reporter's system which tied up
too much swiotlb resources the whole time; and then just waiting a bit
until the next queucommand won't get things going.
*) The report does not sound like there was a DMA mappig leak caused by
copying between usb-storage and firewire-sbp2. Else he would have hit
the problem again even with increased swiotlb default size.
> My dma mapping debug patchset might be useful:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git dmafault
>
> You can inject dma mapping failures per device:
>
> vine:/debug/pci/0000:00:1d.1# ls
> interval probability space task-filter times verbose
This could be handy, thanks.
--
Stefan Richter
-=====-==--= ---= =-=-=
http://arcgraph.de/sr/
On Wed, 21 Jan 2009 08:56:31 +0100
Stefan Richter <[email protected]> wrote:
> FUJITA Tomonori wrote:
> > The bug reporter said that copying stooped but it should not
> > happen. It doesn't happen with SCSI (copying can continue a bit
> > slowly). dma mapping errors are transient so SCSI retries.
> ...
> > If the bug report is true, then the FireWire stack or the driver (or
> > both) has problems. Make sure that FireWire can work even with dma
> > mapping failures.
>
> sbp2_scsi_queuecommand returns SCSI_MLQUEUE_HOST_BUSY if DMA mapping
> failed. Isn't this what should happen?
Returning SCSI_MLQUEUE_HOST_BUSY is the right thing and such problem
should not happen.
> However, both usb-storage and firewire-sbp2 currently have a queudepth
> of only 1; if there are no DMA resources to map just this one SCSI
> request, how should the system be able to recover?
Handling one outstanding command is must. If you can't, the system
can deadlock in OOM.
If you put the deadlock issue aside, the above host->can_queue issue
is irrelevant. Even if you set host->can_queue to 1, scsi-ml sends one
command to the LLD again and again until it succeeds, I think. As long
as the LLD can send a command occasionally, the system works.
> It can wait, but it
> can't lower the part of the workload which is related to this particular
> copying operation (which, as the reporter wrote, ultimately stopped). I
> suppose there is something else* on the reporter's system which tied up
> too much swiotlb resources the whole time; and then just waiting a bit
Tying swiotlb resource the whole time is unlikely. Everyone uses it
temporarily.
> until the next queucommand won't get things going.
>
> *) The report does not sound like there was a DMA mappig leak caused by
> copying between usb-storage and firewire-sbp2. Else he would have hit
> the problem again even with increased swiotlb default size.
Maybe the reporter doesn't copy enough to hit the deadlock.
If you need 512MB swiotlb buffer, surely it's something wrong. The
kernel should work smoothly with much less (even if it works slowly).
On Tue, 27 Jan 2009 16:54:30 +0900
FUJITA Tomonori <[email protected]> wrote:
> > *) The report does not sound like there was a DMA mappig leak caused by
> > copying between usb-storage and firewire-sbp2. Else he would have hit
> > the problem again even with increased swiotlb default size.
>
> Maybe the reporter doesn't copy enough to hit the deadlock.
Hmm, we don't have leak here?
=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] fw-sbp2: fix dma mapping leak on the failure path
Signed-off-by: FUJITA Tomonori <[email protected]>
---
drivers/firewire/fw-sbp2.c | 12 +++++++++++-
1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c
index e88d506..d40910d 100644
--- a/drivers/firewire/fw-sbp2.c
+++ b/drivers/firewire/fw-sbp2.c
@@ -1491,8 +1491,18 @@ static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done)
orb->base.request_bus =
dma_map_single(device->card->device, &orb->request,
sizeof(orb->request), DMA_TO_DEVICE);
- if (dma_mapping_error(device->card->device, orb->base.request_bus))
+ if (dma_mapping_error(device->card->device, orb->base.request_bus)) {
+ if (scsi_sg_count(cmd)) {
+ dma_unmap_sg(device->card->device, scsi_sglist(cmd),
+ scsi_sg_count(cmd),
+ cmd->sc_data_direction);
+ dma_unmap_single(device->card->device,
+ orb->page_table_bus,
+ sizeof(orb->page_table),
+ DMA_TO_DEVICE);
+ }
goto out;
+ }
sbp2_send_orb(&orb->base, lu, lu->tgt->node_id, generation,
lu->command_block_agent_address + SBP2_ORB_POINTER);
--
1.6.0.6
On 27 Jan, FUJITA Tomonori wrote at LSML and LKML:
[I wrote]
>> > *) The report does not sound like there was a DMA mappig leak caused by
>> > copying between usb-storage and firewire-sbp2. Else he would have hit
>> > the problem again even with increased swiotlb default size.
> Hmm, we don't have leak here?
>
> =
> From: FUJITA Tomonori <[email protected]>
> Subject: [PATCH] fw-sbp2: fix dma mapping leak on the failure path
>
> Signed-off-by: FUJITA Tomonori <[email protected]>
> ---
> drivers/firewire/fw-sbp2.c | 12 +++++++++++-
> 1 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c
> index e88d506..d40910d 100644
> --- a/drivers/firewire/fw-sbp2.c
> +++ b/drivers/firewire/fw-sbp2.c
> @@ -1491,8 +1491,18 @@ static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done)
> orb->base.request_bus =
> dma_map_single(device->card->device, &orb->request,
> sizeof(orb->request), DMA_TO_DEVICE);
> - if (dma_mapping_error(device->card->device, orb->base.request_bus))
> + if (dma_mapping_error(device->card->device, orb->base.request_bus)) {
> + if (scsi_sg_count(cmd)) {
> + dma_unmap_sg(device->card->device, scsi_sglist(cmd),
> + scsi_sg_count(cmd),
> + cmd->sc_data_direction);
> + dma_unmap_single(device->card->device,
> + orb->page_table_bus,
> + sizeof(orb->page_table),
> + DMA_TO_DEVICE);
> + }
> goto out;
> + }
>
> sbp2_send_orb(&orb->base, lu, lu->tgt->node_id, generation,
> lu->command_block_agent_address + SBP2_ORB_POINTER);
Yes, we do have a leak there. Thanks a lot for checking the code.
There is one thing missing in the fix: Even with scsi_sg_count(cmd) !=
0, there is sometimes no orb->page_table to unmap. It's when the
scatterlist was mapped into a single physical segment.
From: Stefan Richter <[email protected]>
Subject: firewire: sbp2: fix DMA mapping leak on the failure path
Reported-by: FUJITA Tomonori <[email protected]>
who also provided a first version of the fix.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/fw-sbp2.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -1323,6 +1323,19 @@ static unsigned int sbp2_status_to_sense
}
}
+static void sbp2_unmap_scatterlist(struct device *card_device,
+ struct sbp2_command_orb *orb)
+{
+ if (scsi_sg_count(orb->cmd))
+ dma_unmap_sg(card_device, scsi_sglist(orb->cmd),
+ scsi_sg_count(orb->cmd),
+ orb->cmd->sc_data_direction);
+
+ if (orb->page_table_bus)
+ dma_unmap_single(card_device, orb->page_table_bus,
+ sizeof(orb->page_table), DMA_TO_DEVICE);
+}
+
static void complete_command_orb(struct sbp2_orb *base_orb,
struct sbp2_status *status)
{
@@ -1364,15 +1377,7 @@ static void complete_command_orb(struct
dma_unmap_single(device->card->device, orb->base.request_bus,
sizeof(orb->request), DMA_TO_DEVICE);
-
- if (scsi_sg_count(orb->cmd) > 0)
- dma_unmap_sg(device->card->device, scsi_sglist(orb->cmd),
- scsi_sg_count(orb->cmd),
- orb->cmd->sc_data_direction);
-
- if (orb->page_table_bus != 0)
- dma_unmap_single(device->card->device, orb->page_table_bus,
- sizeof(orb->page_table), DMA_TO_DEVICE);
+ sbp2_unmap_scatterlist(device->card->device, orb);
orb->cmd->result = result;
orb->done(orb->cmd);
@@ -1493,8 +1498,10 @@ static int sbp2_scsi_queuecommand(struct
orb->base.request_bus =
dma_map_single(device->card->device, &orb->request,
sizeof(orb->request), DMA_TO_DEVICE);
- if (dma_mapping_error(device->card->device, orb->base.request_bus))
+ if (dma_mapping_error(device->card->device, orb->base.request_bus)) {
+ sbp2_unmap_scatterlist(device->card->device, orb);
goto out;
+ }
sbp2_send_orb(&orb->base, lu, lu->tgt->node_id, generation,
lu->command_block_agent_address + SBP2_ORB_POINTER);
--
Stefan Richter
-=====-==--= ---= ==-==
http://arcgraph.de/sr/
On Tue, 27 Jan 2009 18:03:40 +0100 (CET)
Stefan Richter <[email protected]> wrote:
> On 27 Jan, FUJITA Tomonori wrote at LSML and LKML:
> [I wrote]
> >> > *) The report does not sound like there was a DMA mappig leak caused by
> >> > copying between usb-storage and firewire-sbp2. Else he would have hit
> >> > the problem again even with increased swiotlb default size.
>
> > Hmm, we don't have leak here?
> >
> > =
> > From: FUJITA Tomonori <[email protected]>
> > Subject: [PATCH] fw-sbp2: fix dma mapping leak on the failure path
> >
> > Signed-off-by: FUJITA Tomonori <[email protected]>
> > ---
> > drivers/firewire/fw-sbp2.c | 12 +++++++++++-
> > 1 files changed, 11 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c
> > index e88d506..d40910d 100644
> > --- a/drivers/firewire/fw-sbp2.c
> > +++ b/drivers/firewire/fw-sbp2.c
> > @@ -1491,8 +1491,18 @@ static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done)
> > orb->base.request_bus =
> > dma_map_single(device->card->device, &orb->request,
> > sizeof(orb->request), DMA_TO_DEVICE);
> > - if (dma_mapping_error(device->card->device, orb->base.request_bus))
> > + if (dma_mapping_error(device->card->device, orb->base.request_bus)) {
> > + if (scsi_sg_count(cmd)) {
> > + dma_unmap_sg(device->card->device, scsi_sglist(cmd),
> > + scsi_sg_count(cmd),
> > + cmd->sc_data_direction);
> > + dma_unmap_single(device->card->device,
> > + orb->page_table_bus,
> > + sizeof(orb->page_table),
> > + DMA_TO_DEVICE);
> > + }
> > goto out;
> > + }
> >
> > sbp2_send_orb(&orb->base, lu, lu->tgt->node_id, generation,
> > lu->command_block_agent_address + SBP2_ORB_POINTER);
>
> Yes, we do have a leak there. Thanks a lot for checking the code.
>
> There is one thing missing in the fix: Even with scsi_sg_count(cmd) !=
> 0, there is sometimes no orb->page_table to unmap. It's when the
> scatterlist was mapped into a single physical segment.
>
>
> From: Stefan Richter <[email protected]>
> Subject: firewire: sbp2: fix DMA mapping leak on the failure path
>
> Reported-by: FUJITA Tomonori <[email protected]>
> who also provided a first version of the fix.
>
> Signed-off-by: Stefan Richter <[email protected]>
> ---
> drivers/firewire/fw-sbp2.c | 27 +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> Index: linux/drivers/firewire/fw-sbp2.c
> ===================================================================
> --- linux.orig/drivers/firewire/fw-sbp2.c
> +++ linux/drivers/firewire/fw-sbp2.c
> @@ -1323,6 +1323,19 @@ static unsigned int sbp2_status_to_sense
> }
> }
>
> +static void sbp2_unmap_scatterlist(struct device *card_device,
> + struct sbp2_command_orb *orb)
> +{
> + if (scsi_sg_count(orb->cmd))
> + dma_unmap_sg(card_device, scsi_sglist(orb->cmd),
> + scsi_sg_count(orb->cmd),
> + orb->cmd->sc_data_direction);
> +
> + if (orb->page_table_bus)
> + dma_unmap_single(card_device, orb->page_table_bus,
> + sizeof(orb->page_table), DMA_TO_DEVICE);
Well, this does not look correct since zero can be a valid dma
address.
static void sbp2_unmap_scatterlist(struct device *card_device,
struct sbp2_command_orb *orb)
{
if (scsi_sg_count(orb->cmd))
dma_unmap_sg(card_device, scsi_sglist(orb->cmd),
scsi_sg_count(orb->cmd),
orb->cmd->sc_data_direction);
if (scsi_sg_count(orb->cmd) > 1)
dma_unmap_single(card_device, orb->page_table_bus,
sizeof(orb->page_table), DMA_TO_DEVICE);
Well, looks cranky a bit but firewire is cranky for me :)
FUJITA Tomonori wrote:
> On Tue, 27 Jan 2009 18:03:40 +0100 (CET)
> Stefan Richter <[email protected]> wrote:
>> +static void sbp2_unmap_scatterlist(struct device *card_device,
>> + struct sbp2_command_orb *orb)
>> +{
>> + if (scsi_sg_count(orb->cmd))
>> + dma_unmap_sg(card_device, scsi_sglist(orb->cmd),
>> + scsi_sg_count(orb->cmd),
>> + orb->cmd->sc_data_direction);
>> +
>> + if (orb->page_table_bus)
>> + dma_unmap_single(card_device, orb->page_table_bus,
>> + sizeof(orb->page_table), DMA_TO_DEVICE);
>
> Well, this does not look correct since zero can be a valid dma
> address.
Hmm.
> static void sbp2_unmap_scatterlist(struct device *card_device,
> struct sbp2_command_orb *orb)
> {
> if (scsi_sg_count(orb->cmd))
> dma_unmap_sg(card_device, scsi_sglist(orb->cmd),
> scsi_sg_count(orb->cmd),
> orb->cmd->sc_data_direction);
>
> if (scsi_sg_count(orb->cmd) > 1)
> dma_unmap_single(card_device, orb->page_table_bus,
> sizeof(orb->page_table), DMA_TO_DEVICE);
>
>
> Well, looks cranky a bit but firewire is cranky for me :)
Even if scsi_sg_count(cmd) > 1, the outcome of dma_map_sg() could have
been 1, couldn't it? So I need to take note of that in another way.
--
Stefan Richter
-=====-==--= ---= ===--
http://arcgraph.de/sr/
On Wed, 28 Jan 2009 00:46:13 +0100
Stefan Richter <[email protected]> wrote:
> FUJITA Tomonori wrote:
> > On Tue, 27 Jan 2009 18:03:40 +0100 (CET)
> > Stefan Richter <[email protected]> wrote:
> >> +static void sbp2_unmap_scatterlist(struct device *card_device,
> >> + struct sbp2_command_orb *orb)
> >> +{
> >> + if (scsi_sg_count(orb->cmd))
> >> + dma_unmap_sg(card_device, scsi_sglist(orb->cmd),
> >> + scsi_sg_count(orb->cmd),
> >> + orb->cmd->sc_data_direction);
> >> +
> >> + if (orb->page_table_bus)
> >> + dma_unmap_single(card_device, orb->page_table_bus,
> >> + sizeof(orb->page_table), DMA_TO_DEVICE);
> >
> > Well, this does not look correct since zero can be a valid dma
> > address.
>
> Hmm.
>
> > static void sbp2_unmap_scatterlist(struct device *card_device,
> > struct sbp2_command_orb *orb)
> > {
> > if (scsi_sg_count(orb->cmd))
> > dma_unmap_sg(card_device, scsi_sglist(orb->cmd),
> > scsi_sg_count(orb->cmd),
> > orb->cmd->sc_data_direction);
> >
> > if (scsi_sg_count(orb->cmd) > 1)
> > dma_unmap_single(card_device, orb->page_table_bus,
> > sizeof(orb->page_table), DMA_TO_DEVICE);
> >
> >
> > Well, looks cranky a bit but firewire is cranky for me :)
>
> Even if scsi_sg_count(cmd) > 1, the outcome of dma_map_sg() could have
> been 1, couldn't it? So I need to take note of that in another way.
Oops, right. Probably, you need to store the return value of
dma_map_sg in orb.
> FUJITA Tomonori wrote:
>> On Tue, 27 Jan 2009 18:03:40 +0100 (CET)
>> Stefan Richter <[email protected]> wrote:
>>> + if (orb->page_table_bus)
>>> + dma_unmap_single(card_device, orb->page_table_bus,
>>> + sizeof(orb->page_table), DMA_TO_DEVICE);
>>
>> Well, this does not look correct since zero can be a valid dma
>> address.
From: Stefan Richter <[email protected]>
Subject: firewire: sbp2: fix DMA mapping leak on the failure path
Reported-by: FUJITA Tomonori <[email protected]>
who also provided a first version of the fix.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/fw-sbp2.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -1323,6 +1323,19 @@ static unsigned int sbp2_status_to_sense
}
}
+static void sbp2_unmap_scatterlist(struct device *card_device,
+ struct sbp2_command_orb *orb)
+{
+ if (scsi_sg_count(orb->cmd))
+ dma_unmap_sg(card_device, scsi_sglist(orb->cmd),
+ scsi_sg_count(orb->cmd),
+ orb->cmd->sc_data_direction);
+
+ if (orb->request.misc & cpu_to_be32(COMMAND_ORB_PAGE_TABLE_PRESENT))
+ dma_unmap_single(card_device, orb->page_table_bus,
+ sizeof(orb->page_table), DMA_TO_DEVICE);
+}
+
static void complete_command_orb(struct sbp2_orb *base_orb,
struct sbp2_status *status)
{
@@ -1364,15 +1377,7 @@ static void complete_command_orb(struct
dma_unmap_single(device->card->device, orb->base.request_bus,
sizeof(orb->request), DMA_TO_DEVICE);
-
- if (scsi_sg_count(orb->cmd) > 0)
- dma_unmap_sg(device->card->device, scsi_sglist(orb->cmd),
- scsi_sg_count(orb->cmd),
- orb->cmd->sc_data_direction);
-
- if (orb->page_table_bus != 0)
- dma_unmap_single(device->card->device, orb->page_table_bus,
- sizeof(orb->page_table), DMA_TO_DEVICE);
+ sbp2_unmap_scatterlist(device->card->device, orb);
orb->cmd->result = result;
orb->done(orb->cmd);
@@ -1493,8 +1498,10 @@ static int sbp2_scsi_queuecommand(struct
orb->base.request_bus =
dma_map_single(device->card->device, &orb->request,
sizeof(orb->request), DMA_TO_DEVICE);
- if (dma_mapping_error(device->card->device, orb->base.request_bus))
+ if (dma_mapping_error(device->card->device, orb->base.request_bus)) {
+ sbp2_unmap_scatterlist(device->card->device, orb);
goto out;
+ }
sbp2_send_orb(&orb->base, lu, lu->tgt->node_id, generation,
lu->command_block_agent_address + SBP2_ORB_POINTER);
--
Stefan Richter
-=====-==--= ---= ===--
http://arcgraph.de/sr/
On Wed, 28 Jan 2009 01:03:34 +0100 (CET)
Stefan Richter <[email protected]> wrote:
> > FUJITA Tomonori wrote:
> >> On Tue, 27 Jan 2009 18:03:40 +0100 (CET)
> >> Stefan Richter <[email protected]> wrote:
> >>> + if (orb->page_table_bus)
> >>> + dma_unmap_single(card_device, orb->page_table_bus,
> >>> + sizeof(orb->page_table), DMA_TO_DEVICE);
> >>
> >> Well, this does not look correct since zero can be a valid dma
> >> address.
>
>
> From: Stefan Richter <[email protected]>
> Subject: firewire: sbp2: fix DMA mapping leak on the failure path
>
> Reported-by: FUJITA Tomonori <[email protected]>
> who also provided a first version of the fix.
>
> Signed-off-by: Stefan Richter <[email protected]>
> ---
> drivers/firewire/fw-sbp2.c | 27 +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
It's time to ask him to test this patch with the default or _less_
swiotlb resource, I guess.