mm/huge_memory.c | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-)
Smatch/coverity checkers report NULL mapping referencing issues[1][2][3]
every time the code is modified, because they do not understand that
mapping cannot be NULL when a folio is in page cache in the code.
Refactor the code to make it explicit.
No functional change is intended.
[1]https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/
[2]https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/
[3]https://lore.kernel.org/linux-mm/20250716145804.4836-1-antonio@mandelbit.com/
Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Zi Yan <ziy@nvidia.com>
---
mm/huge_memory.c | 43 ++++++++++++++++++++++++++++---------------
1 file changed, 28 insertions(+), 15 deletions(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 31b5c4e61a57..fe17b0a157cd 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3804,6 +3804,8 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
*/
for (new_folio = folio_next(folio); new_folio != next_folio;
new_folio = next) {
+ unsigned long nr_pages = folio_nr_pages(new_folio);
+
next = folio_next(new_folio);
expected_refs = folio_expected_ref_count(new_folio) + 1;
@@ -3811,25 +3813,36 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
lru_add_split_folio(folio, new_folio, lruvec, list);
- /* Some pages can be beyond EOF: drop them from cache */
- if (new_folio->index >= end) {
- if (shmem_mapping(mapping))
- nr_shmem_dropped += folio_nr_pages(new_folio);
- else if (folio_test_clear_dirty(new_folio))
- folio_account_cleaned(
- new_folio,
- inode_to_wb(mapping->host));
- __filemap_remove_folio(new_folio, NULL);
- folio_put_refs(new_folio,
- folio_nr_pages(new_folio));
- } else if (mapping) {
- __xa_store(&mapping->i_pages, new_folio->index,
- new_folio, 0);
- } else if (swap_cache) {
+ /*
+ * Anonymous folio with swap cache.
+ * NOTE: shmem in swap cache is not supported yet.
+ */
+ if (swap_cache) {
__xa_store(&swap_cache->i_pages,
swap_cache_index(new_folio->swap),
new_folio, 0);
+ continue;
+ }
+
+ /* Anonymouse folio without swap cache */
+ if (!mapping)
+ continue;
+
+ /* Add the new folio to the page cache. */
+ if (new_folio->index < end) {
+ __xa_store(&mapping->i_pages, new_folio->index,
+ new_folio, 0);
+ continue;
}
+
+ /* Drop folio beyond EOF: ->index >= end */
+ if (shmem_mapping(mapping))
+ nr_shmem_dropped += nr_pages;
+ else if (folio_test_clear_dirty(new_folio))
+ folio_account_cleaned(
+ new_folio, inode_to_wb(mapping->host));
+ __filemap_remove_folio(new_folio, NULL);
+ folio_put_refs(new_folio, nr_pages);
}
/*
* Unfreeze @folio only after all page cache entries, which
--
2.47.2On Wed, Jul 16, 2025 at 01:11:12PM -0400, Zi Yan wrote:
> Smatch/coverity checkers report NULL mapping referencing issues[1][2][3]
> every time the code is modified, because they do not understand that
> mapping cannot be NULL when a folio is in page cache in the code.
> Refactor the code to make it explicit.
>
> No functional change is intended.
>
> [1]https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/
> [2]https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/
> [3]https://lore.kernel.org/linux-mm/20250716145804.4836-1-antonio@mandelbit.com/
>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Zi Yan <ziy@nvidia.com>
This is fantastic, thanks Zi! There's a nit below but I actually almost
_don't_ want you to address it :P
Therefore:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> mm/huge_memory.c | 43 ++++++++++++++++++++++++++++---------------
> 1 file changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 31b5c4e61a57..fe17b0a157cd 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3804,6 +3804,8 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> */
> for (new_folio = folio_next(folio); new_folio != next_folio;
> new_folio = next) {
> + unsigned long nr_pages = folio_nr_pages(new_folio);
> +
> next = folio_next(new_folio);
>
> expected_refs = folio_expected_ref_count(new_folio) + 1;
> @@ -3811,25 +3813,36 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>
> lru_add_split_folio(folio, new_folio, lruvec, list);
>
> - /* Some pages can be beyond EOF: drop them from cache */
> - if (new_folio->index >= end) {
> - if (shmem_mapping(mapping))
> - nr_shmem_dropped += folio_nr_pages(new_folio);
> - else if (folio_test_clear_dirty(new_folio))
> - folio_account_cleaned(
> - new_folio,
> - inode_to_wb(mapping->host));
> - __filemap_remove_folio(new_folio, NULL);
> - folio_put_refs(new_folio,
> - folio_nr_pages(new_folio));
> - } else if (mapping) {
> - __xa_store(&mapping->i_pages, new_folio->index,
> - new_folio, 0);
> - } else if (swap_cache) {
> + /*
> + * Anonymous folio with swap cache.
> + * NOTE: shmem in swap cache is not supported yet.
Nice added context!
> + */
> + if (swap_cache) {
> __xa_store(&swap_cache->i_pages,
> swap_cache_index(new_folio->swap),
> new_folio, 0);
> + continue;
> + }
> +
> + /* Anonymouse folio without swap cache */
I almost don't want to comment here because 'anony-mouse' is really cute :P
but yeah nit I think you have a trailing 'e' here that my cats would be
VERY interested in... ;)
> + if (!mapping)
> + continue;
> +
> + /* Add the new folio to the page cache. */
> + if (new_folio->index < end) {
> + __xa_store(&mapping->i_pages, new_folio->index,
> + new_folio, 0);
> + continue;
> }
> +
> + /* Drop folio beyond EOF: ->index >= end */
> + if (shmem_mapping(mapping))
> + nr_shmem_dropped += nr_pages;
> + else if (folio_test_clear_dirty(new_folio))
> + folio_account_cleaned(
> + new_folio, inode_to_wb(mapping->host));
> + __filemap_remove_folio(new_folio, NULL);
> + folio_put_refs(new_folio, nr_pages);
> }
> /*
> * Unfreeze @folio only after all page cache entries, which
> --
> 2.47.2
>
Since we no longer need to make new_folio->index >= end work for anon
folios, can we drop the end = -1 in the if (is_anon) { ... } branch?
On 17 Jul 2025, at 10:46, Lorenzo Stoakes wrote:
> On Wed, Jul 16, 2025 at 01:11:12PM -0400, Zi Yan wrote:
>> Smatch/coverity checkers report NULL mapping referencing issues[1][2][3]
>> every time the code is modified, because they do not understand that
>> mapping cannot be NULL when a folio is in page cache in the code.
>> Refactor the code to make it explicit.
>>
>> No functional change is intended.
>>
>> [1]https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/
>> [2]https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/
>> [3]https://lore.kernel.org/linux-mm/20250716145804.4836-1-antonio@mandelbit.com/
>>
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>
> This is fantastic, thanks Zi! There's a nit below but I actually almost
> _don't_ want you to address it :P
>
> Therefore:
>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
>> ---
>> mm/huge_memory.c | 43 ++++++++++++++++++++++++++++---------------
>> 1 file changed, 28 insertions(+), 15 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 31b5c4e61a57..fe17b0a157cd 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -3804,6 +3804,8 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>> */
>> for (new_folio = folio_next(folio); new_folio != next_folio;
>> new_folio = next) {
>> + unsigned long nr_pages = folio_nr_pages(new_folio);
>> +
>> next = folio_next(new_folio);
>>
>> expected_refs = folio_expected_ref_count(new_folio) + 1;
>> @@ -3811,25 +3813,36 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>
>> lru_add_split_folio(folio, new_folio, lruvec, list);
>>
>> - /* Some pages can be beyond EOF: drop them from cache */
>> - if (new_folio->index >= end) {
>> - if (shmem_mapping(mapping))
>> - nr_shmem_dropped += folio_nr_pages(new_folio);
>> - else if (folio_test_clear_dirty(new_folio))
>> - folio_account_cleaned(
>> - new_folio,
>> - inode_to_wb(mapping->host));
>> - __filemap_remove_folio(new_folio, NULL);
>> - folio_put_refs(new_folio,
>> - folio_nr_pages(new_folio));
>> - } else if (mapping) {
>> - __xa_store(&mapping->i_pages, new_folio->index,
>> - new_folio, 0);
>> - } else if (swap_cache) {
>> + /*
>> + * Anonymous folio with swap cache.
>> + * NOTE: shmem in swap cache is not supported yet.
>
> Nice added context!
>
>> + */
>> + if (swap_cache) {
>> __xa_store(&swap_cache->i_pages,
>> swap_cache_index(new_folio->swap),
>> new_folio, 0);
>> + continue;
>> + }
>> +
>> + /* Anonymouse folio without swap cache */
>
> I almost don't want to comment here because 'anony-mouse' is really cute :P
> but yeah nit I think you have a trailing 'e' here that my cats would be
> VERY interested in... ;)
Will change it. :p
>
>> + if (!mapping)
>> + continue;
>> +
>> + /* Add the new folio to the page cache. */
>> + if (new_folio->index < end) {
>> + __xa_store(&mapping->i_pages, new_folio->index,
>> + new_folio, 0);
>> + continue;
>> }
>> +
>> + /* Drop folio beyond EOF: ->index >= end */
>> + if (shmem_mapping(mapping))
>> + nr_shmem_dropped += nr_pages;
>> + else if (folio_test_clear_dirty(new_folio))
>> + folio_account_cleaned(
>> + new_folio, inode_to_wb(mapping->host));
>> + __filemap_remove_folio(new_folio, NULL);
>> + folio_put_refs(new_folio, nr_pages);
>> }
>> /*
>> * Unfreeze @folio only after all page cache entries, which
>> --
>> 2.47.2
>>
>
> Since we no longer need to make new_folio->index >= end work for anon
> folios, can we drop the end = -1 in the if (is_anon) { ... } branch?
Sure.
OK, since I also need to address your comments on
“mm/huge_memory: move unrelated code out of __split_unmapped_folio()”,
I am going to send a new series with both this patch and
patches from __split_unmapped_folio().
We are not in a rush, so let’s make it as good as possible. :)
Best Regards,
Yan, Zi
>>
>> Since we no longer need to make new_folio->index >= end work for anon
>> folios, can we drop the end = -1 in the if (is_anon) { ... } branch?
>
> Sure.
A second thought on this one. If I remove end = -1, can static analysis
tools understand that end is not used when a folio is anonymous?
Probably, I can initialize end to -1 and remove end = -1 in is_anon
branch.
Best Regards,
Yan, Zi
On Thu, Jul 17, 2025 at 11:45:13AM -0400, Zi Yan wrote:
>
> >>
> >> Since we no longer need to make new_folio->index >= end work for anon
> >> folios, can we drop the end = -1 in the if (is_anon) { ... } branch?
> >
> > Sure.
>
> A second thought on this one. If I remove end = -1, can static analysis
> tools understand that end is not used when a folio is anonymous?
> Probably, I can initialize end to -1 and remove end = -1 in is_anon
> branch.
I don't think we should be concering ourselves with this generally
speaking.
But doesn't David's suggested changes preclude this anyway? As you'd only
be referencing end if mapping is set? But then that'd rely on !anon...
Perhaps you can just move figuring out what end is to the if (mapping)
block...
But as Dan alluded I think (hope :P) humans read this stuff in the end
before reporting :)
So if you add a comment very clearly stating that end won't be used if
!mapping then that alone would do I think.
But I see Dan's replying here anyway so will leave to his expertise/your
discretion.
>
> Best Regards,
> Yan, Zi
On Thu, Jul 17, 2025 at 11:45:13AM -0400, Zi Yan wrote:
>
> >>
> >> Since we no longer need to make new_folio->index >= end work for anon
> >> folios, can we drop the end = -1 in the if (is_anon) { ... } branch?
> >
> > Sure.
>
> A second thought on this one. If I remove end = -1, can static analysis
> tools understand that end is not used when a folio is anonymous?
> Probably, I can initialize end to -1 and remove end = -1 in is_anon
> branch.
Smatch says that "if "mapping" is non-NULL then "end" is initialized"
and it doesn't trigger a warning. I don't know how the other checkers
handle it.
Btw, the only thing you really have to pay attention to is Clang because
we treat build warnings as failure. You're always free to ignore other
checkers.
regards,
dan carpenter
On 17 Jul 2025, at 15:22, Dan Carpenter wrote:
> On Thu, Jul 17, 2025 at 11:45:13AM -0400, Zi Yan wrote:
>>
>>>>
>>>> Since we no longer need to make new_folio->index >= end work for anon
>>>> folios, can we drop the end = -1 in the if (is_anon) { ... } branch?
>>>
>>> Sure.
>>
>> A second thought on this one. If I remove end = -1, can static analysis
>> tools understand that end is not used when a folio is anonymous?
>> Probably, I can initialize end to -1 and remove end = -1 in is_anon
>> branch.
>
> Smatch says that "if "mapping" is non-NULL then "end" is initialized"
> and it doesn't trigger a warning. I don't know how the other checkers
> handle it.
Great. Thank you for running smatch for it.
>
> Btw, the only thing you really have to pay attention to is Clang because
> we treat build warnings as failure. You're always free to ignore other
> checkers.
Good to know. I will compile my code using clang to get a sense on how
checkers will react to my changes. Thank you for the information.
Best Regards,
Yan, Zi
On 16/07/2025 19:11, Zi Yan wrote: > Smatch/coverity checkers report NULL mapping referencing issues[1][2][3] > every time the code is modified, because they do not understand that > mapping cannot be NULL when a folio is in page cache in the code. > Refactor the code to make it explicit. > > No functional change is intended. > > [1]https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/ > [2]https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/ > [3]https://lore.kernel.org/linux-mm/20250716145804.4836-1-antonio@mandelbit.com/ > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Zi Yan <ziy@nvidia.com> Much easier to grasp - Thanks a lot! I am sure Coverity will be happy too at this point, because the ambiguity has been fully removed. In a previous email you asked me how to prevent Coverity from complaining about certain code: my thinking is fully aligned with Dan's reply. IMHO refactoring the code was the best choice - thanks again. Regards, -- Antonio Quartulli CEO and Co-Founder Mandelbit Srl https://www.mandelbit.com
On 16 Jul 2025, at 15:01, Antonio Quartulli wrote: > On 16/07/2025 19:11, Zi Yan wrote: >> Smatch/coverity checkers report NULL mapping referencing issues[1][2][3] >> every time the code is modified, because they do not understand that >> mapping cannot be NULL when a folio is in page cache in the code. >> Refactor the code to make it explicit. >> >> No functional change is intended. >> >> [1]https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/ >> [2]https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/ >> [3]https://lore.kernel.org/linux-mm/20250716145804.4836-1-antonio@mandelbit.com/ >> >> Suggested-by: David Hildenbrand <david@redhat.com> >> Signed-off-by: Zi Yan <ziy@nvidia.com> > > Much easier to grasp - Thanks a lot! > > I am sure Coverity will be happy too at this point, because the ambiguity has been fully removed. > > In a previous email you asked me how to prevent Coverity from complaining about certain code: my thinking is fully aligned with Dan's reply. IMHO refactoring the code was the best choice - thanks again. Sure. Coverity/smatch makes the code better this time. :) Best Regards, Yan, Zi
On 16.07.25 19:11, Zi Yan wrote: > Smatch/coverity checkers report NULL mapping referencing issues[1][2][3] > every time the code is modified, because they do not understand that > mapping cannot be NULL when a folio is in page cache in the code. > Refactor the code to make it explicit. > > No functional change is intended. > > [1]https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/ > [2]https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/ > [3]https://lore.kernel.org/linux-mm/20250716145804.4836-1-antonio@mandelbit.com/ > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Zi Yan <ziy@nvidia.com> > --- Thanks! Acked-by: David Hildenbrand <david@redhat.com> -- Cheers, David / dhildenb
On 16 Jul 2025, at 14:14, David Hildenbrand wrote: > On 16.07.25 19:11, Zi Yan wrote: >> Smatch/coverity checkers report NULL mapping referencing issues[1][2][3] >> every time the code is modified, because they do not understand that >> mapping cannot be NULL when a folio is in page cache in the code. >> Refactor the code to make it explicit. >> >> No functional change is intended. >> >> [1]https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/ >> [2]https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/ >> [3]https://lore.kernel.org/linux-mm/20250716145804.4836-1-antonio@mandelbit.com/ >> >> Suggested-by: David Hildenbrand <david@redhat.com> >> Signed-off-by: Zi Yan <ziy@nvidia.com> >> --- > > Thanks! > > Acked-by: David Hildenbrand <david@redhat.com> > Thanks. Best Regards, Yan, Zi
On Wed, Jul 16, 2025 at 01:11:12PM -0400, Zi Yan wrote: > Smatch/coverity checkers report NULL mapping referencing issues[1][2][3] > every time the code is modified, because they do not understand that > mapping cannot be NULL when a folio is in page cache in the code. > Refactor the code to make it explicit. > > No functional change is intended. > > [1]https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/ > [2]https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/ > [3]https://lore.kernel.org/linux-mm/20250716145804.4836-1-antonio@mandelbit.com/ > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Zi Yan <ziy@nvidia.com> > --- This silences the Smatch warning. :) regards, dan carpenter
On 16 Jul 2025, at 13:59, Dan Carpenter wrote: > On Wed, Jul 16, 2025 at 01:11:12PM -0400, Zi Yan wrote: >> Smatch/coverity checkers report NULL mapping referencing issues[1][2][3] >> every time the code is modified, because they do not understand that >> mapping cannot be NULL when a folio is in page cache in the code. >> Refactor the code to make it explicit. >> >> No functional change is intended. >> >> [1]https://lore.kernel.org/linux-mm/2afe3d59-aca5-40f7-82a3-a6d976fb0f4f@stanley.mountain/ >> [2]https://lore.kernel.org/oe-kbuild/64b54034-f311-4e7d-b935-c16775dbb642@suswa.mountain/ >> [3]https://lore.kernel.org/linux-mm/20250716145804.4836-1-antonio@mandelbit.com/ >> >> Suggested-by: David Hildenbrand <david@redhat.com> >> Signed-off-by: Zi Yan <ziy@nvidia.com> >> --- > > This silences the Smatch warning. :) Thank you for the confirmation. Best Regards, Yan, Zi
© 2016 - 2025 Red Hat, Inc.