diff mbox series

[2/8] mm/percpu: use list_first_entry_or_null in pcpu_reclaim_populated()

Message ID 20221024081435.204970-3-bhe@redhat.com
State New
Headers
Series Cleanup and optimization patches for percpu |

Commit Message

Baoquan He Oct. 24, 2022, 8:14 a.m. UTC 
  To replace list_empty()/list_first_entry() pair to simplify code.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/percpu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

kernel test robot Oct. 24, 2022, 10:17 a.m. UTC | #1 
Hi Baoquan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on vbabka-slab/for-next]
[also build test WARNING on akpm-mm/mm-everything linus/master v6.1-rc2 next-20221024]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/Cleanup-and-optimization-patches-for-percpu/20221024-161636
base:   git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git for-next
patch link:    https://lore.kernel.org/r/20221024081435.204970-3-bhe%40redhat.com
patch subject: [PATCH 2/8] mm/percpu: use list_first_entry_or_null in pcpu_reclaim_populated()
config: um-i386_defconfig
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/75b290e1aa943a113526ab78c22b38d07bf7a462
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Baoquan-He/Cleanup-and-optimization-patches-for-percpu/20221024-161636
        git checkout 75b290e1aa943a113526ab78c22b38d07bf7a462
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   mm/percpu.c: In function 'pcpu_reclaim_populated':
>> mm/percpu.c:2146:16: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
    2146 |         while (chunk = list_first_entry_or_null(
         |                ^~~~~


vim +2146 mm/percpu.c

  2114	
  2115	/**
  2116	 * pcpu_reclaim_populated - scan over to_depopulate chunks and free empty pages
  2117	 *
  2118	 * Scan over chunks in the depopulate list and try to release unused populated
  2119	 * pages back to the system.  Depopulated chunks are sidelined to prevent
  2120	 * repopulating these pages unless required.  Fully free chunks are reintegrated
  2121	 * and freed accordingly (1 is kept around).  If we drop below the empty
  2122	 * populated pages threshold, reintegrate the chunk if it has empty free pages.
  2123	 * Each chunk is scanned in the reverse order to keep populated pages close to
  2124	 * the beginning of the chunk.
  2125	 *
  2126	 * CONTEXT:
  2127	 * pcpu_lock (can be dropped temporarily)
  2128	 *
  2129	 */
  2130	static void pcpu_reclaim_populated(void)
  2131	{
  2132		struct pcpu_chunk *chunk;
  2133		struct pcpu_block_md *block;
  2134		int freed_page_start, freed_page_end;
  2135		int i, end;
  2136		bool reintegrate;
  2137	
  2138		lockdep_assert_held(&pcpu_lock);
  2139	
  2140		/*
  2141		 * Once a chunk is isolated to the to_depopulate list, the chunk is no
  2142		 * longer discoverable to allocations whom may populate pages.  The only
  2143		 * other accessor is the free path which only returns area back to the
  2144		 * allocator not touching the populated bitmap.
  2145		 */
> 2146		while (chunk = list_first_entry_or_null(
  2147				&pcpu_chunk_lists[pcpu_to_depopulate_slot],
  2148				struct pcpu_chunk, list)) {
  2149			WARN_ON(chunk->immutable);
  2150	
  2151			/*
  2152			 * Scan chunk's pages in the reverse order to keep populated
  2153			 * pages close to the beginning of the chunk.
  2154			 */
  2155			freed_page_start = chunk->nr_pages;
  2156			freed_page_end = 0;
  2157			reintegrate = false;
  2158			for (i = chunk->nr_pages - 1, end = -1; i >= 0; i--) {
  2159				/* no more work to do */
  2160				if (chunk->nr_empty_pop_pages == 0)
  2161					break;
  2162	
  2163				/* reintegrate chunk to prevent atomic alloc failures */
  2164				if (pcpu_nr_empty_pop_pages < PCPU_EMPTY_POP_PAGES_HIGH) {
  2165					reintegrate = true;
  2166					goto end_chunk;
  2167				}
  2168	
  2169				/*
  2170				 * If the page is empty and populated, start or
  2171				 * extend the (i, end) range.  If i == 0, decrease
  2172				 * i and perform the depopulation to cover the last
  2173				 * (first) page in the chunk.
  2174				 */
  2175				block = chunk->md_blocks + i;
  2176				if (block->contig_hint == PCPU_BITMAP_BLOCK_BITS &&
  2177				    test_bit(i, chunk->populated)) {
  2178					if (end == -1)
  2179						end = i;
  2180					if (i > 0)
  2181						continue;
  2182					i--;
  2183				}
  2184	
  2185				/* depopulate if there is an active range */
  2186				if (end == -1)
  2187					continue;
  2188	
  2189				spin_unlock_irq(&pcpu_lock);
  2190				pcpu_depopulate_chunk(chunk, i + 1, end + 1);
  2191				cond_resched();
  2192				spin_lock_irq(&pcpu_lock);
  2193	
  2194				pcpu_chunk_depopulated(chunk, i + 1, end + 1);
  2195				freed_page_start = min(freed_page_start, i + 1);
  2196				freed_page_end = max(freed_page_end, end + 1);
  2197	
  2198				/* reset the range and continue */
  2199				end = -1;
  2200			}
  2201	
  2202	end_chunk:
  2203			/* batch tlb flush per chunk to amortize cost */
  2204			if (freed_page_start < freed_page_end) {
  2205				spin_unlock_irq(&pcpu_lock);
  2206				pcpu_post_unmap_tlb_flush(chunk,
  2207							  freed_page_start,
  2208							  freed_page_end);
  2209				cond_resched();
  2210				spin_lock_irq(&pcpu_lock);
  2211			}
  2212	
  2213			if (reintegrate || chunk->free_bytes == pcpu_unit_size)
  2214				pcpu_reintegrate_chunk(chunk);
  2215			else
  2216				list_move_tail(&chunk->list,
  2217					       &pcpu_chunk_lists[pcpu_sidelined_slot]);
  2218		}
  2219	}
  2220
Dennis Zhou Oct. 24, 2022, 4:52 p.m. UTC | #2 
On Mon, Oct 24, 2022 at 04:14:29PM +0800, Baoquan He wrote:
> To replace list_empty()/list_first_entry() pair to simplify code.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  mm/percpu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/percpu.c b/mm/percpu.c
> index 26d8cd2ca323..a3fde4ac03a4 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -2143,9 +2143,9 @@ static void pcpu_reclaim_populated(void)
>  	 * other accessor is the free path which only returns area back to the
>  	 * allocator not touching the populated bitmap.
>  	 */
> -	while (!list_empty(&pcpu_chunk_lists[pcpu_to_depopulate_slot])) {
> -		chunk = list_first_entry(&pcpu_chunk_lists[pcpu_to_depopulate_slot],
> -					 struct pcpu_chunk, list);
> +	while (chunk = list_first_entry_or_null(
> +			&pcpu_chunk_lists[pcpu_to_depopulate_slot],
> +			struct pcpu_chunk, list)) {
>  		WARN_ON(chunk->immutable);
>  
>  		/*
> -- 
> 2.34.1
> 

With added parenthesis,

Acked-by: Dennis Zhou <dennis@kernel.org>

Thanks,
Dennis
diff mbox series

Patch

diff --git a/mm/percpu.c b/mm/percpu.c
index 26d8cd2ca323..a3fde4ac03a4 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -2143,9 +2143,9 @@  static void pcpu_reclaim_populated(void)
 	 * other accessor is the free path which only returns area back to the
 	 * allocator not touching the populated bitmap.
 	 */
-	while (!list_empty(&pcpu_chunk_lists[pcpu_to_depopulate_slot])) {
-		chunk = list_first_entry(&pcpu_chunk_lists[pcpu_to_depopulate_slot],
-					 struct pcpu_chunk, list);
+	while (chunk = list_first_entry_or_null(
+			&pcpu_chunk_lists[pcpu_to_depopulate_slot],
+			struct pcpu_chunk, list)) {
 		WARN_ON(chunk->immutable);
 
 		/*