Page MenuHomeFreeBSD

LinuxKPI: skbuff: adjust to updated malloc/contigmalloc and free(9).
Needs ReviewPublic

Authored by bz on Jun 30 2024, 7:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Sep 20, 6:03 PM
Unknown Object (File)
Fri, Sep 20, 6:02 PM
Unknown Object (File)
Fri, Sep 20, 5:52 PM
Unknown Object (File)
Wed, Sep 18, 2:18 PM
Unknown Object (File)
Tue, Sep 17, 11:34 PM
Unknown Object (File)
Tue, Sep 17, 6:28 PM
Unknown Object (File)
Tue, Sep 17, 12:30 PM
Unknown Object (File)
Sun, Sep 15, 4:46 AM

Details

Reviewers
jhb
Summary

Simplify the code using the extended contigmalloc(9) which means we
(a) no longer have to remember our allocation size, and (b) we can
call free(9) independent of the allocator (malloc or contigmalloc).

This will likely also allow us to make the TUNABLE a SYSCTL in the
future, allow drivers to change it on the fly (at least lowering the
limit) and with that removing the need for user adjustments.

Sponsored by: The FreeBSD Foundation
MFC after: 3 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 58430
Build 55318: arc lint + arc unit

Event Timeline

bz requested review of this revision.Jun 30 2024, 7:52 PM

s/siplify/simplify/ in the commit message

sys/compat/linuxkpi/common/src/linux_skbuff.c
100

@jhb This should likely be len > PAGE_SIZE only and not >= ?

sys/compat/linuxkpi/common/src/linux_skbuff.c
100

I don't understand why we will need this tunable at all once kmalloc() and friends are physically contiguous? I guess what you really need is a way for a driver to actually use bus_dmamem_alloc under the hood instead of kmalloc() for things like descriptor rings?

sys/compat/linuxkpi/common/src/linux_skbuff.c
100

Because ...^%$%^&*?

From all I could say DMA only worked with the lower 32bit on realtek rtw88 and they did not even set that "hint" on Linux in the rtw88 driver when I first ported it over.
For rtw89 I think they had it then... and it's 32bit (unless I added that too).

QCA/Atheros is very picky on coming out of reset and who knows what, ...
sys/contrib/dev/athk/ath10k/pci.c- /* Target expects 32 bit DMA. Enforce it. */
sys/contrib/dev/athk/ath10k/pci.c: ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

ath11k is 36 bits

ath12k is sys/contrib/dev/athk/ath12k/pci.c:#define ATH12K_PCI_DMA_MASK 32

Mediatek is all 32bit but the mt7996 chipset which is 36bit.

For Intel I found one place so far where they used a KPI which didn't "inherit and adhere to" the limit they give for the chipset; they have different limits depending on chipset generation (later ones are fine, older ones are 36 or 32 bit DMA too);

if (trans->trans_cfg->gen2) {
        trans->txqs.tfd.addr_size = 64;
... } else { ...
        trans->txqs.tfd.addr_size = 36;
}

addr_size = trans->txqs.tfd.addr_size;
ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(addr_size));
if (ret) {
        ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

The problem remains that even if physically contig you then still need to bounce because of that limit and most of them not always using the proper KPIs on Linux (I assume); and you still do not get large enough contig segments at least on arm64 for that.

Completely rewriting the drivers to busdma means we'd half port them over natively and that's a pain to maintain too -- at least for now; I added a hack back then to LinuxKPI to be able to use bus_read/write as a start by adding a 1-liner to a driver and that was an okay-ish tradeoff...

As seen by Intel, it's not that the drivers couldn't do the right thing ... I am just in no place to *NOW* deal with all their internals while too many other things are pressing. In the future having a proper abstraction for these things like we do for Ethernet and have a foo_linux.c and a foo_freebsd.c would be awesome; we are just in no place yet to get there.

But I'd get rid of the TUNABLE/SYSCTL in the future and at least let the driver express (set -- as in lower if needed) that limit (or rather let LinuxKPI do that internally given they do call dma_set_mask() or dma_set_mask_and_coherent().

100

The real question was - I should adjust the boundary check, right?