lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 8 Dec 2022 13:58:20 -0800 From: Sidhartha Kumar <sidhartha.kumar@...cle.com> To: Mike Kravetz <mike.kravetz@...cle.com>, Matthew Wilcox <willy@...radead.org> Cc: John Hubbard <jhubbard@...dia.com>, linux-kernel@...r.kernel.org, linux-mm@...ck.org, akpm@...ux-foundation.org, songmuchun@...edance.com, tsahu@...ux.ibm.com, david@...hat.com Subject: Re: [PATCH mm-unstable] mm: clarify folio_set_compound_order() zero support On 12/8/22 12:01 PM, Mike Kravetz wrote: > On 12/08/22 19:33, Matthew Wilcox wrote: >> On Thu, Dec 08, 2022 at 10:06:07AM -0800, Sidhartha Kumar wrote: >>> On 12/7/22 6:27 PM, John Hubbard wrote: >>>> On 12/7/22 17:42, Sidhartha Kumar wrote: >>> This works for me, I will take this approach along with Muchun's feedback >>> about a wrapper function so as not to touch _folio_order directly and send >>> out a new version. >>> >>> One question I have is if I should then get rid of >>> folio_set_compound_order() as hugetlb is the only compound page user I've >>> converted to folios so far and its use can be replaced by the suggested >>> folio_set_nr_pages() and folio_set_order(). >>> >>> Hugetlb also has one has one call to folio_set_compound_order() with a >>> non-zero order, should I replace this with a call to folio_set_order() and >>> folio_set_nr_pages() as well, or keep folio_set_compound_order() and remove >>> zero order support and the comment. Please let me know which approach you >>> would prefer. >> >> None of the above! >> >> Whatever we're calling this function *it does not belong* in mm.h. >> Anything outside the MM calling it is going to be a disaster -- can you >> imagine what will happen if a filesystem or device driver is handed a >> folio and decides "Oh, I'll just change the size of this folio"? It is >> an attractive nuisance and should be confined to mm/internal.h *at best*. > > I suspect it was placed in mm.h as it is the 'folio version' of > set_compound_order which resides in mm.h. But, no need to repeat that > unfortunate placement. > >> >> Equally, we *must not have* separate folio_set_order() and >> folio_set_nr_pages(). These are the same thing! They must be kept >> in sync! If we are to have a folio_set_order() instead of open-coding >> it, then it should also update nr_pages. > > Ok. Agree. > >> So, given that this is now an internal-to-mm, if not internal-to-hugetlb >> function, I see no reason that it should not handle the case of 0. >> I haven't studied what hugetlb_dissolve does, or why it can't use the >> standard split_folio(), but I'm sure there's a good reason. > > The hugetlb code is changing the compound page/folio it created from a set of > individual pages back to individual pages so they can be returned to the > low level allocator. Somewhat like what page_alloc/page_free do. split_folio > is overkill. split_page would be a closer match. > > It makes perfect sense to put the function in mm internal.h. > Thanks John, Mike, Matthew, and Muchun for the feedback. To summarize this discussion and outline the next version of this patch, the changes I'll make include: 1) change the name of folio_set_compound_order() to folio_set_order() 2) change the placement of this function from mm.h to mm/internal.h 3) folio_set_order() will set both _folio_order and _folio_nr_pages and handle the zero order case correctly. 4) remove the comment about hugetlb's specific use for zero orders 5) improve the style of folio_set_order() by removing ifdefs from inside the function to doing #ifdef CONFIG_64BIT static inline void folio_set_order(struct folio *folio, unsigned int order) { VM_BUG_ON_FOLIO(!folio_test_large(folio), folio); folio->_folio_order = order; folio->_folio_nr_pages = order ? 1U << order : 0; } #else static inline void folio_set_order(struct folio *folio, unsigned int order) { VM_BUG_ON_FOLIO(!folio_test_large(folio), folio); folio->_folio_order = order; } #endif Please let me know if I missing something. Thanks, Sidhartha Kumar > Thanks,
Powered by blists - more mailing lists