Received-SPF: pass (google.com: domain of linux-kernel+bounces-63651-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 13 Feb 2024 16:15:54 +0200
From: Mike Rapoport <rppt@kernel.org>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, Michal Hocko <mhocko@kernel.org>,
	Mel Gorman <mgorman@techsingularity.net>,
	Matthew Wilcox <willy@infradead.org>,
	Kent Overstreet <kent.overstreet@gmail.com>,
	Pasha Tatashin <pasha.tatashin@soleen.com>,
	Mike Rapoport <rppt@linux.ibm.com>
Subject: Re: [PATCH] mm: document memalloc_noreclaim_save() and
 memalloc_pin_save()
Message-ID: <Zct5mtrHoeGD5S0f@kernel.org>
References: <20240212182950.32730-2-vbabka@suse.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240212182950.32730-2-vbabka@suse.cz>

On Mon, Feb 12, 2024 at 07:29:51PM +0100, Vlastimil Babka wrote:
> The memalloc_noreclaim_save() function currently has no documentation
> comment, so the implications of its usage are not obvious. Namely that
> it not only prevents entering reclaim (as the name suggests), but also
> allows using all memory reserves and thus should be only used in
> contexts that are allocating memory to free memory. This may lead to new
> improper usages being added.
> 
> Thus add a documenting comment, based on the description of
> __GFP_MEMALLOC. While at it, also document memalloc_pin_save() so that
> all the memalloc_ scopes are documented. In the comments describing the
> relevant PF_MEMALLOC flags, refer to their scope setting functions.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Except few nits below

Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org>

> ---
>  include/linux/sched.h    |  9 ++++----
>  include/linux/sched/mm.h | 45 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index ffe8f618ab86..f2cb479f56a7 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1623,15 +1623,15 @@ extern struct pid *cad_pid;
>  #define PF_SUPERPRIV		0x00000100	/* Used super-user privileges */
>  #define PF_DUMPCORE		0x00000200	/* Dumped core */
>  #define PF_SIGNALED		0x00000400	/* Killed by a signal */
> -#define PF_MEMALLOC		0x00000800	/* Allocating memory */
> +#define PF_MEMALLOC		0x00000800	/* Allocating memory to free memory. See memalloc_noreclaim_save() */
>  #define PF_NPROC_EXCEEDED	0x00001000	/* set_user() noticed that RLIMIT_NPROC was exceeded */
>  #define PF_USED_MATH		0x00002000	/* If unset the fpu must be initialized before use */
>  #define PF_USER_WORKER		0x00004000	/* Kernel thread cloned from userspace thread */
>  #define PF_NOFREEZE		0x00008000	/* This thread should not be frozen */
>  #define PF__HOLE__00010000	0x00010000
>  #define PF_KSWAPD		0x00020000	/* I am kswapd */
> -#define PF_MEMALLOC_NOFS	0x00040000	/* All allocation requests will inherit GFP_NOFS */
> -#define PF_MEMALLOC_NOIO	0x00080000	/* All allocation requests will inherit GFP_NOIO */
> +#define PF_MEMALLOC_NOFS	0x00040000	/* All allocations inherit GFP_NOFS. See memalloc_nfs_save() */
> +#define PF_MEMALLOC_NOIO	0x00080000	/* All allocations inherit GFP_NOIO. See memalloc_noio_save() */
>  #define PF_LOCAL_THROTTLE	0x00100000	/* Throttle writes only against the bdi I write to,
>  						 * I am cleaning dirty pages from some other bdi. */
>  #define PF_KTHREAD		0x00200000	/* I am a kernel thread */
> @@ -1641,7 +1641,8 @@ extern struct pid *cad_pid;
>  #define PF__HOLE__02000000	0x02000000
>  #define PF_NO_SETAFFINITY	0x04000000	/* Userland is not allowed to meddle with cpus_mask */
>  #define PF_MCE_EARLY		0x08000000      /* Early kill for mce process policy */
> -#define PF_MEMALLOC_PIN		0x10000000	/* Allocation context constrained to zones which allow long term pinning. */
> +#define PF_MEMALLOC_PIN		0x10000000	/* Allocations constrained to zones which allow long term pinning.
> +						 * See memalloc_pin_save() */
>  #define PF__HOLE__20000000	0x20000000
>  #define PF__HOLE__40000000	0x40000000
>  #define PF_SUSPEND_TASK		0x80000000      /* This thread called freeze_processes() and should not be frozen */
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index 9a19f1b42f64..eef8fa5ba5de 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -368,6 +368,27 @@ static inline void memalloc_nofs_restore(unsigned int flags)
>  	current->flags = (current->flags & ~PF_MEMALLOC_NOFS) | flags;
>  }
>  
> +/**
> + * memalloc_noreclaim_save - Marks implicit __GFP_MEMALLOC scope.
> + *
> + * This functions marks the beginning of the __GFP_MEMALLOC allocation scope.

          ^ function

> + * All further allocations will implicitly add the __GFP_MEMALLOC flag, which
> + * prevents entering reclaim and allows access to all memory reserves. This
> + * should only be used when the caller guarantees the allocation will allow more
> + * memory to be freed very shortly, i.e. it needs to allocate some memory in
> + * the process of freeing memory, and cannot reclaim due to potential recursion.
> + *
> + * Users of this scope have to be extremely careful to not deplete the reserves
> + * completely and implement a throttling mechanism which controls the
> + * consumption of the reserve based on the amount of freed memory. Usage of a
> + * pre-allocated pool (e.g. mempool) should be always considered before using
> + * this scope.
> + *
> + * Individual allocations under the scope can opt out using __GFP_NOMEMALLOC
> + *
> + * This function should not be used in an interrupt context as that one does not
> + * give PF_MEMALLOC access to reserves, see __gfp_pfmemalloc_flags().

Missing Return: description

> + */
>  static inline unsigned int memalloc_noreclaim_save(void)
>  {
>  	unsigned int flags = current->flags & PF_MEMALLOC;
> @@ -375,11 +396,27 @@ static inline unsigned int memalloc_noreclaim_save(void)
>  	return flags;
>  }
>  
> +/**
> + * memalloc_noreclaim_restore - Ends the implicit __GFP_MEMALLOC scope.
> + * @flags: Flags to restore.
> + *
> + * Ends the implicit __GFP_MEMALLOC scope started by memalloc_noreclaim_save
> + * function. Always make sure that the given flags is the return value from the
> + * pairing memalloc_noreclaim_save call.
> + */
>  static inline void memalloc_noreclaim_restore(unsigned int flags)
>  {
>  	current->flags = (current->flags & ~PF_MEMALLOC) | flags;
>  }
>  
> +/**
> + * memalloc_pin_save - Marks implicit ~__GFP_MOVABLE scope.
> + *
> + * This functions marks the beginning of the ~__GFP_MOVABLE allocation scope.

          ^ function

> + * All further allocations will implicitly remove the __GFP_MOVABLE flag, which
> + * will constraint the allocations to zones that allow long term pinning, i.e.
> + * not ZONE_MOVABLE zones.

Missing Return: description

> + */
>  static inline unsigned int memalloc_pin_save(void)
>  {
>  	unsigned int flags = current->flags & PF_MEMALLOC_PIN;
> @@ -388,6 +425,14 @@ static inline unsigned int memalloc_pin_save(void)
>  	return flags;
>  }
>  
> +/**
> + * memalloc_pin_restore - Ends the implicit ~__GFP_MOVABLE scope.
> + * @flags: Flags to restore.
> + *
> + * Ends the implicit ~__GFP_MOVABLE scope started by memalloc_pin_save function.
> + * Always make sure that the given flags is the return value from the pairing
> + * memalloc_pin_save call.
> + */
>  static inline void memalloc_pin_restore(unsigned int flags)
>  {
>  	current->flags = (current->flags & ~PF_MEMALLOC_PIN) | flags;
> -- 
> 2.43.0
> 
> 

-- 
Sincerely yours,
Mike.