Re: [linux-next:master 1753/1936] mm/swapfile.c:1517:20: warning: function 'swap_is_last_ref' is not needed and will not be emitted

[Baoquan He <bhe@redhat.com>]

On 04/08/25 at 05:39pm, Andrew Morton wrote:
> On Tue, 8 Apr 2025 18:06:11 +0800 kernel test robot <lkp@intel.com> wrote:
> 
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> > head:   7702d0130dc002bab2c3571ddb6ff68f82d99aea
> > commit: 6bb001b6b64ec20d900d0d81294a0f031af59c3e [1753/1936] mm: swap: enable swap_entry_range_free() to drop any kind of last ref
> > config: hexagon-randconfig-001-20250408 (https://download.01.org/0day-ci/archive/20250408/202504081841.jbL7yL0r-lkp@intel.com/config)
> > compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 92c93f5286b9ff33f27ff694d2dc33da1c07afdd)
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250408/202504081841.jbL7yL0r-lkp@intel.com/reproduce)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202504081841.jbL7yL0r-lkp@intel.com/
> > 
> > All warnings (new ones prefixed by >>):
> > 
> > >> mm/swapfile.c:1517:20: warning: function 'swap_is_last_ref' is not needed and will not be emitted [-Wunneeded-internal-declaration]
> 
> It's annoying that gcc warns about this.
> 
> >     1517 | static inline bool swap_is_last_ref(unsigned char count)
> >          |                    ^~~~~~~~~~~~~~~~
> >    1 warning generated.
> > 
> > 
> > vim +/swap_is_last_ref +1517 mm/swapfile.c
> > 
> >   1512	
> >   1513	/*
> >   1514	 * Drop the last ref(1, SWAP_HAS_CACHE or SWAP_MAP_SHMEM) of swap entries,
> >   1515	 * caller have to ensure all entries belong to the same cgroup and cluster.
> >   1516	 */
> > > 1517	static inline bool swap_is_last_ref(unsigned char count)
> >   1518	{
> >   1519		return (count == SWAP_HAS_CACHE) || (count == 1) ||
> >   1520		       (count == SWAP_MAP_SHMEM);
> >   1521	}
> >   1522	
> 
> : static inline bool swap_is_last_ref(unsigned char count)
> : {
> : 	return (count == SWAP_HAS_CACHE) || (count == 1) ||
> : 	       (count == SWAP_MAP_SHMEM);
> : }
> : 
> : static void swap_entries_free(struct swap_info_struct *si,
> : 			      struct swap_cluster_info *ci,
> : 			      swp_entry_t entry, unsigned int nr_pages)
> : {
> : 	unsigned long offset = swp_offset(entry);
> : 	unsigned char *map = si->swap_map + offset;
> : 	unsigned char *map_end = map + nr_pages;
> : 
> : 	/* It should never free entries across different clusters */
> : 	VM_BUG_ON(ci != offset_to_cluster(si, offset + nr_pages - 1));
> : 	VM_BUG_ON(cluster_is_empty(ci));
> : 	VM_BUG_ON(ci->count < nr_pages);
> : 
> : 	ci->count -= nr_pages;
> : 	do {
> : 		VM_BUG_ON(!swap_is_last_ref(*map));
> 
> 
> And it's annoying that VM_BUG_ON() doesn't reference its arg when
> CONFIG_DEBUG_VM=n.
> 
> I'll add this:
> 
> --- a/mm/swapfile.c~mm-swap-enable-swap_entry_range_free-to-drop-any-kind-of-last-ref-fix
> +++ a/mm/swapfile.c
> @@ -1514,11 +1514,13 @@ fallback:
>   * Drop the last ref(1, SWAP_HAS_CACHE or SWAP_MAP_SHMEM) of swap entries,
>   * caller have to ensure all entries belong to the same cgroup and cluster.
>   */
> +#ifdef CONFIG_DEBUG_VM
>  static inline bool swap_is_last_ref(unsigned char count)
>  {
>  	return (count == SWAP_HAS_CACHE) || (count == 1) ||
>  	       (count == SWAP_MAP_SHMEM);
>  }
> +#endif
>  
>  static void swap_entries_free(struct swap_info_struct *si,
>  			      struct swap_cluster_info *ci,
> _
> 
> Kemeng, can you please review that comment "Drop the last ref...". 
> From its positioning it appears to be describing swap_is_last_ref(),
> but swap_is_last_ref() doesn't do those things.

Indeed, maybe below change can be squashed into above patch from Andrew.

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 2887ed5d34cc..baaf8ad29a6b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1536,8 +1536,8 @@ static bool swap_entries_put_map_nr(struct swap_info_struct *si,
 }
 
 /*
- * Drop the last ref(1, SWAP_HAS_CACHE or SWAP_MAP_SHMEM) of swap entries,
- * caller have to ensure all entries belong to the same cgroup and cluster.
+ * Check if it's the last ref of swap entry in the freeing path.
+ * Qualified vlaue includes 1, SWAP_HAS_CACHE or SWAP_MAP_SHMEM.
  */
 static inline bool swap_is_last_ref(unsigned char count)
 {