| Message ID | 20230501165450.15352-4-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Memory allocation profiling | expand |
On Mon, 1 May 2023 09:54:13 -0700 Suren Baghdasaryan <surenb@google.com> wrote: > From: Kent Overstreet <kent.overstreet@linux.dev> > > We're introducing alloc tagging, which tracks memory allocations by > callsite. Converting alloc_inode_sb() to a macro means allocations will > be tracked by its caller, which is a bit more useful. > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > --- > include/linux/fs.h | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 21a981680856..4905ce14db0b 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2699,11 +2699,7 @@ int setattr_should_drop_sgid(struct mnt_idmap *idmap, > * This must be used for allocating filesystems specific inodes to set > * up the inode reclaim context correctly. > */ > -static inline void * > -alloc_inode_sb(struct super_block *sb, struct kmem_cache *cache, gfp_t gfp) > -{ > - return kmem_cache_alloc_lru(cache, &sb->s_inode_lru, gfp); > -} > +#define alloc_inode_sb(_sb, _cache, _gfp) kmem_cache_alloc_lru(_cache, &_sb->s_inode_lru, _gfp) Honestly, I don't like this change. In general, pre-processor macros are ugly and error-prone. Besides, it works for you only because __kmem_cache_alloc_lru() is declared __always_inline (unless CONFIG_SLUB_TINY is defined, but then you probably don't want the tracking either). In any case, it's going to be difficult for people to understand why and how this works. If the actual caller of alloc_inode_sb() is needed, I'd rather add it as a parameter and pass down _RET_IP_ explicitly here. Just my two cents, Petr T
On Tue, May 02, 2023 at 02:35:30PM +0200, Petr Tesařík wrote: > On Mon, 1 May 2023 09:54:13 -0700 > Suren Baghdasaryan <surenb@google.com> wrote: > > > From: Kent Overstreet <kent.overstreet@linux.dev> > > > > We're introducing alloc tagging, which tracks memory allocations by > > callsite. Converting alloc_inode_sb() to a macro means allocations will > > be tracked by its caller, which is a bit more useful. > > > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > --- > > include/linux/fs.h | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 21a981680856..4905ce14db0b 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -2699,11 +2699,7 @@ int setattr_should_drop_sgid(struct mnt_idmap *idmap, > > * This must be used for allocating filesystems specific inodes to set > > * up the inode reclaim context correctly. > > */ > > -static inline void * > > -alloc_inode_sb(struct super_block *sb, struct kmem_cache *cache, gfp_t gfp) > > -{ > > - return kmem_cache_alloc_lru(cache, &sb->s_inode_lru, gfp); > > -} > > +#define alloc_inode_sb(_sb, _cache, _gfp) kmem_cache_alloc_lru(_cache, &_sb->s_inode_lru, _gfp) > > Honestly, I don't like this change. In general, pre-processor macros > are ugly and error-prone. It's a one line macro, it's fine. > Besides, it works for you only because __kmem_cache_alloc_lru() is > declared __always_inline (unless CONFIG_SLUB_TINY is defined, but then > you probably don't want the tracking either). In any case, it's going > to be difficult for people to understand why and how this works. I think you must be confused. kmem_cache_alloc_lru() is a macro, and we need that macro to be expanded at the alloc_inode_sb() callsite. It's got nothing to do with whether or not __kmem_cache_alloc_lru() is inline or not. > If the actual caller of alloc_inode_sb() is needed, I'd rather add it > as a parameter and pass down _RET_IP_ explicitly here. That approach was considered, but adding an ip parameter to every memory allocation function would've been far more churn.
On Tue, 2 May 2023 15:57:51 -0400 Kent Overstreet <kent.overstreet@linux.dev> wrote: > On Tue, May 02, 2023 at 02:35:30PM +0200, Petr Tesařík wrote: > > On Mon, 1 May 2023 09:54:13 -0700 > > Suren Baghdasaryan <surenb@google.com> wrote: > > > > > From: Kent Overstreet <kent.overstreet@linux.dev> > > > > > > We're introducing alloc tagging, which tracks memory allocations by > > > callsite. Converting alloc_inode_sb() to a macro means allocations will > > > be tracked by its caller, which is a bit more useful. > > > > > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > > --- > > > include/linux/fs.h | 6 +----- > > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > > index 21a981680856..4905ce14db0b 100644 > > > --- a/include/linux/fs.h > > > +++ b/include/linux/fs.h > > > @@ -2699,11 +2699,7 @@ int setattr_should_drop_sgid(struct mnt_idmap *idmap, > > > * This must be used for allocating filesystems specific inodes to set > > > * up the inode reclaim context correctly. > > > */ > > > -static inline void * > > > -alloc_inode_sb(struct super_block *sb, struct kmem_cache *cache, gfp_t gfp) > > > -{ > > > - return kmem_cache_alloc_lru(cache, &sb->s_inode_lru, gfp); > > > -} > > > +#define alloc_inode_sb(_sb, _cache, _gfp) kmem_cache_alloc_lru(_cache, &_sb->s_inode_lru, _gfp) > > > > Honestly, I don't like this change. In general, pre-processor macros > > are ugly and error-prone. > > It's a one line macro, it's fine. It's not the same. A macro effectively adds a keyword, because it gets expanded regardless of context; for example, you can't declare a local variable called alloc_inode_sb, and the compiler errors may be quite confusing at first. See also the discussion about patch 19/40 in this series. > > Besides, it works for you only because __kmem_cache_alloc_lru() is > > declared __always_inline (unless CONFIG_SLUB_TINY is defined, but then > > you probably don't want the tracking either). In any case, it's going > > to be difficult for people to understand why and how this works. > > I think you must be confused. kmem_cache_alloc_lru() is a macro, and we > need that macro to be expanded at the alloc_inode_sb() callsite. It's > got nothing to do with whether or not __kmem_cache_alloc_lru() is inline > or not. Oh no, I am not confused. Look at the definition of kmem_cache_alloc_lru(): void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru, gfp_t gfpflags) { return __kmem_cache_alloc_lru(s, lru, gfpflags); } See? No _RET_IP_ here. That's because it's here: static __fastpath_inline void *__kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru, gfp_t gfpflags) { void *ret = slab_alloc(s, lru, gfpflags, _RET_IP_, s->object_size); trace_kmem_cache_alloc(_RET_IP_, ret, s, gfpflags, NUMA_NO_NODE); return ret; } Now, if __kmem_cache_alloc_lru() is not inlined, then this _RET_IP_ will be somewhere inside kmem_cache_alloc_lru(), which is not very useful. But what is __fastpath_inline? Well, it depends: #ifndef CONFIG_SLUB_TINY #define __fastpath_inline __always_inline #else #define __fastpath_inline #endif In short, if CONFIG_SLUB_TINY is defined, it's up to the C compiler whether __kmem_cache_alloc_lru() is inlined or not. > > If the actual caller of alloc_inode_sb() is needed, I'd rather add it > > as a parameter and pass down _RET_IP_ explicitly here. > > That approach was considered, but adding an ip parameter to every memory > allocation function would've been far more churn. See my reply to patch 19/40. Rename the original function, but add an __always_inline function with the original signature, and let it take care of _RET_IP_. Petr T
diff --git a/include/linux/fs.h b/include/linux/fs.h index 21a981680856..4905ce14db0b 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2699,11 +2699,7 @@ int setattr_should_drop_sgid(struct mnt_idmap *idmap, * This must be used for allocating filesystems specific inodes to set * up the inode reclaim context correctly. */ -static inline void * -alloc_inode_sb(struct super_block *sb, struct kmem_cache *cache, gfp_t gfp) -{ - return kmem_cache_alloc_lru(cache, &sb->s_inode_lru, gfp); -} +#define alloc_inode_sb(_sb, _cache, _gfp) kmem_cache_alloc_lru(_cache, &_sb->s_inode_lru, _gfp) extern void __insert_inode_hash(struct inode *, unsigned long hashval); static inline void insert_inode_hash(struct inode *inode)