On 1/24/25 17:19, Alexei Starovoitov wrote: > On Fri, Jan 24, 2025 at 6:19 AM Vlastimil Babka <vbabka@xxxxxxx> wrote: >> >> On 1/24/25 15:16, Matthew Wilcox wrote: >> > On Thu, Jan 23, 2025 at 07:56:49PM -0800, Alexei Starovoitov wrote: >> >> - Considered using __GFP_COMP in try_alloc_pages to simplify >> >> free_pages_nolock a bit, but then decided to make it work >> >> for all types of pages, since free_pages_nolock() is used by >> >> stackdepot and currently it's using non-compound order 2. >> >> I felt it's best to leave it as-is and make free_pages_nolock() >> >> support all pages. >> > >> > We're trying to eliminate non-use of __GFP_COMP. Because people don't >> > use __GFP_COMP, there's a security check that we can't turn on. Would >> > you reconsider this change you made? >> >> This means changing stackdepot to use __GFP_COMP. Which would be a good >> thing on its own. But if you consider if off-topic to your series, I can >> look at it. > > Ohh. I wasn't aware of that. > I can certainly add __GFP_COMP to try_alloc_pages() and Yeah IIRC I suggested that previously. > will check stackdepot too. Great, thanks. > I spotted this line: > VM_BUG_ON_PAGE(compound && compound_order(page) != order, page); > that line alone was a good enough reason to use __GFP_COMP, > but since it's debug only I could only guess where the future lies. > > Should it be something like: > > if (WARN_ON(compound && compound_order(page) != order)) > order = compound_order(page); > > since proceeding with the wrong order is certain to crash. > ? That's the common question, should we be paranoid and add overhead to fast paths in production. Here we do only for DEBUG_VM, which is meant for easier debugging during development of new code. I think it's not worth adding this overhead in normal configs, as the (increasing) majority of order > 0 parameters should come here from compound_order() anyway (i.e. put_folio()) As said we're trying to eliminate the other cases so we don't need to cater for them more.
