Re: [PATCH v2] [RFC] mm: fs: Invalidate BH LRU during page migration

Minchan Kim
2021-02-11T22:18:14Z

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.