Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Minchan Kim <minchan.kim@gmail.com>
Date:   Thu, 11 Feb 2021 14:17:05 -0800
From:   Minchan Kim <minchan@kernel.org>
To:     Chris Goldsworthy <cgoldswo@codeaurora.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>, linux-mm@kvack.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH v2] [RFC] mm: fs: Invalidate BH LRU during page migration
Message-ID: <YCWs4ZJ9xk7h6reT@google.com>
References: <cover.1613020616.git.cgoldswo@codeaurora.org>
 <c083b0ab6e410e33ca880d639f90ef4f6f3b33ff.1613020616.git.cgoldswo@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c083b0ab6e410e33ca880d639f90ef4f6f3b33ff.1613020616.git.cgoldswo@codeaurora.org>
Precedence: bulk

On Wed, Feb 10, 2021 at 09:35:40PM -0800, Chris Goldsworthy wrote:
> Pages containing buffer_heads that are in one of the per-CPU
> buffer_head LRU caches will be pinned and thus cannot be migrated.
> This can prevent CMA allocations from succeeding, which are often used
> on platforms with co-processors (such as a DSP) that can only use
> physically contiguous memory. It can also prevent memory
> hot-unplugging from succeeding, which involves migrating at least
> MIN_MEMORY_BLOCK_SIZE bytes of memory, which ranges from 8 MiB to 1
> GiB based on the architecture in use.
> 
> Correspondingly, invalidate the BH LRU caches before a migration
> starts and stop any buffer_head from being cached in the LRU caches,
> until migration has finished.
> 
> Signed-off-by: Chris Goldsworthy <cgoldswo@codeaurora.org>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  fs/buffer.c                 | 54 +++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/buffer_head.h |  8 +++++++
>  include/linux/migrate.h     |  2 ++
>  mm/migrate.c                | 19 ++++++++++++++++
>  mm/page_alloc.c             |  3 +++
>  mm/swap.c                   |  7 +++++-
>  6 files changed, 90 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 96c7604..634e474 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1274,6 +1274,10 @@ struct bh_lru {
>  
>  static DEFINE_PER_CPU(struct bh_lru, bh_lrus) = {{ NULL }};
>  
> +/* These are used to control the BH LRU invalidation during page migration */
> +static struct cpumask lru_needs_invalidation;
> +static bool bh_lru_disabled = false;
> +
>  #ifdef CONFIG_SMP
>  #define bh_lru_lock()	local_irq_disable()
>  #define bh_lru_unlock()	local_irq_enable()
> @@ -1292,7 +1296,9 @@ static inline void check_irqs_on(void)
>  /*
>   * Install a buffer_head into this cpu's LRU.  If not already in the LRU, it is
>   * inserted at the front, and the buffer_head at the back if any is evicted.
> - * Or, if already in the LRU it is moved to the front.
> + * Or, if already in the LRU it is moved to the front. Note that if LRU is
> + * disabled because of an ongoing page migration, we won't insert bh into the
> + * LRU.
>   */
>  static void bh_lru_install(struct buffer_head *bh)
>  {
> @@ -1303,6 +1309,9 @@ static void bh_lru_install(struct buffer_head *bh)
>  	check_irqs_on();
>  	bh_lru_lock();
>  
> +	if (bh_lru_disabled)
> +		goto out;
> +
>  	b = this_cpu_ptr(&bh_lrus);
>  	for (i = 0; i < BH_LRU_SIZE; i++) {
>  		swap(evictee, b->bhs[i]);
> @@ -1313,6 +1322,7 @@ static void bh_lru_install(struct buffer_head *bh)
>  	}
>  
>  	get_bh(bh);
> +out:
>  	bh_lru_unlock();
>  	brelse(evictee);
>  }
> @@ -1328,6 +1338,10 @@ lookup_bh_lru(struct block_device *bdev, sector_t block, unsigned size)
>  
>  	check_irqs_on();
>  	bh_lru_lock();
> +
> +	if (bh_lru_disabled)
> +		goto out;
> +
>  	for (i = 0; i < BH_LRU_SIZE; i++) {
>  		struct buffer_head *bh = __this_cpu_read(bh_lrus.bhs[i]);
>  
> @@ -1346,6 +1360,7 @@ lookup_bh_lru(struct block_device *bdev, sector_t block, unsigned size)
>  			break;
>  		}
>  	}
> +out:
>  	bh_lru_unlock();
>  	return ret;
>  }
> @@ -1446,7 +1461,7 @@ EXPORT_SYMBOL(__bread_gfp);
>   * This doesn't race because it runs in each cpu either in irq
>   * or with preempt disabled.
>   */
> -static void invalidate_bh_lru(void *arg)
> +void invalidate_bh_lru(void *arg)
>  {
>  	struct bh_lru *b = &get_cpu_var(bh_lrus);
>  	int i;
> @@ -1477,6 +1492,41 @@ void invalidate_bh_lrus(void)
>  }
>  EXPORT_SYMBOL_GPL(invalidate_bh_lrus);
>  
> +bool need_bh_lru_invalidation(int cpu)
> +{
> +	return cpumask_test_cpu(cpu, &lru_needs_invalidation);
> +}
> +
> +void bh_lru_disable(void)
> +{
> +	int cpu;
> +
> +	bh_lru_disabled = true;

What happens if the function is nested? Shouldn't we make it count?
So only disble when the count is zero.