[-next] mm/vmscan: fix some -Wenum-conversion warnings
diff mbox series

Message ID 1573826697-869-1-git-send-email-cai@lca.pw
State New
Headers show
Series
  • [-next] mm/vmscan: fix some -Wenum-conversion warnings
Related show

Commit Message

Qian Cai Nov. 15, 2019, 2:04 p.m. UTC
The -next commit "mm: vmscan: enforce inactive:active ratio at the
reclaim root" [1] introduced some GCC -Wenum-conversion warnings,

mm/vmscan.c:2216:39: warning: implicit conversion from enumeration type
'enum lru_list' to different enumeration type 'enum node_stat_item'
[-Wenum-conversion]
        inactive = lruvec_page_state(lruvec, inactive_lru);
                   ~~~~~~~~~~~~~~~~~         ^~~~~~~~~~~~
mm/vmscan.c:2217:37: warning: implicit conversion from enumeration type
'enum lru_list' to different enumeration type 'enum node_stat_item'
[-Wenum-conversion]
        active = lruvec_page_state(lruvec, active_lru);
                 ~~~~~~~~~~~~~~~~~         ^~~~~~~~~~
mm/vmscan.c:2746:42: warning: implicit conversion from enumeration type
'enum lru_list' to different enumeration type 'enum node_stat_item'
[-Wenum-conversion]
        file = lruvec_page_state(target_lruvec, LRU_INACTIVE_FILE);
               ~~~~~~~~~~~~~~~~~                ^~~~~~~~~~~~~~~~~

Fix them by adding casts where it is safe.

[1] http://lkml.kernel.org/r/20191107205334.158354-4-hannes@cmpxchg.org

Signed-off-by: Qian Cai <cai@lca.pw>
---
 mm/vmscan.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Johannes Weiner Nov. 15, 2019, 4:17 p.m. UTC | #1
On Fri, Nov 15, 2019 at 09:04:57AM -0500, Qian Cai wrote:
> The -next commit "mm: vmscan: enforce inactive:active ratio at the
> reclaim root" [1] introduced some GCC -Wenum-conversion warnings,
> 
> mm/vmscan.c:2216:39: warning: implicit conversion from enumeration type
> 'enum lru_list' to different enumeration type 'enum node_stat_item'
> [-Wenum-conversion]
>         inactive = lruvec_page_state(lruvec, inactive_lru);
>                    ~~~~~~~~~~~~~~~~~         ^~~~~~~~~~~~
> mm/vmscan.c:2217:37: warning: implicit conversion from enumeration type
> 'enum lru_list' to different enumeration type 'enum node_stat_item'
> [-Wenum-conversion]
>         active = lruvec_page_state(lruvec, active_lru);
>                  ~~~~~~~~~~~~~~~~~         ^~~~~~~~~~
> mm/vmscan.c:2746:42: warning: implicit conversion from enumeration type
> 'enum lru_list' to different enumeration type 'enum node_stat_item'
> [-Wenum-conversion]
>         file = lruvec_page_state(target_lruvec, LRU_INACTIVE_FILE);

Interesting, it doesn't show these for me with gcc-9.2.0.

We should definitely fix these!

> @@ -2213,8 +2213,9 @@ static bool inactive_is_low(struct lruvec *lruvec, enum lru_list inactive_lru)
>  	unsigned long inactive_ratio;
>  	unsigned long gb;
>  
> -	inactive = lruvec_page_state(lruvec, inactive_lru);
> -	active = lruvec_page_state(lruvec, active_lru);
> +	inactive = lruvec_page_state(lruvec,
> +				     (enum node_stat_item)inactive_lru);
> +	active = lruvec_page_state(lruvec, (enum node_stat_item)active_lru);

This is fragile, as it relies on the absolute values being identical,
which we don't guarantee. However, we do guarantee the relative order
between the LRU items, and we use NR_LRU_BASE for the translation.

Please use NR_LRU_BASE + (in)active_lru here.

> @@ -2743,7 +2744,8 @@ static bool shrink_node(pg_data_t *pgdat, struct scan_control *sc)
>  	 * thrashing, try to reclaim those first before touching
>  	 * anonymous pages.
>  	 */
> -	file = lruvec_page_state(target_lruvec, LRU_INACTIVE_FILE);
> +	file = lruvec_page_state(target_lruvec,
> +				 (enum node_stat_item)LRU_INACTIVE_FILE);

