Re: [PATCH 05/14] tracefs: replace call_rcu by kfree_rcu for simple kmem_cache_free callback

Re: [PATCH 05/14] tracefs: replace call_rcu by kfree_rcu for simple kmem_cache_free callback

[Vlastimil Babka]

On 6/10/24 10:36 PM, Steven Rostedt wrote:
> On Mon, 10 Jun 2024 08:46:42 -0700
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
>> > > index 7c29f4afc23d..338c52168e61 100644
>> > > --- a/fs/tracefs/inode.c
>> > > +++ b/fs/tracefs/inode.c
>> > > @@ -53,14 +53,6 @@ static struct inode *tracefs_alloc_inode(struct super_block *sb)
>> > >  	return &ti->vfs_inode;
>> > >  }
>> > >  
>> > > -static void tracefs_free_inode_rcu(struct rcu_head *rcu)
>> > > -{
>> > > -	struct tracefs_inode *ti;
>> > > -
>> > > -	ti = container_of(rcu, struct tracefs_inode, rcu);
>> > > -	kmem_cache_free(tracefs_inode_cachep, ti);  
>> > 
>> > Does this work?
>> > 
>> > tracefs needs to be freed via the tracefs_inode_cachep. Does
>> > kfree_rcu() handle specific frees for objects that were not allocated
>> > via kmalloc()?  
>> 
>> A recent change to kfree() allows it to correctly handle memory allocated
>> via kmem_cache_alloc().  News to me as of a few weeks ago.  ;-)
> 
> If that's the case then:
> 
> Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> 
> Do we have a way to add a "Depends-on" tag so that anyone backporting this
> will know that it requires the change to whatever allowed that to happen?

Looks like people use that tag, although no grep hits in Documentation, so
Cc'ing workflows@ and Thorsten.

In this case it would be

Depends-on: c9929f0e344a ("mm/slob: remove CONFIG_SLOB")

> Or we need to update the change log to explicitly state that this should
> *not* be backported.
> 
> -- Steve

Re: [PATCH 05/14] tracefs: replace call_rcu by kfree_rcu for simple kmem_cache_free callback

[Greg KH]

On Mon, Jun 10, 2024 at 11:40:54PM +0200, Vlastimil Babka wrote:
> On 6/10/24 10:36 PM, Steven Rostedt wrote:
> > On Mon, 10 Jun 2024 08:46:42 -0700
> > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > 
> >> > > index 7c29f4afc23d..338c52168e61 100644
> >> > > --- a/fs/tracefs/inode.c
> >> > > +++ b/fs/tracefs/inode.c
> >> > > @@ -53,14 +53,6 @@ static struct inode *tracefs_alloc_inode(struct super_block *sb)
> >> > >  	return &ti->vfs_inode;
> >> > >  }
> >> > >  
> >> > > -static void tracefs_free_inode_rcu(struct rcu_head *rcu)
> >> > > -{
> >> > > -	struct tracefs_inode *ti;
> >> > > -
> >> > > -	ti = container_of(rcu, struct tracefs_inode, rcu);
> >> > > -	kmem_cache_free(tracefs_inode_cachep, ti);  
> >> > 
> >> > Does this work?
> >> > 
> >> > tracefs needs to be freed via the tracefs_inode_cachep. Does
> >> > kfree_rcu() handle specific frees for objects that were not allocated
> >> > via kmalloc()?  
> >> 
> >> A recent change to kfree() allows it to correctly handle memory allocated
> >> via kmem_cache_alloc().  News to me as of a few weeks ago.  ;-)
> > 
> > If that's the case then:
> > 
> > Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > 
> > Do we have a way to add a "Depends-on" tag so that anyone backporting this
> > will know that it requires the change to whatever allowed that to happen?
> 
> Looks like people use that tag, although no grep hits in Documentation, so
> Cc'ing workflows@ and Thorsten.
> 
> In this case it would be
> 
> Depends-on: c9929f0e344a ("mm/slob: remove CONFIG_SLOB")

Ick, no, use the documented way of handling this as described in the
stable kernel rules file.

thanks,

greg k-h

Re: [PATCH 05/14] tracefs: replace call_rcu by kfree_rcu for simple kmem_cache_free callback

[Vlastimil Babka]

