Page MenuHomeFreeBSD

Deprecate contigfree(9) in favour of free(9)
ClosedPublic

Authored by bz on Jul 24 2024, 3:14 PM.
Tags
None
Referenced Files
F97839153: D46099.id141332.diff
Tue, Oct 1, 11:40 AM
Unknown Object (File)
Mon, Sep 30, 4:49 PM
Unknown Object (File)
Mon, Sep 30, 2:10 PM
Unknown Object (File)
Mon, Sep 30, 4:22 AM
Unknown Object (File)
Sat, Sep 28, 2:31 PM
Unknown Object (File)
Sat, Sep 28, 12:19 AM
Unknown Object (File)
Fri, Sep 27, 3:31 PM
Unknown Object (File)
Fri, Sep 27, 12:02 PM

Details

Summary

As of 9e6544dd6e02c46b805d11ab925c4f3b18ad7a4b contigfree(9) is no longer
needed and should not be used anymore. We leave a wrapper under
BURN_BRIDGES for 3rd party code in 15.x but remove (almost) all other
cases from the tree.

This leaves one use of contigfree(9) untouched; that was the original
trigger for 9e6544dd6e02 and is handled in D45813 (to be committed
seperately later).

X-MFC: never
Sponsored by: The FreeBSD Foundation

Diff Detail

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

Event Timeline

bz requested review of this revision.Jul 24 2024, 3:14 PM

I've run a make tinderbox and I tried to remove any variables only used to hold the length for contigfree but nothing else; I did punt at one point (vmci code) and did not track all callers of vmci_free_kernel_mem().

Given there is a lot of vendor code please add others to the review as you know and would be needed.

share/man/man9/contigmalloc.9
107

s/See/Use ?

sys/i386/acpica/acpi_wakeup.c
351–352

Remove the NULL check

sys/kern/kern_malloc.c
515

I think we are stuck with the symbol for very long time, and I do not like these 'bridges'.

sys/powerpc/powernv/opal_nvram.c
169–170

NULL check is not needed

sys/x86/isa/isa_dma.c
100

I suspect the malloc() attempt is long time useless. But this is for different change.

bz marked 5 inline comments as done.

Address comments from @kib (I left the bridges alone but I agree a better name would be appreciated)

sys/x86/isa/isa_dma.c
100

Indeed.

In D46099#1050861, @bz wrote:

Address comments from @kib (I left the bridges alone but I agree a better name would be appreciated)

I mean, I prefer to not add any #ifdef's around definition and declaration.

I tried to flag more NULL-check-no-longer-needed places, but definitely did not listed them all.

sys/amd64/acpica/acpi_wakeup.c
363–364

There too (NULL check)

sys/dev/bwn/if_bwn.c
2990

NULL check

3009

NULL check

tools/test/stress2/misc/contigmalloc.sh
199–200

NULL check

tools/test/stress2/misc/contigmalloc2.sh
166–167

NULL check

tools/test/stress2/misc/contigmalloc3.sh
168–169

NULL check

Apart from the one comment I left, I like it. Here's more context on some of these drivers than maybe you wanted...

There's a number of drivers that shouldn't be changed, but instead we should likely remove them (go ahead and make the change, I'm mostly socializing this idea):
agp_i820
bwn
hpt27xx
hptmv
hptnr
hptrr

isci and pms are likely also on that list, but I'm less sure of these. isci is a 10-year old pain in the #%!#$^!$^ that intel foisted onto the world that wasn't ahci, but embedded on motherboards. Netflix retired all its isci systems a few years ago, having deployed them originally in 2013 I think, they are long since out of our fleet.... pms was dumped into the tree, and was abandon-ware before the commit message reached most people's inbox...

qat and vmpi are, I think, legit vendor code, as is the acpi code, but maybe it's purely FreeBSD specific by now the bits that you changed.

Some and/or all of these likely should have been imported as sys/contrib, but we live in an imperfect world. None of them have seen an update recently enough to try to move some of them there (though the Qat naming suggests that those files might be updated in the future)

sys/kern/kern_malloc.c
515

I think we shouldn't have this ifdef here. I agree with kib.
We've generally moved away from this construct.
Also, all the other BURN_BRIDGES have been extremely resistant to removal, while lots of other things that aren't tagged have been removed.
Last time I tried, 5 major releases weren't enough to ditch things, so I stopped trying to clean these things out.
So much so I had planned on seeing if we can just remove them all from the tree, but not the code they bracket.

@kib @imp I removed the BURN_BRIDGES (will come with the other changes in a few minutes); almost want an attribute __deprecated. I wonder how (other) big projects annotate deprecated APIs to yield warnings on use...

bz marked 6 inline comments as done.Jul 24 2024, 9:04 PM
bz marked an inline comment as done.Jul 24 2024, 9:06 PM
This revision is now accepted and ready to land.Jul 25 2024, 10:02 AM

I ran stress tests for 10 hours with D46099.141332.patch. I did not observe any issues.

In D46099#1051506, @pho wrote:

I ran stress tests for 10 hours with D46099.141332.patch. I did not observe any issues.

Awesome. Thank you!

I kinda like moving to __deprecated... but we can do that separately after the final form is agreed to.