Page MenuHomeFreeBSD

LinuxKPI: add simplified vesion of page_frag_cache
ClosedPublic

Authored by bz on Dec 3 2022, 12:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 1, 3:03 AM
Unknown Object (File)
Sep 29 2024, 7:13 PM
Unknown Object (File)
Sep 23 2024, 4:05 AM
Unknown Object (File)
Sep 12 2024, 4:07 AM
Unknown Object (File)
Sep 12 2024, 4:07 AM
Unknown Object (File)
Sep 9 2024, 3:47 PM
Unknown Object (File)
Sep 4 2024, 9:54 PM
Unknown Object (File)
Sep 4 2024, 7:39 PM

Details

Summary

For the moment and the currently only consumer (mt76) add a simplified
version of the page_frag_cache. We will only accept fragement sizes up
to 1 PAGE_SIZE (KASSERT) and we will always return a full page.
Should we add more consumers or small (or large) objects would become a
problem we can always add a more elaborate version.

Discussed with: markj
MFC after: 3 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bz requested review of this revision.Dec 3 2022, 12:42 AM
hselasky added inline comments.
sys/compat/linuxkpi/common/src/linux_page.c
453

Doesn't fls() take an integer. While fragsz is size_t ? Not that it matters, but maybe check the valid range?

bz marked an inline comment as done.Dec 3 2022, 11:13 PM
bz added inline comments.
sys/compat/linuxkpi/common/src/linux_page.c
453

Well with the KASSERT above and the 0 check I think the input is pretty limited into fls.

sys/compat/linuxkpi/common/src/linux_page.c
453

Can you change that KASSERT() into if (fragsz == 0 || fragsz > PAGE_SIZE) return (NULL); ?

I mean try to avoid panicing?

markj added inline comments.
sys/compat/linuxkpi/common/src/linux_page.c
453

I'd still argue for using flsl() here. Or just don't bother with it at all, i.e., just write pages = alloc_pages(gfp, 1);, since it will take a lot more work to support fragsz > PAGE_SIZE.

This revision is now accepted and ready to land.Dec 4 2022, 3:13 PM
bz marked 2 inline comments as done.Dec 4 2022, 4:33 PM
bz added inline comments.
sys/compat/linuxkpi/common/src/linux_page.c
453

The panic is on purpose as if we "hide" the implementation limitation we might have spurious errors one day which may take hours to debug only to find this.

453

It's alloc_pages(gfp, 0); I now understand what @hselasky meant above. Would changing it to flsl() work for both of you?

sys/compat/linuxkpi/common/src/linux_page.c
453

Well, only panic if gfp & M_WAITOK && fragsz > PAGE_SIZE ? Else clients will handle and print failures on NULL allocations?

I'm worried that user-space input may be passed directly to fragsz, which is not good.

--HPS

sys/compat/linuxkpi/common/src/linux_page.c
453

That's fine with me.

bz marked an inline comment as done.

Update to flsl() as suggested during review.
Keep the panic in the cases KASSERTs is not a NOP.

This revision now requires review to proceed.Dec 18 2022, 10:07 PM
This revision is now accepted and ready to land.Dec 19 2022, 3:13 PM