On Fri, Jan 24, 2025 at 8:28 AM Vlastimil Babka <vbabka@xxxxxxx> wrote: > > 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. Yes, as a way to simplify free_pages_nolock(). Hence I looked into it and added the above comment to the cover letter. Now I see that there are more and stronger reasons to use it. > > 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. Understood. I also agree with Matthew comment about page corruption. Whether compound_order(page) or order is correct is indeed a question. I'll wait for review on patch 3 before resubmitting with GFP_COMP included.
