Re: [PATCH 2/2] mm: introduce PF_MEMALLOC_NOWARN

[Kent Overstreet <kent.overstreet@linux.dev>]

On Mon, Jan 29, 2024 at 11:48:00AM +0100, Michal Hocko wrote:
> On Sun 28-01-24 14:43:16, Kent Overstreet wrote:
> > On Sun, Jan 28, 2024 at 04:45:32PM +0100, Michal Hocko wrote:
> > > On Fri 26-01-24 17:07:56, Kent Overstreet wrote:
> > > > If we're using PF_MEMALLOC, we might have a fallback and might not to
> > > > warn about a failing allocation - thus we need a PF_* equivalent of
> > > > __GFP_NOWARN.
> > > 
> > > Could you be more specific about the user? Is this an allocation from
> > > the reclaim path or an explicit PF_MEMALLOC one? It would be also really
> > > helpful to explain why GFP_NOWARN cannot be used directly.
> > 
> > Explicit PF_MEMALLOC.
> > 
> > It's for a call to alloc_inode(), which doesn't take gfp flags, and
> > plumbing it would require modifying a s_ops callback and would touch
> > every filesystem -
> 
> OK, I see. This should be part of the changelog.
> 
> > but we want to get away from gfp flags anyways :)
> > 
> > More specifically, the code where I'm using it is doing a "try
> > GFP_NOWAIT first; if that fails drop locks and do GFP_KERNEL" dance;
> > it's part of a cleanup for some weird lifetime stuff related to
> > fs/inode.c.
> > 
> > #define memalloc_flags_do(_flags, _do)						\
> > ({										\
> > 	unsigned _saved_flags = memalloc_flags_save(_flags);			\
> > 	typeof(_do) _ret = _do;							\
> > 	memalloc_noreclaim_restore(_saved_flags);				\
> > 	_ret;									\
> > })
> > 
> > /*
> >  * Allocate a new inode, dropping/retaking btree locks if necessary:
> >  */
> > static struct bch_inode_info *bch2_new_inode(struct btree_trans *trans)
> > {
> > 	struct bch_fs *c = trans->c;
> > 
> > 	struct bch_inode_info *inode =
> > 		memalloc_flags_do(PF_MEMALLOC|PF_MEMALLOC_NOWARN, to_bch_ei(new_inode(c->vfs_sb)));
> 
> Is this what you meant by GFP_NOWAIT allocation? You would be right
> about this allocation not entering the direct reclaim but do you realize
> this is not an ordinary NOWAIT request beause PF_MEMALLOC will allow to
> dip into memory reserves without any limits? (Unless the specific
> allocation down the road is explicitly GFP_NOMEMALLOC)
> A failure here means that the system is really in a desparate state with all
> the memory reserves gone which migt break reclaimers who rely on those
> reserves.
> 
> Are you sure it is a good idea? Unless I am missing something you are
> just giving an ordinary user access to those reserves by creating inodes
> without any bounds.

Did not realize that; thank you.

How about this version?

From 0e87e55058ccddde4b6bcc092f43e66a4e632575 Mon Sep 17 00:00:00 2001
From: Kent Overstreet <kent.overstreet@linux.dev>
Date: Thu, 25 Jan 2024 19:00:24 -0500
Subject: [PATCH] mm: introduce PF_MEMALLOC_NORECLAIM, PF_MEMALLOC_NOWARN

Introduce PF_MEMALLOC_* equivalents of some GFP_ flags:

PF_MEMALLOC_NORECLAIM	-> GFP_NOWAIT
PF_MEMALLOC_NOWARN	-> __GFP_NOWARN

Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>

diff --git a/include/linux/sched.h b/include/linux/sched.h
index cdb8ea53c365..36de7a2343c3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1632,12 +1632,12 @@ extern struct pid *cad_pid;
 #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_LOCAL_THROTTLE	0x00100000	/* Throttle writes only against the bdi I write to,
+#define PF_MEMALLOC_NORECLAIM	0x00100000	/* All allocation requests will inherit __GFP_NOWARN */
+#define PF_MEMALLOC_NOWARN	0x00200000	/* All allocation requests will inherit __GFP_NOWARN */
+#define PF_LOCAL_THROTTLE	0x00400000	/* 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 */
-#define PF_RANDOMIZE		0x00400000	/* Randomize virtual address space */
-#define PF__HOLE__00800000	0x00800000
-#define PF__HOLE__01000000	0x01000000
+#define PF_KTHREAD		0x00800000	/* I am a kernel thread */
+#define PF_RANDOMIZE		0x01000000	/* Randomize virtual address space */
 #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 */
diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index f00d7ecc2adf..c29059a76052 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -236,16 +236,25 @@ static inline gfp_t current_gfp_context(gfp_t flags)
 {
 	unsigned int pflags = READ_ONCE(current->flags);
 
-	if (unlikely(pflags & (PF_MEMALLOC_NOIO | PF_MEMALLOC_NOFS | PF_MEMALLOC_PIN))) {
+	if (unlikely(pflags & (PF_MEMALLOC_NOIO |
+			       PF_MEMALLOC_NOFS |
+			       PF_MEMALLOC_NORECLAIM |
+			       PF_MEMALLOC_NOWARN |
+			       PF_MEMALLOC_PIN))) {
 		/*
-		 * NOIO implies both NOIO and NOFS and it is a weaker context
-		 * so always make sure it makes precedence
+		 * Stronger flags before weaker flags:
+		 * NORECLAIM implies NOIO, which in turn implies NOFS
 		 */
-		if (pflags & PF_MEMALLOC_NOIO)
+		if (pflags & PF_MEMALLOC_NORECLAIM)
+			flags &= ~__GFP_DIRECT_RECLAIM;
+		else if (pflags & PF_MEMALLOC_NOIO)
 			flags &= ~(__GFP_IO | __GFP_FS);
 		else if (pflags & PF_MEMALLOC_NOFS)
 			flags &= ~__GFP_FS;
 
+		if (pflags & PF_MEMALLOC_NOWARN)
+			flags |= __GFP_NOWARN;
+
 		if (pflags & PF_MEMALLOC_PIN)
 			flags &= ~__GFP_MOVABLE;
 	}