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

Authored by bz on Jul 24 2024, 3:14 PM.

Details

Reviewers

markj
kib
jhb
andrew
manu

Group Reviewers

bhyve

Commits

rGd1bdc2821fcd: Deprecate contigfree(9) in favour of free(9)

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 created this revision.Jul 24 2024, 3:14 PM

Herald added a reviewer: andrew. · View Herald TranscriptJul 24 2024, 3:14 PM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a reviewer: manu. · View Herald Transcript

Herald added a reviewer: bhyve. · View Herald Transcript

Herald added subscribers: riscv, olce, ehem_freebsd_m5p.com and 6 others. · View Herald Transcript

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

Harbormaster completed remote builds in B58798: Diff 141309.Jul 24 2024, 3:16 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.

kib added inline comments.Jul 24 2024, 3:44 PM

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.

kib added a subscriber: pho.Jul 24 2024, 3:44 PM

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

Harbormaster completed remote builds in B58803: Diff 141315.Jul 24 2024, 4:15 PM

bz added inline comments.Jul 24 2024, 4:15 PM

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

ehem_freebsd_m5p.com removed a subscriber: ehem_freebsd_m5p.com.Jul 24 2024, 7:22 PM

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

Address comments from @kib and @imp

Harbormaster completed remote builds in B58807: Diff 141332.Jul 24 2024, 9:05 PM

bz marked an inline comment as done.Jul 24 2024, 9:06 PM

kib accepted this revision.Jul 25 2024, 10:02 AM

This revision is now accepted and ready to land.Jul 25 2024, 10:02 AM

markj accepted this revision as: markj.Jul 25 2024, 12:58 PM

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