Avoid use of contiguous memory allocations in busdma

[• hselasky]

Summary

When the BUSDMA boundary is non-zero, as for AMD64 due to PCI_DMA_BOUNDARY and the BUSDMA lowaddr is equal to BUS_SPACE_MAXADDR, the bus_dmamem_alloc() function always fall back into using a contiguous allocation even when this is not needed.

This patch widens the conditions for using kmem_alloc_attr() to also cover more cases where the boundary parameter is non-zero and less than lowaddr.

This patch also fixes the nsegments check for using kmem_alloc_attr() when the maximum segment size is less than PAGE_SIZE .

Sponsored by: Mellanox Technologies
MFC after: 1 week

[kib]

I only looked at x86, but it seems to be reasonable.

That said, may be we should remove the magic 4G boundary for PCI, finally ?

[gallatin]

I added Scott, as he's been quite involved in the busdma code. I want to make sure this looks OK to him too.

[imp]

Comments need to be updated to reflect the change in the last bit of the if.

There's a lot of cut-n-paste in this code, so that needs to be done in many places.

sys/arm/arm/busdma_machdep-v4.c

735 ↗

(On Diff #28146)

This commit is missing the mandatory update to the comments here :)

sys/arm/arm/busdma_machdep-v6.c

787 ↗

(On Diff #28146)

And here

sys/arm64/arm64/busdma_bounce.c

470 ↗

(On Diff #28146)

And here.

sys/mips/mips/busdma_machdep.c

698 ↗

(On Diff #28146)

and here.

sys/x86/x86/busdma_bounce.c

406 ↗

(On Diff #28146)

and here.

[jhb]

The goal is to see if kmem_alloc_attr() will give us usable memory, and some things like maxsegsz are not directly applicable. I think in place of 'maxsegsz' you probably want 'min(maxsize, PAGE_SIZE)' as kmem_alloc_attr() just returns arbitrary pages. However, the 'alignment % maxsegsz' (or 'alignment % PAGE_SIZE') doesn't seem right. If you use an alignment of '1' to mean 1-byte alignment (and thus no real constraint), then '1 % <anything>' will be non-zero. But also requests that want, say, an alignment of 4 or 16 bytes but ask to allocate a page will always fail this check and fall through to kmem_alloc_contig(). It seems that this patch can only work for tags with an alignment of 0. I wonder if instead this should just be using '(boundary % PAGE_SIZE) == 0' as the new condition to add (and you can probably drop the 'boundary >= lowaddr' check from x86 if so):

if (nsegments >= btoc(maxsize) &&
    alignment <= PAGE_SIZE &&
    (boundary % PAGE_SIZE) == 0)
        kmem_alloc_attr()

(Note that this also handles the boundary == 0 case since 0 % PAGE_SIZE == 0.) Drivers that want a large, contiguous range larger than a page that span the 4G boundary (like twe/twa) would fail the 'nsegments >= btoc' test since they would have nsegments == 1. (Which is the original source of the PCI boundary as there are known-broken firmwares.)

[kib]

In D10645#221477, @jhb wrote:

Drivers that want a large, contiguous range larger than a page that span the 4G boundary (like twe/twa) would fail the 'nsegments >= btoc' test since they would have nsegments == 1. (Which is the original source of the PCI boundary as there are known-broken firmwares.)

Still I do not understand why a workaround for some broken HBAs is imposed on all PCI devices.

[• hselasky]

The goal is to see if kmem_alloc_attr() will give us usable memory, and some things like maxsegsz are not directly applicable. I think in place of 'maxsegsz' you probably want 'min(maxsize, PAGE_SIZE)' as kmem_alloc_attr() just returns arbitrary pages. However, the 'alignment % maxsegsz' (or 'alignment % PAGE_SIZE') doesn't seem right. If you use an alignment of '1' to mean 1-byte alignment (and thus no real constraint), then '1 % <anything>' will be non-zero. But also requests that want, say, an alignment of 4 or 16 bytes but ask to allocate a page will always fail this check and fall through to kmem_alloc_contig(). It seems that this patch can only work for tags with an alignment of 0. I wonder if instead this should just be using '(boundary % PAGE_SIZE) == 0' as the new condition to add (and you can probably drop the 'boundary >= lowaddr' check from x86 if so):

if (nsegments >= btoc(maxsize) &&

alignment <= PAGE_SIZE &&
(boundary % PAGE_SIZE) == 0)
    kmem_alloc_attr()

It is not enough to check (boundary % PAGE_SIZE) == 0, because the alignment might be incorrectly specified as non-power of two.

How about this:

/* check if effective boundary is zero */
if (boundary >= lowaddr)
    boundary = 0;

/* check if constraints are OK for kmem_alloc_attr() */
if (maxsegsz != 0 && /* guard against division by zero */
    nsegments >= howmany(maxsize, maxsegsz) && /* maxsegsz may be smaller than PAGE_SIZE, though not likely */
    alignment != 0 && alignment <= PAGE_SIZE && /* kmem allocates PAGE_SIZE aligned memory - OK */
    (boundary % alignment) == 0 && /* this case also verifies non-power of two alignments */
    (PAGE_SIZE % maxsegsz) == 0) /* this case verifies that a segment cannot cross two pages */
         kmem_alloc_attr()

(Note that this also handles the boundary == 0 case since 0 % PAGE_SIZE == 0.) Drivers that want a large, contiguous range larger than a page that span the 4G boundary (like twe/twa) would fail the 'nsegments >= btoc' test since they would have nsegments == 1. (Which is the original source of the PCI boundary as there are known-broken firmwares.)

[• hselasky]

@imp : I will fix your comments once @jhb agrees on an algorithm.

@jhb: Thinking a bit more about this, we need a min() for maxsegsz:

/* check if effective boundary is zero */
if (boundary >= lowaddr)
    boundary = 0;

/* check if the effective maxsegsz is PAGE_SIZE */
if (maxsegsz > PAGE_SIZE)
    maxsegsz = PAGE_SIZE;

/* check if constraints are OK for kmem_alloc_attr() */
if (maxsegsz != 0 && /* guard against division by zero */
    nsegments >= howmany(maxsize, maxsegsz) && /* maxsegsz may be smaller than PAGE_SIZE, though not likely */
    alignment != 0 && alignment <= PAGE_SIZE && /* kmem allocates PAGE_SIZE aligned memory - OK */
    (boundary % alignment) == 0 && /* this case also verifies non-power of two alignments */
    (PAGE_SIZE % maxsegsz) == 0) /* this case verifies that a segment cannot cross two pages */
         kmem_alloc_attr()

Assuming that maxsegsz is power of two and that maxsegsz is not zero, that alignment is power of two and not zero and that a sane boundary is power of two and greater than PAGE_SIZE, some checks can be optimised away, leaving the code like this:

/* check if constraints are OK for kmem_alloc_attr() */
if (nsegments >= howmany(maxsize, MIN(maxsegsz, PAGE_SIZE)) &&
    alignment <= PAGE_SIZE &&
    (boundary % PAGE_SIZE) == 0)
         kmem_alloc_attr()

@jhb
The only difference between my and your proposal is the MIN() for maxsegsz in the nsegments check.

[jhb]

Per bus_dma(9), alignment is required to be a power of 2. It may be that bus_dma_tag_create doesn't currently EINVAL if that isn't true, but that check can be added. boundary should also be a power of 2 and I have a very low priority item on my todo list to add an EINVAL check for that as well. I guess maxsegsz is needed for the segment check to support maxsegsz < PAGE_SIZE.

[• hselasky]

Implement suggestions from @jhb and @imp .

[• hselasky]

Fix for C&P issues. Not all platforms have the "common" field in the dma tag.