On Thu, Dec 17, 2020 at 09:11:54AM +0800, Yafang Shao wrote:
> From: "Matthew Wilcox (Oracle)" <willy@xxxxxxxxxxxxx>
>
> Since XFS needs to pretend to be kswapd in some of its worker threads,
> create methods to save & restore kswapd state. Don't bother restoring
> kswapd state in kswapd -- the only time we reach this code is when we're
> exiting and the task_struct is about to be destroyed anyway.
>
> Cc: Dave Chinner <david@xxxxxxxxxxxxx>
> Acked-by: Michal Hocko <mhocko@xxxxxxxx>
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_btree.c | 14 ++++++++------
> include/linux/sched/mm.h | 23 +++++++++++++++++++++++
> mm/vmscan.c | 16 +---------------
> 3 files changed, 32 insertions(+), 21 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index 51dbff9b0908..0f35b7a38e76 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -2813,8 +2813,9 @@ xfs_btree_split_worker(
> {
> struct xfs_btree_split_args *args = container_of(work,
> struct xfs_btree_split_args, work);
> + bool is_kswapd = args->kswapd;
> unsigned long pflags;
> - unsigned long new_pflags = PF_MEMALLOC_NOFS;
> + int memalloc_nofs;
>
> /*
> * we are in a transaction context here, but may also be doing work
> @@ -2822,16 +2823,17 @@ xfs_btree_split_worker(
> * temporarily to ensure that we don't block waiting for memory reclaim
> * in any way.
> */
> - if (args->kswapd)
> - new_pflags |= PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD;
> -
> - current_set_flags_nested(&pflags, new_pflags);
> + if (is_kswapd)
> + pflags = become_kswapd();
> + memalloc_nofs = memalloc_nofs_save();
>
> args->result = __xfs_btree_split(args->cur, args->level, args->ptrp,
> args->key, args->curp, args->stat);
> complete(args->done);
>
> - current_restore_flags_nested(&pflags, new_pflags);
> + memalloc_nofs_restore(memalloc_nofs);
> + if (is_kswapd)
> + restore_kswapd(pflags);
> }
>
> /*
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index d5ece7a9a403..2faf03e79a1e 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -278,6 +278,29 @@ static inline void memalloc_nocma_restore(unsigned int flags)
> }
> #endif
>
> +/*
> + * Tell the memory management code that this thread is working on behalf
> + * of background memory reclaim (like kswapd). That means that it will
> + * get access to memory reserves should it need to allocate memory in
> + * order to make forward progress. With this great power comes great
> + * responsibility to not exhaust those reserves.
> + */
> +#define KSWAPD_PF_FLAGS (PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD)
> +
> +static inline unsigned long become_kswapd(void)
> +{
> + unsigned long flags = current->flags & KSWAPD_PF_FLAGS;
> +
> + current->flags |= KSWAPD_PF_FLAGS;
> +
> + return flags;
> +}
You can get rid of the empty lines out of this function.
> +static inline void restore_kswapd(unsigned long flags)
> +{
> + current->flags &= ~(flags ^ KSWAPD_PF_FLAGS);
> +}
Urk, that requires thinking about to determine whether it is
correct. And it is 3 runtime logic operations (^, ~ and &) too. The
way all the memalloc_*_restore() functions restore the previous
flags is obviously correct and only requires 2 runtime logic
operations because the compiler calculates the ~ operation on the
constant. So why do it differently here? i.e.:
current->flags = (current->flags & ~KSWAPD_PF_FLAGS) | flags;
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3870,19 +3870,7 @@ static int kswapd(void *p)
> if (!cpumask_empty(cpumask))
> set_cpus_allowed_ptr(tsk, cpumask);
>
> - /*
> - * Tell the memory management that we're a "memory allocator",
> - * and that if we need more memory we should get access to it
> - * regardless (see "__alloc_pages()"). "kswapd" should
> - * never get caught in the normal page freeing logic.
> - *
> - * (Kswapd normally doesn't need memory anyway, but sometimes
> - * you need a small amount of memory in order to be able to
> - * page out something else, and this flag essentially protects
> - * us from recursively trying to free more memory as we're
> - * trying to free the first piece of memory in the first place).
> - */
> - tsk->flags |= PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD;
> + become_kswapd();
> set_freezable();
>
> WRITE_ONCE(pgdat->kswapd_order, 0);
> @@ -3932,8 +3920,6 @@ static int kswapd(void *p)
> goto kswapd_try_sleep;
> }
>
> - tsk->flags &= ~(PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD);
> -
Missing a restore_kswapd()?
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
