libata .sg_tablesize: why always dividing by 2 ?
Jeff,
We had a discussion here today about IOMMUs,
and they *never* split sg list entries -- they only ever *merge*.
And this happens only after the block layer has already done merging while respecting q->seg_boundary_mask.
So worst case, the IOMMU may merge everything, and then in libata we unmerge them again. But the end result can never exceed the max_sg_entries limit enforced by the block layer.
So.. why are we still specifying .sg_tablesize as half of what the LLD can really handle?
This can cost a lot of memory, as using NCQ effectively multiplies everything by 32..
Based on this information, I should be able to do this in sata_mv, for example:
- .sg_tablesize = MV_MAX_SG_CT / 2, + .sg_tablesize = MV_MAX_SG_CT,
???
Quoted 11 lines · Mark Lord
Jeff,
We had a discussion here today about IOMMUs,
and they *never* split sg list entries -- they only ever *merge*.And this happens only after the block layer has already done merging while respecting
q->seg_boundary_mask.So worst case, the IOMMU may merge everything, and then in libata we unmerge them again. But the end result can never exceed the
max_sg_entrieslimit enforced by the block layer.
<shrug> Early experience said otherwise. The split in foo_fill_sg() and resulting sg_tablesize reduction were both needed to successfully transfer data, when Ben H originally did the work.
If Ben H and everyone on the arch side agrees with the above analysis, I would be quite happy to remove all those "/ 2".
Quoted 2 lines
This can cost a lot of memory, as using NCQ effectively multiplies everything by 32..
I recommend dialing down the hyperbole a bit :)
"a lot" in this case is... maybe another page or two per table, if that. Compared with everything else in the system going on, with 16-byte S/G entries, S/G table size is really the least of our worries.
If you were truly concerned about memory usage in sata_mv, a more effective route is simply reducing MV_MAX_SG_CT to a number closer to the average s/g table size -- which is far, far lower than 256 (currently MV_MAX_SG_CT), or even 128 (MV_MAX_SG_CT/2).
Or moving to a scheme where you allocate (for example) S/G tables with 32 entries... then allocate on the fly for the rare case where the S/G table must be larger.
Memory usage is simply not an effective argument in this case. Safety and correctness are far more paramount.
Jeff
Quoted 18 lines · Jeff Garzik
Quoted 11 lines · Mark Lord
Jeff,
We had a discussion here today about IOMMUs,
and they *never* split sg list entries -- they only ever *merge*.And this happens only after the block layer has already done merging while respecting
q->seg_boundary_mask.So worst case, the IOMMU may merge everything, and then in libata we unmerge them again. But the end result can never exceed the
max_sg_entrieslimit enforced by the block layer.<shrug> Early experience said otherwise. The split in
foo_fill_sg()and resultingsg_tablesizereduction were both needed to successfully transfer data, when Ben H originally did the work.If Ben H and everyone on the arch side agrees with the above analysis, I would be quite happy to remove all those "/ 2".
Quoted 6 lines
Quoted 2 lines
This can cost a lot of memory, as using NCQ effectively multiplies everything by 32..
I recommend dialing down the hyperbole a bit :)
"a lot" in this case is... maybe another page or two per table, if that. Compared with everything else in the system going on, with 16-byte S/G entries, S/G table size is really the least of our worries.
..
Well, today each sg table is about a page in size, and sata_mv has 32 of them per port.
So cutting them in half would save 16 pages per port, or 64 pages per host controller.
That's a lot for a small system, but maybe not for my 4GB test boxes.
Quoted 3 lines
If you were truly concerned about memory usage in
sata_mv, a more effective route is simply reducingMV_MAX_SG_CTto a number closer to the average s/g table size -- which is far, far lower than 256 (currentlyMV_MAX_SG_CT), or even 128 (MV_MAX_SG_CT/2).Or moving to a scheme where you allocate (for example) S/G tables with 32 entries... then allocate on the fly for the rare case where the S/G table must be larger...
..
Oh, absolutely.. that's on my "clean up" list once the rest of
the driver becomes stable and mostly done. But for now, "safety and correctness is far more paramount" in sata_mv. :)
Cheers
Quoted 13 lines · Jeff Garzik
Quoted 11 lines · Mark Lord
Jeff,
We had a discussion here today about IOMMUs,
and they *never* split sg list entries -- they only ever *merge*.And this happens only after the block layer has already done merging while respecting
q->seg_boundary_mask.So worst case, the IOMMU may merge everything, and then in libata we unmerge them again. But the end result can never exceed the
max_sg_entrieslimit enforced by the block layer.
Quoted 6 lines
<shrug> Early experience said otherwise. The split in
foo_fill_sg()and resultingsg_tablesizereduction were both needed to successfully transfer data, when Ben H originally did the work.If Ben H and everyone on the arch side agrees with the above analysis, I would be quite happy to remove all those "/ 2".
The split wasn't done by the iommu. The split was done by the IDE code itself to handle the stupid 64k crossing thingy. If it's done differently now, it might be possible to remove it, I haven't looked.
Cheers,
Ben.
Quoted 13 lines · Benjamin Herrenschmidt
Quoted 12 lines · Jeff Garzik
Quoted 11 lines · Mark Lord
Jeff,
We had a discussion here today about IOMMUs,
and they *never* split sg list entries -- they only ever *merge*.And this happens only after the block layer has already done merging while respecting
q->seg_boundary_mask.So worst case, the IOMMU may merge everything, and then in libata we unmerge them again. But the end result can never exceed the
max_sg_entrieslimit enforced by the block layer.
Quoted 7 lines
Quoted 3 lines
<shrug> Early experience said otherwise. The split in
foo_fill_sg()and resultingsg_tablesizereduction were both needed to successfully transfer data, when Ben H originally did the work.If Ben H and everyone on the arch side agrees with the above analysis, I would be quite happy to remove all those "/ 2".
The split wasn't done by the iommu. The split was done by the IDE code itself to handle the stupid 64k crossing thingy. If it's done differently now, it might be possible to remove it, I haven't looked.
..
The block layer uses seg_boundary_mask to ensure that we never have to split them again in the LLD.
A very long time ago, when I wrote the IDE DMA code, this was not the case.
Cheers
Quoted 1 line · Mark Lord
Benjamin Herrenschmidt wrote:
..
Quoted 7 lines
Quoted 3 lines
The split wasn't done by the iommu. The split was done by the IDE code itself to handle the stupid 64k crossing thingy. If it's done differently now, it might be possible to remove it, I haven't looked.
..
The block layer uses
seg_boundary_maskto ensure that we never have to split them again in the LLD.
..
James B. suggests that we stick a WARN_ON() into libata to let us know if that precondition is violated. Sounds like an easy thing to do for a couple of -rc cycles someday.
Cheers
Quoted 4 lines
James B. suggests that we stick a
WARN_ON()into libata to let us know if that precondition is violated. Sounds like an easy thing to do for a couple of -rc cycles someday.
If the block layer gives us a 32k block aligned on a 32k boundary (aligned), we have no guarantee that the iommu will not turn that into something unaligned crossing a 32k (and thus possibly a 64k) boundary.
On powerpc, the iommu operates on 4k pages and only provides that level of alignment to dma_map_sg() (dma_alloc_coherent are naturally aligned, but not map_sg, that would put way too much pressure on the allocator on machines that have pinhole-sized iommu space).
Cheers,
Ben.
Quoted 7 lines · Benjamin Herrenschmidt
Quoted 3 lines
James B. suggests that we stick a
WARN_ON()into libata to let us know if that precondition is violated. Sounds like an easy thing to do for a couple of -rc cycles someday.If the block layer gives us a 32k block aligned on a 32k boundary (aligned), we have no guarantee that the iommu will not turn that into something unaligned crossing a 32k (and thus possibly a 64k) boundary.
..
Certainly, but never any worse than what the block layer gave originally.
The important note being: IOMMU only ever *merges*, it never *splits*.
Which means that, by the time we split up any mis-merges again for 64K crossings, we can never have more SG segments than what the block layer originally fed to the IOMMU stuff.
Or so the IOMMU and SCSI experts here at LSF'08 have assured me, even after my own skeptical questioning.
Cheers
Quoted 5 lines · Mark Lord
Quoted 4 lines · Benjamin Herrenschmidt
Quoted 3 lines
James B. suggests that we stick a
WARN_ON()into libata to let us know if that precondition is violated. Sounds like an easy thing to do for a couple of -rc cycles someday.
Quoted 8 lines
Quoted 3 lines
If the block layer gives us a 32k block aligned on a 32k boundary (aligned), we have no guarantee that the iommu will not turn that into something unaligned crossing a 32k (and thus possibly a 64k) boundary.
..
Certainly, but never any worse than what the block layer gave originally.
The important note being: IOMMU only ever *merges*, it never *splits*.
Yes, but it will also change the address and doesn't guarantee the alignment.
Quoted 6 lines
Which means that, by the time we split up any mis-merges again for 64K crossings, we can never have more SG segments than what the block layer originally fed to the IOMMU stuff.
Or so the IOMMU and SCSI experts here at LSF'08 have assured me, even after my own skeptical questioning.
I suppose so. I don't remember all of the details, but iirc, it has to do with crossing 64K boundaries. Some controllers can't handle it.
It's not only the _size_ of the segments, it's their alignment.
The iommu will not keep alignement beyond the page size (and even then... on powerpc with a 64k base page size, you may still end up with a 4k aligned result, but let's not go there now).
So that means that even if your block layer gives you nice aligned less than 64k segments that don't cross 64k boundaries, and even if your iommu isn't doing any merging at all, it may still give you back things that do not respect that 64k alignment boundary, might cross them, and thus might need to be split.
Now, it would make sense (if we don't have it already) to have a flag provided by the host controller that tells us whether it suffers from that limitation, and if not, we get avoid the whole thing.
Ben.
Quoted 24 lines · Benjamin Herrenschmidt
Quoted 4 lines · Mark Lord
Quoted 3 lines · Benjamin Herrenschmidt
Quoted 3 lines
James B. suggests that we stick a
WARN_ON()into libata to let us know if that precondition is violated. Sounds like an easy thing to do for a couple of -rc cycles someday.Quoted 8 lines
Quoted 3 lines
If the block layer gives us a 32k block aligned on a 32k boundary (aligned), we have no guarantee that the iommu will not turn that into something unaligned crossing a 32k (and thus possibly a 64k) boundary.
..
Certainly, but never any worse than what the block layer gave originally.
The important note being: IOMMU only ever *merges*, it never *splits*.
Yes, but it will also change the address and doesn't guarantee the alignment.
Quoted 6 lines
Which means that, by the time we split up any mis-merges again for 64K crossings, we can never have more SG segments than what the block layer originally fed to the IOMMU stuff.
Or so the IOMMU and SCSI experts here at LSF'08 have assured me, even after my own skeptical questioning.
Quoted 8 lines
I suppose so. I don't remember all of the details, but iirc, it has to do with crossing 64K boundaries. Some controllers can't handle it.
It's not only the _size_ of the segments, it's their alignment.
The iommu will not keep alignement beyond the page size (and even then... on powerpc with a 64k base page size, you may still end up with a 4k aligned result, but let's not go there now).
..
That's just not possible, unless the IOMMU *splits* segments. And the IOMMU experts here say that it never does that.
-ml
Quoted 5 lines · Mark Lord
Quoted 5 lines
I suppose so. I don't remember all of the details, but iirc, it has to do with crossing 64K boundaries. Some controllers can't handle it.
It's not only the _size_ of the segments, it's their alignment.
Quoted 7 lines
Quoted 3 lines
The iommu will not keep alignement beyond the page size (and even then... on powerpc with a 64k base page size, you may still end up with a 4k aligned result, but let's not go there now).
..
That's just not possible, unless the IOMMU *splits* segments. And the IOMMU experts here say that it never does that.
It is totally possible, and I know as wrote part of the powerpc iommu code :-)
The iommu code makes no guarantee vs. preserving the alignment of a segment, at least not below PAGE_SIZE.
Thus if you pass to dma_map_sg() a 64K aligned 64K segment, you may well get back a 4K aligned 64K segment.
Enforcing natural alignment in the iommu code only happens for dma_alloc_coherent (it uses order-N allocations anyway), it doesn't happen for map_sg. If we were to do that, we would make it very likely for iommu allocations to fail on machine with small DMA windows.
Ben.
Quoted 11 lines · Benjamin Herrenschmidt
Quoted 10 lines · Mark Lord
Quoted 8 lines
I suppose so. I don't remember all of the details, but iirc, it has to do with crossing 64K boundaries. Some controllers can't handle it.
It's not only the _size_ of the segments, it's their alignment.
The iommu will not keep alignement beyond the page size (and even then... on powerpc with a 64k base page size, you may still end up with a 4k aligned result, but let's not go there now).
..
Quoted 8 lines
Quoted 2 lines
That's just not possible, unless the IOMMU *splits* segments. And the IOMMU experts here say that it never does that.
It is totally possible, and I know as wrote part of the powerpc iommu code :-)
The iommu code makes no guarantee vs. preserving the alignment of a segment, at least not below
PAGE_SIZE.
It's supposed to, precisely to forestall this case. The alignment guarantees of the parisc iommu code are sg length aligned up to a fixed maximum (128k on 32 bit and 256k on 64 bit because of the way the
allocator works). However, tomo's code is fixing this, so it shouldn't
be a problem much longer.
James
Quoted 8 lines · James Bottomley
Quoted 2 lines
The iommu code makes no guarantee vs. preserving the alignment of a segment, at least not below
PAGE_SIZE.It's supposed to, precisely to forestall this case. The alignment guarantees of the parisc iommu code are sg length aligned up to a fixed maximum (128k on 32 bit and 256k on 64 bit because of the way the
allocator works). However, tomo's code is fixing this, so it shouldn't
be a problem much longer.
It will be. If we start enforcing that alignment, pSeries machines will continuously run out of iommu space when the BIO starts handing out large chunks.
Not acceptable for us. I suspect x86_64 will have a similar problem as, afaik, the IOMMU space is very scarce there.
Can you guys stop designing the whole IO layer around PA-RISC ?
Ben.
Quoted 23 lines · Benjamin Herrenschmidt
Quoted 5 lines · Mark Lord
Quoted 5 lines
I suppose so. I don't remember all of the details, but iirc, it has to do with crossing 64K boundaries. Some controllers can't handle it.
It's not only the _size_ of the segments, it's their alignment.
Quoted 7 lines
Quoted 3 lines
The iommu will not keep alignement beyond the page size (and even then... on powerpc with a 64k base page size, you may still end up with a 4k aligned result, but let's not go there now).
..
That's just not possible, unless the IOMMU *splits* segments. And the IOMMU experts here say that it never does that.
It is totally possible, and I know as wrote part of the powerpc iommu code :-)
The iommu code makes no guarantee vs. preserving the alignment of a segment, at least not below
PAGE_SIZE.Thus if you pass to
dma_map_sg()a 64K aligned 64K segment, you may well get back a 4K aligned 64K segment.
Quoted 6 lines
Enforcing natural alignment in the iommu code only happens for
dma_alloc_coherent(it uses order-N allocations anyway), it doesn't happen formap_sg. If we were to do that, we would make it very likely for iommu allocations to fail on machine with small DMA windows.Ben.
..
That's interesting. Can you point us to the exact file::lines where this happens? It would be good to ensure that this gets fixed.
I'm copying Fujita Tomonori on this thread now -- he's the dude who's trying to sort out the IOMMU mess.
Thanks
Quoted 29 lines · Mark Lord
Quoted 28 lines · Benjamin Herrenschmidt
Quoted 5 lines · Mark Lord
Quoted 5 lines
I suppose so. I don't remember all of the details, but iirc, it has to do with crossing 64K boundaries. Some controllers can't handle it.
It's not only the _size_ of the segments, it's their alignment.
Quoted 7 lines
Quoted 3 lines
The iommu will not keep alignement beyond the page size (and even then... on powerpc with a 64k base page size, you may still end up with a 4k aligned result, but let's not go there now).
..
That's just not possible, unless the IOMMU *splits* segments. And the IOMMU experts here say that it never does that.
It is totally possible, and I know as wrote part of the powerpc iommu code :-)
The iommu code makes no guarantee vs. preserving the alignment of a segment, at least not below
PAGE_SIZE.Thus if you pass to
dma_map_sg()a 64K aligned 64K segment, you may well get back a 4K aligned 64K segment.Enforcing natural alignment in the iommu code only happens for
dma_alloc_coherent(it uses order-N allocations anyway), it doesn't happen formap_sg. If we were to do that, we would make it very likely for iommu allocations to fail on machine with small DMA windows.
Quoted 8 lines
Quoted 1 line
Ben.
..
That's interesting. Can you point us to the exact
file::lineswhere this happens? It would be good to ensure that this gets fixed.I'm copying Fujita Tomonori on this thread now -- he's the dude who's trying to sort out the IOMMU mess.
..
Mmm.. looks like ppc is already fixed in mainline, commit 740c3ce66700640a6e6136ff679b067e92125794
Ben?
Quoted 10 lines · Mark Lord
Quoted 4 lines
That's interesting. Can you point us to the exact
file::lineswhere this happens? It would be good to ensure that this gets fixed.I'm copying Fujita Tomonori on this thread now -- he's the dude
who's
Quoted 1 line
trying to sort out the IOMMU mess.
..
Mmm.. looks like ppc is already fixed in mainline, commit
740c3ce66700640a6e6136ff679b067e92125794
No, I only fixed alignent up to PAGE_SIZE, not above.
Our iommu can use smaller than PAGE_SIZE allocations when PAGE_SIZE is 64k (it can use 4k), and that helps a lot with networking to avoid filling up pSeries small iommu's too fast. Unfortunately, that meant we used to not even get PAGE_SIZE alignment for some cases. The workaround fixes that but does not provide natural size alignment.
If we were to provide size based alignment of sg requests, fragmentation would become so bad we would basically end up failing a large amount of iommu allocations on servers.
Ben.
Quoted 20 lines · Benjamin Herrenschmidt
Quoted 10 lines · Mark Lord
Quoted 4 lines
That's interesting. Can you point us to the exact
file::lineswhere this happens? It would be good to ensure that this gets fixed.I'm copying Fujita Tomonori on this thread now -- he's the dude
who's
Quoted 1 line
trying to sort out the IOMMU mess.
..
Mmm.. looks like ppc is already fixed in mainline, commit
740c3ce66700640a6e6136ff679b067e92125794No, I only fixed alignent up to
PAGE_SIZE, not above.Our iommu can use smaller than
PAGE_SIZEallocations whenPAGE_SIZEis 64k (it can use 4k), and that helps a lot with networking to avoid filling up pSeries small iommu's too fast. Unfortunately, that meant we used to not even getPAGE_SIZEalignment for some cases. The workaround fixes that but does not provide natural size alignment.
Quoted 3 lines
If we were to provide size based alignment of sg requests, fragmentation would become so bad we would basically end up failing a large amount of iommu allocations on servers.
Ben, he just quoted the wrong patch ... that was the segment length limit patch. The boundary alignment patch is this one:
fb3475e9b6bfa666107512fbd6006c26014f04b8
And it does alter the ppc iommu to respect the dma_boundary, so all of the mechanics for removing the ppc special casing in libata/ide is upstream.
James
Quoted 10 lines · James Bottomley
Quoted 9 lines
Our iommu can use smaller than
PAGE_SIZEallocations whenPAGE_SIZEis 64k (it can use 4k), and that helps a lot with networking to avoid filling up pSeries small iommu's too fast. Unfortunately, that meant we used to not even getPAGE_SIZEalignment for some cases. The workaround fixes that but does not provide natural size alignment.If we were to provide size based alignment of sg requests, fragmentation would become so bad we would basically end up failing a large amount of iommu allocations on servers.
Quoted 8 lines
Ben, he just quoted the wrong patch ... that was the segment length limit patch. The boundary alignment patch is this one:
fb3475e9b6bfa666107512fbd6006c26014f04b8And it does alter the ppc iommu to respect the
dma_boundary, so all of the mechanics for removing the ppc special casing in libata/ide is upstream.
And the fact that this will totally blow fragmentation through the roof and cause half of the dma-map requests to fail on machines with small iommu has been totally ignored I suppose ?
Ben.
Quoted 4 lines
And the fact that this will totally blow fragmentation through the roof and cause half of the dma-map requests to fail on machines with small iommu has been totally ignored I suppose ?
Ok, so after checking those patches more closely, it appears that alignment will only be enforced for devices that impose segment boundaries, which means that normal devices can still pack. That sounds good.
Ben.
Quoted 5 lines · Mark Lord
That's interesting. Can you point us to the exact
file::lineswhere this happens? It would be good to ensure that this gets fixed.I'm copying Fujita Tomonori on this thread now -- he's the dude who's trying to sort out the IOMMU mess.
As I said before, it should not be fixed.
If it's "fixed", we'll run out of iommu space all the time.
We should stop having stupid requirements instead.
Ben.
Quoted 3 lines · Benjamin Herrenschmidt
As I said before, it should not be fixed.
If it's "fixed", we'll run out of iommu space all the time. We should stop having stupid requirements instead.
Hints would be extremely good. Teaching the I/O layers to do
if (blk_queue_align_64k(queue) == -EGOSTICKYOURHEADINABUCKET) size /= 2;
is not very hard - or even an arch helper for
n = blk_queue_worst_case(65536, 256);
Alan
Quoted 5 lines · Alan Cox
Quoted 3 lines · Benjamin Herrenschmidt
As I said before, it should not be fixed.
If it's "fixed", we'll run out of iommu space all the time. We should stop having stupid requirements instead.
Quoted 8 lines
Hints would be extremely good. Teaching the I/O layers to do
if (blk_queue_align_64k(queue) == -EGOSTICKYOURHEADINABUCKET) size /= 2;is not very hard - or even an arch helper for
n = blk_queue_worst_case(65536, 256);
Looks like the new iommu stuff will deal with it fine by passing an optional boundary not-to-cross requirement down. Normal devices don't set that and the iommu can still pack. IDE can pass 64K and the iommu will align things that may cross that boundary.
That should work out fine.
BTW. If we -still- run out of iommu space, do we have sane mechanisms nowadays to break things up and retry ?
Ben.
Quoted 14 lines · On Wed, 27 Feb 2008 10:19:36 +1100 Benjamin Herrenschmidt…
Quoted 12 lines · Alan Cox
Quoted 3 lines · Benjamin Herrenschmidt
As I said before, it should not be fixed.
If it's "fixed", we'll run out of iommu space all the time. We should stop having stupid requirements instead.Hints would be extremely good. Teaching the I/O layers to do
if (blk_queue_align_64k(queue) == -EGOSTICKYOURHEADINABUCKET) size /= 2;is not very hard - or even an arch helper for
Quoted 8 lines
Quoted 1 line
n = blk_queue_worst_case(65536, 256);Looks like the new iommu stuff will deal with it fine by passing an optional boundary not-to-cross requirement down. Normal devices don't set that and the iommu can still pack. IDE can pass 64K and the iommu will align things that may cross that boundary.
That should work out fine.
Yeah, nothing changes for drivers that don't set the boundary limit.
Even for drivers setting the boundary limit, the IOMMUs just allocate a memory area that doesn't cross the boundary. It is NOT the size-alignment allocation that POWER IOMMU's dma_alloc_coherent does (as discussed in this thread). So the IOMMU space doesn't run out quickly.
I have not fixed some IOMMUs yet (as I explained at LSF). If I finish the job, libata can remove the workaround for splitting sg lists.
Quoted 2 lines
BTW. If we -still- run out of iommu space, do we have sane mechanisms nowadays to break things up and retry ?
About SCSI, some drivers do the right thing (retrying), some assume that IOMMU mapping failure never happens, some just crash. I'll take care about it after fixing the IOMMUs.
Quoted 7 lines · FUJITA Tomonori
Quoted 1 line
BTW. If we -still- run out of iommu space, do we have sane
mechanisms
Quoted 1 line
nowadays to break things up and retry ?
About SCSI, some drivers do the right thing (retrying), some assume that IOMMU mapping failure never happens, some just crash. I'll take care about it after fixing the IOMMUs.
Thanks for doing this.
Cheers,
Ben.
Quoted 5 lines
The block layer uses
seg_boundary_maskto ensure that we never have to split them again in the LLD.A very long time ago, when I wrote the IDE DMA code, this was not the case.
Not good enough, still, because the boundaries can change due to the iommu merging, thus the split must be re-done.
Ben.
As an aside, ISTR tomo-san was working on eliminating the need for the "/2" by tackling the details on the IOMMU side...
Jeff
Quoted 1 line · Jeff Garzik
As an aside, ISTR tomo-san was working on eliminating the need for the "/2" by tackling the details on the IOMMU side...
..
Yes, tomo-san just led a nice detailed discussion of it here at LSF'08, and he agrees that even today it shouldn't affect us that way.
Cheers