Nikita Kalyazin <[email protected]> writes: > On 15/01/2026 21:40, Ackerley Tng wrote: >> "Kalyazin, Nikita" <[email protected]> writes: >> >>> From: Patrick Roy <[email protected]> >>> >>> This drops an optimization in gup_fast_folio_allowed() where >>> secretmem_mapping() was only called if CONFIG_SECRETMEM=y. secretmem is >>> enabled by default since commit b758fe6df50d ("mm/secretmem: make it on >>> by default"), so the secretmem check did not actually end up elided in >>> most cases anymore anyway. >>> >>> This is in preparation of the generalization of handling mappings where >>> direct map entries of folios are set to not present. Currently, >>> mappings that match this description are secretmem mappings >>> (memfd_secret()). Later, some guest_memfd configurations will also fall >>> into this category. >>> >>> Signed-off-by: Patrick Roy <[email protected]> >>> Acked-by: Vlastimil Babka <[email protected]> >>> Signed-off-by: Nikita Kalyazin <[email protected]> >>> --- >>> mm/gup.c | 11 +---------- >>> 1 file changed, 1 insertion(+), 10 deletions(-) >>> >>> diff --git a/mm/gup.c b/mm/gup.c >>> index 95d948c8e86c..9cad53acbc99 100644 >>> --- a/mm/gup.c >>> +++ b/mm/gup.c >>> @@ -2739,7 +2739,6 @@ static bool gup_fast_folio_allowed(struct folio >>> *folio, unsigned int flags) >>> { >>> bool reject_file_backed = false; >>> struct address_space *mapping; >>> - bool check_secretmem = false; >>> unsigned long mapping_flags; >>> >>> /* >>> @@ -2751,14 +2750,6 @@ static bool gup_fast_folio_allowed(struct folio >>> *folio, unsigned int flags) >> >> Copying some lines the diff didn't contain: >> >> /* >> * If we aren't pinning then no problematic write can occur. A long >> term >> * pin is the most egregious case so this is the one we disallow. >> */ >> if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) == >> (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) >> >> If we're pinning, can we already return true here? IIUC this function >> is passed a folio that is file-backed, and the check if (!mapping) is >> just there to catch the case where the mapping got truncated. > > I have to admit that I am not comfortable with removing this check, > unless someone says it's certainly alright. >
Perhaps David can help here, David last changed this in f002882ca369aba3eece5006f3346ccf75ede7c5 (mm: merge folio_is_secretmem() and folio_fast_pin_allowed() into gup_fast_folio_allowed()) from return true to check_secretmem = true :) >> >> Or should we wait for the check where the mapping got truncated? If so, >> then maybe we can move this "are we pinning" check to after this check >> and remove the reject_file_backed variable? > > I can indeed move the pinning check to the end to remove the variable. > I'd do it in a separate patch. > >> >> /* >> * The mapping may have been truncated, in any case we cannot >> determine >> * if this mapping is safe - fall back to slow path to determine >> how to >> * proceed. >> */ >> if (!mapping) >> return false; >> >> >>> reject_file_backed = true; >>> >>> /* We hold a folio reference, so we can safely access folio fields. >>> */ >>> - >>> - /* secretmem folios are always order-0 folios. */ >>> - if (IS_ENABLED(CONFIG_SECRETMEM) && !folio_test_large(folio)) >>> - check_secretmem = true; >>> - >>> - if (!reject_file_backed && !check_secretmem) >>> - return true; >>> - >>> if (WARN_ON_ONCE(folio_test_slab(folio))) >>> return false; >>> >>> @@ -2800,7 +2791,7 @@ static bool gup_fast_folio_allowed(struct folio >>> *folio, unsigned int flags) >>> * At this point, we know the mapping is non-null and points to an >>> * address_space object. >>> */ >>> - if (check_secretmem && secretmem_mapping(mapping)) >>> + if (secretmem_mapping(mapping)) >>> return false; >>> /* The only remaining allowed file system is shmem. */ >>> return !reject_file_backed || shmem_mapping(mapping); >>> -- >>> 2.50.1
