LinuxKPI: add simplified vesion of page_frag_cache
ClosedPublic
Actions

Authored by bz on Dec 3 2022, 12:42 AM.

Details

Reviewers

markj

Group Reviewers

linuxkpi

Commits

rG9bf3a5616c51: LinuxKPI: add simplified version of page_frag_cache
rG55038a6306a5: LinuxKPI: add simplified vesion of page_frag_cache

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 created this revision.Dec 3 2022, 12:42 AM

Herald added subscribers: emaste, imp. · View Herald TranscriptDec 3 2022, 12:42 AM

bz requested review of this revision.Dec 3 2022, 12:42 AM

Harbormaster completed remote builds in B48603: Diff 113786.Dec 3 2022, 12:42 AM

• hselasky added a subscriber: • hselasky.Dec 3 2022, 3:20 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.

• hselasky added inline comments.Dec 4 2022, 3:04 PM

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 accepted this revision.Dec 4 2022, 3:13 PM

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?

• hselasky added inline comments.Dec 4 2022, 8:34 PM

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