diff mbox series

[1/3,fix] mm,thp,rmap: subpages_mapcount of PTE-mapped subpages: fix

Message ID 7a4f5e5e-de33-dace-c526-4a3d3cf5f6e0@google.com
State New
Headers
Series [1/3,fix] mm,thp,rmap: subpages_mapcount of PTE-mapped subpages: fix |

Commit Message

Hugh Dickins Nov. 19, 2022, 1:35 a.m. UTC 
  Yu Zhao reports compiler warning in page_add_anon_rmap():

mm/rmap.c:1236:13: warning: variable 'first' is used uninitialized
whenever 'if' condition is false [-Wsometimes-uninitialized]
        } else if (PageTransHuge(page)) {
                   ^~~~~~~~~~~~~~~~~~~
mm/rmap.c:1248:18: note: uninitialized use occurs here
        VM_BUG_ON_PAGE(!first && (flags & RMAP_EXCLUSIVE), page);
                        ^~~~~

We do need to fix that, even though it's only uninitialized in an
impossible condition: I've chosen to initialize "first" true, to
minimize the BUGs it might then hit; but you could just as well
choose to initialize it false, to maximize the BUGs it might hit.

Reported-by: Yu Zhao <yuzhao@google.com>
Signed-off-by: Hugh Dickins <hughd@google.com>
---
 mm/rmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kirill A. Shutemov Nov. 21, 2022, 12:38 p.m. UTC | #1 
On Fri, Nov 18, 2022 at 05:35:05PM -0800, Hugh Dickins wrote:
> Yu Zhao reports compiler warning in page_add_anon_rmap():
> 
> mm/rmap.c:1236:13: warning: variable 'first' is used uninitialized
> whenever 'if' condition is false [-Wsometimes-uninitialized]
>         } else if (PageTransHuge(page)) {
>                    ^~~~~~~~~~~~~~~~~~~
> mm/rmap.c:1248:18: note: uninitialized use occurs here
>         VM_BUG_ON_PAGE(!first && (flags & RMAP_EXCLUSIVE), page);
>                         ^~~~~
> 
> We do need to fix that, even though it's only uninitialized in an
> impossible condition: I've chosen to initialize "first" true, to
> minimize the BUGs it might then hit; but you could just as well
> choose to initialize it false, to maximize the BUGs it might hit.
> 
> Reported-by: Yu Zhao <yuzhao@google.com>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
>  mm/rmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 66be8cae640f..25b720d5ba17 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1281,7 +1281,7 @@ void page_add_anon_rmap(struct page *page,
>  	struct compound_mapcounts mapcounts;
>  	int nr = 0, nr_pmdmapped = 0;
>  	bool compound = flags & RMAP_COMPOUND;
> -	bool first;
> +	bool first = true;
>  
>  	if (unlikely(PageKsm(page)))
>  		lock_page_memcg(page);

Other option is to drop PageTransHuge() check that you already claim to be
redundant.

Or have else BUG() to catch cases where the helper called with
compound=true on non-THP page.
Hugh Dickins Nov. 22, 2022, 9:13 a.m. UTC | #2 
On Mon, 21 Nov 2022, Kirill A. Shutemov wrote:
> On Fri, Nov 18, 2022 at 05:35:05PM -0800, Hugh Dickins wrote:
...
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1281,7 +1281,7 @@ void page_add_anon_rmap(struct page *page,
> >  	struct compound_mapcounts mapcounts;
> >  	int nr = 0, nr_pmdmapped = 0;
> >  	bool compound = flags & RMAP_COMPOUND;
> > -	bool first;
> > +	bool first = true;
> >  
> >  	if (unlikely(PageKsm(page)))
> >  		lock_page_memcg(page);
> 
> Other option is to drop PageTransHuge() check that you already claim to be
> redundant.
> 
> Or have else BUG() to catch cases where the helper called with
> compound=true on non-THP page.

I'm sticking with the "first = true".  I did receive a report of bloating
some tiny config a little, on the very first series, so I've been on guard
since then about adding THP code where the optimizer cannot see to remove
it: so do want to keep the PageTransHuge check in there.  Could be done
in other ways, yes, but I'd ended up feeling this was the best compromise.

Hugh
diff mbox series

Patch

diff --git a/mm/rmap.c b/mm/rmap.c
index 66be8cae640f..25b720d5ba17 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1281,7 +1281,7 @@  void page_add_anon_rmap(struct page *page,
 	struct compound_mapcounts mapcounts;
 	int nr = 0, nr_pmdmapped = 0;
 	bool compound = flags & RMAP_COMPOUND;
-	bool first;
+	bool first = true;
 
 	if (unlikely(PageKsm(page)))
 		lock_page_memcg(page);