On 6/11/24 8:23 AM, Greg KH wrote:
> On Mon, Jun 10, 2024 at 11:40:54PM +0200, Vlastimil Babka wrote:
>> On 6/10/24 10:36 PM, Steven Rostedt wrote:
>> > On Mon, 10 Jun 2024 08:46:42 -0700
>> > "Paul E. McKenney" <paulmck@kernel.org> wrote:
>> > 
>> >> > > index 7c29f4afc23d..338c52168e61 100644
>> >> > > --- a/fs/tracefs/inode.c
>> >> > > +++ b/fs/tracefs/inode.c
>> >> > > @@ -53,14 +53,6 @@ static struct inode *tracefs_alloc_inode(struct super_block *sb)
>> >> > >  	return &ti->vfs_inode;
>> >> > >  }
>> >> > >  
>> >> > > -static void tracefs_free_inode_rcu(struct rcu_head *rcu)
>> >> > > -{
>> >> > > -	struct tracefs_inode *ti;
>> >> > > -
>> >> > > -	ti = container_of(rcu, struct tracefs_inode, rcu);
>> >> > > -	kmem_cache_free(tracefs_inode_cachep, ti);  
>> >> > 
>> >> > Does this work?
>> >> > 
>> >> > tracefs needs to be freed via the tracefs_inode_cachep. Does
>> >> > kfree_rcu() handle specific frees for objects that were not allocated
>> >> > via kmalloc()?  
>> >> 
>> >> A recent change to kfree() allows it to correctly handle memory allocated
>> >> via kmem_cache_alloc().  News to me as of a few weeks ago.  ;-)
>> > 
>> > If that's the case then:
>> > 
>> > Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
>> > 
>> > Do we have a way to add a "Depends-on" tag so that anyone backporting this
>> > will know that it requires the change to whatever allowed that to happen?
>> 
>> Looks like people use that tag, although no grep hits in Documentation, so
>> Cc'ing workflows@ and Thorsten.
>> 
>> In this case it would be
>> 
>> Depends-on: c9929f0e344a ("mm/slob: remove CONFIG_SLOB")
> 
> Ick, no, use the documented way of handling this as described in the
> stable kernel rules file.

AFAICS that documented way is for a different situation? I assume you mean
this part:

* Specify any additional patch prerequisites for cherry picking::

    Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle

But that would assume we actively want to backport this cleanup patch in the
first place. But as I understand Steven's intention, we want just to make
sure that if in the future this patch is backported (i.e. as a dependency of
something else) it won't be forgotten to also backport c9929f0e344a
("mm/slob: remove CONFIG_SLOB"). How to express that without actively
marking this patch for backport at the same time?

> thanks,
> 
> greg k-h

Re: [PATCH 05/14] tracefs: replace call_rcu by kfree_rcu for simple kmem_cache_free callback

[Thorsten Leemhuis]

On 11.06.24 10:42, Vlastimil Babka wrote:
> On 6/11/24 8:23 AM, Greg KH wrote:
>> On Mon, Jun 10, 2024 at 11:40:54PM +0200, Vlastimil Babka wrote:
>>> On 6/10/24 10:36 PM, Steven Rostedt wrote:
>>>> On Mon, 10 Jun 2024 08:46:42 -0700
>>>> "Paul E. McKenney" <paulmck@kernel.org> wrote:
>>>>
>>>>>>> index 7c29f4afc23d..338c52168e61 100644
>>>>>>> --- a/fs/tracefs/inode.c
>>>>>>> +++ b/fs/tracefs/inode.c
>>>>>>> @@ -53,14 +53,6 @@ static struct inode *tracefs_alloc_inode(struct super_block *sb)
>>>>>>>  	return &ti->vfs_inode;
>>>>>>>  }
>>>>>>>  
>>>>>>> -static void tracefs_free_inode_rcu(struct rcu_head *rcu)
>>>>>>> -{
>>>>>>> -	struct tracefs_inode *ti;
>>>>>>> -
>>>>>>> -	ti = container_of(rcu, struct tracefs_inode, rcu);
>>>>>>> -	kmem_cache_free(tracefs_inode_cachep, ti);  
>>>>>>
>>>>>> Does this work?
>>>>>>
>>>>>> tracefs needs to be freed via the tracefs_inode_cachep. Does
>>>>>> kfree_rcu() handle specific frees for objects that were not allocated
>>>>>> via kmalloc()?  
>>>>>
>>>>> A recent change to kfree() allows it to correctly handle memory allocated
>>>>> via kmem_cache_alloc().  News to me as of a few weeks ago.  ;-)
>>>>
>>>> If that's the case then:
>>>>
>>>> Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
>>>>
>>>> Do we have a way to add a "Depends-on" tag so that anyone backporting this
>>>> will know that it requires the change to whatever allowed that to happen?
>>>
>>> Looks like people use that tag, although no grep hits in Documentation, so
>>> Cc'ing workflows@ and Thorsten.
>>>
>>> In this case it would be
>>>
>>> Depends-on: c9929f0e344a ("mm/slob: remove CONFIG_SLOB")
>>
>> Ick, no, use the documented way of handling this as described in the
>> stable kernel rules file.
> 
> AFAICS that documented way is for a different situation? I assume you mean
> this part:
> 
> * Specify any additional patch prerequisites for cherry picking::
> 
>     Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle
> 
> But that would assume we actively want to backport this cleanup patch in the
> first place. But as I understand Steven's intention, we want just to make
> sure that if in the future this patch is backported (i.e. as a dependency of
> something else) it won't be forgotten to also backport c9929f0e344a
> ("mm/slob: remove CONFIG_SLOB"). How to express that without actively
> marking this patch for backport at the same time?