This should just directly use NR_INACTIVE_FILE instead.

Thanks
Qian Cai Nov. 15, 2019, 4:22 p.m. UTC | #2
On Fri, 2019-11-15 at 11:17 -0500, Johannes Weiner wrote:
> On Fri, Nov 15, 2019 at 09:04:57AM -0500, Qian Cai wrote:
> > The -next commit "mm: vmscan: enforce inactive:active ratio at the
> > reclaim root" [1] introduced some GCC -Wenum-conversion warnings,
> > 
> > mm/vmscan.c:2216:39: warning: implicit conversion from enumeration type
> > 'enum lru_list' to different enumeration type 'enum node_stat_item'
> > [-Wenum-conversion]
> >         inactive = lruvec_page_state(lruvec, inactive_lru);
> >                    ~~~~~~~~~~~~~~~~~         ^~~~~~~~~~~~
> > mm/vmscan.c:2217:37: warning: implicit conversion from enumeration type
> > 'enum lru_list' to different enumeration type 'enum node_stat_item'
> > [-Wenum-conversion]
> >         active = lruvec_page_state(lruvec, active_lru);
> >                  ~~~~~~~~~~~~~~~~~         ^~~~~~~~~~
> > mm/vmscan.c:2746:42: warning: implicit conversion from enumeration type
> > 'enum lru_list' to different enumeration type 'enum node_stat_item'
> > [-Wenum-conversion]
> >         file = lruvec_page_state(target_lruvec, LRU_INACTIVE_FILE);
> 
> Interesting, it doesn't show these for me with gcc-9.2.0.

Sorry, I meant to say it is clang.

> 
> We should definitely fix these!
> 
> > @@ -2213,8 +2213,9 @@ static bool inactive_is_low(struct lruvec *lruvec, enum lru_list inactive_lru)
> >  	unsigned long inactive_ratio;
> >  	unsigned long gb;
> >  
> > -	inactive = lruvec_page_state(lruvec, inactive_lru);
> > -	active = lruvec_page_state(lruvec, active_lru);
> > +	inactive = lruvec_page_state(lruvec,
> > +				     (enum node_stat_item)inactive_lru);
> > +	active = lruvec_page_state(lruvec, (enum node_stat_item)active_lru);
> 
> This is fragile, as it relies on the absolute values being identical,
> which we don't guarantee. However, we do guarantee the relative order
> between the LRU items, and we use NR_LRU_BASE for the translation.
> 
> Please use NR_LRU_BASE + (in)active_lru here.
> 
> > @@ -2743,7 +2744,8 @@ static bool shrink_node(pg_data_t *pgdat, struct scan_control *sc)
> >  	 * thrashing, try to reclaim those first before touching
> >  	 * anonymous pages.
> >  	 */
> > -	file = lruvec_page_state(target_lruvec, LRU_INACTIVE_FILE);
> > +	file = lruvec_page_state(target_lruvec,
> > +				 (enum node_stat_item)LRU_INACTIVE_FILE);
> 
> This should just directly use NR_INACTIVE_FILE instead.
> 
> Thanks

Patch
diff mbox series

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 122b3920aaa4..16c005ddfd9e 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2213,8 +2213,9 @@  static bool inactive_is_low(struct lruvec *lruvec, enum lru_list inactive_lru)
 	unsigned long inactive_ratio;
 	unsigned long gb;
 
-	inactive = lruvec_page_state(lruvec, inactive_lru);
-	active = lruvec_page_state(lruvec, active_lru);
+	inactive = lruvec_page_state(lruvec,
+				     (enum node_stat_item)inactive_lru);
+	active = lruvec_page_state(lruvec, (enum node_stat_item)active_lru);
 
 	gb = (inactive + active) >> (30 - PAGE_SHIFT);
 	if (gb)
@@ -2743,7 +2744,8 @@  static bool shrink_node(pg_data_t *pgdat, struct scan_control *sc)
 	 * thrashing, try to reclaim those first before touching
 	 * anonymous pages.
 	 */
-	file = lruvec_page_state(target_lruvec, LRU_INACTIVE_FILE);
+	file = lruvec_page_state(target_lruvec,
+				 (enum node_stat_item)LRU_INACTIVE_FILE);
 	if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE))
 		sc->cache_trim_mode = 1;
 	else