Hah, waiting a bit spared me the time to write a similar reply. :-D
Writing one now anyway to broaden the scope:

I recently noticed we have the same problem when it comes to the
"delayed backporting" aspect, e.g. this part:

"""
* Delay pick up of patches::

    Cc: <stable@vger.kernel.org> # after -rc3
"""

I'll bring this up in a maintainers summit proposal I'm currently
preparing. But I have no idea how to solve this in an elegant way.
"Cc: <stable+INeitherKnowNorCare@kernel.org> # after -rc3" could work,
but well, as indicated, that's kinda ugly.

Ciao, Thorsten

Re: [PATCH 05/14] tracefs: replace call_rcu by kfree_rcu for simple kmem_cache_free callback

[Steven Rostedt]

On Tue, 11 Jun 2024 08:23:11 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> > Depends-on: c9929f0e344a ("mm/slob: remove CONFIG_SLOB")  
> 
> Ick, no, use the documented way of handling this as described in the
> stable kernel rules file.

You mentioned this before, I guess you mean this:

> To send additional instructions to the stable team, use a shell-style inline
> comment to pass arbitrary or predefined notes:
> 
> * Specify any additional patch prerequisites for cherry picking::
> 
>     Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle
>     Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle
>     Cc: <stable@vger.kernel.org> # 3.3.x: fd21073: sched: Fix affinity logic
>     Cc: <stable@vger.kernel.org> # 3.3.x
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
>   The tag sequence has the meaning of::
> 
>     git cherry-pick a1f84a3
>     git cherry-pick 1b9508f
>     git cherry-pick fd21073
>     git cherry-pick <this commit>
> 
>   Note that for a patch series, you do not have to list as prerequisites the
>   patches present in the series itself. For example, if you have the following
>   patch series::
> 
>     patch1
>     patch2
> 
>   where patch2 depends on patch1, you do not have to list patch1 as
>   prerequisite of patch2 if you have already marked patch1 for stable
>   inclusion.

What's with the "3.3.x"? Isn't that obsolete? And honestly, I find the
above much more "ick" than "Depends-on:". That's because I like to read
human readable tags and not machine processing tags. I'm a human, not a machine.

-- Steve

Re: [PATCH 05/14] tracefs: replace call_rcu by kfree_rcu for simple kmem_cache_free callback

[Steven Rostedt]

On Tue, 11 Jun 2024 10:42:28 +0200
Vlastimil Babka <vbabka@suse.cz> wrote:

> AFAICS that documented way is for a different situation? I assume you mean
> this part:
> 
> * Specify any additional patch prerequisites for cherry picking::
> 
>     Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle
> 
> But that would assume we actively want to backport this cleanup patch in the
> first place. But as I understand Steven's intention, we want just to make
> sure that if in the future this patch is backported (i.e. as a dependency of
> something else) it won't be forgotten to also backport c9929f0e344a
> ("mm/slob: remove CONFIG_SLOB"). How to express that without actively
> marking this patch for backport at the same time?

Exactly! This isn't to be tagged as stable. It's just a way to say "if you
need this patch for any reason, you also need patch X".

I think "Depends-on" is the way to go, as it is *not* a stable thing, and
what is in stable rules is only about stable patches.

-- Steve

Re: [PATCH 05/14] tracefs: replace call_rcu by kfree_rcu for simple kmem_cache_free callback

[Jason A. Donenfeld]

On Tue, Jun 11, 2024 at 10:14:58AM -0400, Steven Rostedt wrote:
> On Tue, 11 Jun 2024 10:42:28 +0200
> Vlastimil Babka <vbabka@suse.cz> wrote:
> 
> > AFAICS that documented way is for a different situation? I assume you mean
> > this part:
> > 
> > * Specify any additional patch prerequisites for cherry picking::
> > 
> >     Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle
> > 
> > But that would assume we actively want to backport this cleanup patch in the
> > first place. But as I understand Steven's intention, we want just to make
> > sure that if in the future this patch is backported (i.e. as a dependency of
> > something else) it won't be forgotten to also backport c9929f0e344a
> > ("mm/slob: remove CONFIG_SLOB"). How to express that without actively
> > marking this patch for backport at the same time?
> 
> Exactly! This isn't to be tagged as stable. It's just a way to say "if you
> need this patch for any reason, you also need patch X".
> 
> I think "Depends-on" is the way to go, as it is *not* a stable thing, and
> what is in stable rules is only about stable patches.

How does "Depends-on" not spiral out of control? There's a *lot* of
"Depends-on" relations one could express in commit series and such. Of
course a lot of git itself is designed to show some subset of these
relationships.

It seems like in most cases, the "Cc: stable@v.g.o # x.y.z+" notation
expresses the backporting safety correctly. What is the purpose of
saying, "if you need this patch for any reason, you also need patch X"?
Who is the intended audience, and are you sure they need this?

I ask these questions because I wind up doing a lot of work backporting
patches to stable and marking things properly for that or submitting
manually backported stable patches and so forth, and in general, patch
applicability for stable things is something I wind up devoting a lot of
time to. If I have to *additionally* start caring about the theoretical
possibility that somebody in the future, outside of the stable flow,
might not understand the context of a given patch and blindly apply it
to some random tree here or there, that sounds like a lot of extra brain
cycles to consider.

So, is this actually necessary, and how does it not spiral out of
control?

Re: [PATCH 05/14] tracefs: replace call_rcu by kfree_rcu for simple kmem_cache_free callback

[Steven Rostedt]

On Wed, 12 Jun 2024 16:09:40 +0200
"Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
> > 
> > I think "Depends-on" is the way to go, as it is *not* a stable thing, and
> > what is in stable rules is only about stable patches.  
> 
> How does "Depends-on" not spiral out of control? There's a *lot* of
> "Depends-on" relations one could express in commit series and such. Of
> course a lot of git itself is designed to show some subset of these
> relationships.

If a change occurs because a recent change happened that allows the
current change to work, then I think a Depends-on is appropriate.

Like in this example. I thought this change was broken, and it would
have been except for a recent change. Having the dependency listed is
useful, especially if the dependency is subtle (doesn't break the build
and may not show the bug immediately).

> 
> It seems like in most cases, the "Cc: stable@v.g.o # x.y.z+" notation
> expresses the backporting safety correctly. What is the purpose of
> saying, "if you need this patch for any reason, you also need patch X"?
> Who is the intended audience, and are you sure they need this?

The intended audience is someone backporting features and not fixes.

> 
> I ask these questions because I wind up doing a lot of work backporting
> patches to stable and marking things properly for that or submitting
> manually backported stable patches and so forth, and in general, patch
> applicability for stable things is something I wind up devoting a lot of
> time to. If I have to *additionally* start caring about the theoretical
> possibility that somebody in the future, outside of the stable flow,
> might not understand the context of a given patch and blindly apply it
> to some random tree here or there, that sounds like a lot of extra brain
> cycles to consider.
> 
> So, is this actually necessary, and how does it not spiral out of
> control?

How would you see it going out of control? And "Depends-on" would only
be used for non stable relationships. If stable backports, we can keep
with the current method.

-- Steve