Since SLOB was removed, it is not necessary to use call_rcu
when the callback only performs kmem_cache_free. Use
kfree_rcu() directly.
The changes were done using the following Coccinelle semantic patch.
This semantic patch is designed to ignore cases where the callback
function is used in another way.
// <smpl>
@r@
expression e;
local idexpression e2;
identifier cb,f;
position p;
@@
(
call_rcu(...,e2)
|
call_rcu(&e->f,cb@p)
)
@r1@
type T;
identifier x,r.cb;
@@
cb(...) {
(
kmem_cache_free(...);
|
T x = ...;
kmem_cache_free(...,x);
|
T x;
x = ...;
kmem_cache_free(...,x);
)
}
@s depends on r1@
position p != r.p;
identifier r.cb;
@@
cb@p
@script:ocaml@
cb << r.cb;
p << s.p;
@@
Printf.eprintf "Other use of %s at %s:%d\n"
cb (List.hd p).file (List.hd p).line
@depends on r1 && !s@
expression e;
identifier r.cb,f;
position r.p;
@@
- call_rcu(&e->f,cb@p)
+ kfree_rcu(e,f)
@r1a depends on !s@
type T;
identifier x,r.cb;
@@
- cb(...) {
(
- kmem_cache_free(...);
|
- T x = ...;
- kmem_cache_free(...,x);
|
- T x;
- x = ...;
- kmem_cache_free(...,x);
)
- }
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
Reviewed-by: Paul E. McKenney <[email protected]>
Reviewed-by: Vlastimil Babka <[email protected]>
---
fs/tracefs/inode.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
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);
-}
-
static void tracefs_free_inode(struct inode *inode)
{
struct tracefs_inode *ti = get_tracefs(inode);
@@ -70,7 +62,7 @@ static void tracefs_free_inode(struct inode *inode)
list_del_rcu(&ti->list);
spin_unlock_irqrestore(&tracefs_inode_lock, flags);
- call_rcu(&ti->rcu, tracefs_free_inode_rcu);
+ kfree_rcu(ti, rcu);
}
static ssize_t default_read_file(struct file *file, char __user *buf,
On Mon, Jun 10, 2024 at 11:22:23AM -0400, Steven Rostedt wrote:
> On Sun, 9 Jun 2024 10:27:17 +0200
> Julia Lawall <[email protected]> wrote:
>
> > diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> > 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. ;-)
Thanx, Paul
> -- Steve
>
>
> > -}
> > -
> > static void tracefs_free_inode(struct inode *inode)
> > {
> > struct tracefs_inode *ti = get_tracefs(inode);
> > @@ -70,7 +62,7 @@ static void tracefs_free_inode(struct inode *inode)
> > list_del_rcu(&ti->list);
> > spin_unlock_irqrestore(&tracefs_inode_lock, flags);
> >
> > - call_rcu(&ti->rcu, tracefs_free_inode_rcu);
> > + kfree_rcu(ti, rcu);
> > }
> >
> > static ssize_t default_read_file(struct file *file, char __user *buf,
>
On Sun, 9 Jun 2024 10:27:17 +0200
Julia Lawall <[email protected]> wrote:
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> 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()?
-- Steve
> -}
> -
> static void tracefs_free_inode(struct inode *inode)
> {
> struct tracefs_inode *ti = get_tracefs(inode);
> @@ -70,7 +62,7 @@ static void tracefs_free_inode(struct inode *inode)
> list_del_rcu(&ti->list);
> spin_unlock_irqrestore(&tracefs_inode_lock, flags);
>
> - call_rcu(&ti->rcu, tracefs_free_inode_rcu);
> + kfree_rcu(ti, rcu);
> }
>
> static ssize_t default_read_file(struct file *file, char __user *buf,
On Mon, 10 Jun 2024 08:46:42 -0700
"Paul E. McKenney" <[email protected]> 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) <[email protected]>
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?
Or we need to update the change log to explicitly state that this should
*not* be backported.
-- Steve
On 6/10/24 5:46 PM, Paul E. McKenney wrote:
> On Mon, Jun 10, 2024 at 11:22:23AM -0400, Steven Rostedt wrote:
>> On Sun, 9 Jun 2024 10:27:17 +0200
>> Julia Lawall <[email protected]> wrote:
>>
>> > diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
>> > 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. ;-)
Hey, I did try not to keep that a secret :)
https://lore.kernel.org/all/[email protected]/
> Thanx, Paul
>
>> -- Steve
>>
>>
>> > -}
>> > -
>> > static void tracefs_free_inode(struct inode *inode)
>> > {
>> > struct tracefs_inode *ti = get_tracefs(inode);
>> > @@ -70,7 +62,7 @@ static void tracefs_free_inode(struct inode *inode)
>> > list_del_rcu(&ti->list);
>> > spin_unlock_irqrestore(&tracefs_inode_lock, flags);
>> >
>> > - call_rcu(&ti->rcu, tracefs_free_inode_rcu);
>> > + kfree_rcu(ti, rcu);
>> > }
>> >
>> > static ssize_t default_read_file(struct file *file, char __user *buf,
>>
On Mon, 10 Jun 2024 22:42:30 +0200
Vlastimil Babka <[email protected]> wrote:
> On 6/10/24 5:46 PM, Paul E. McKenney wrote:
> > On Mon, Jun 10, 2024 at 11:22:23AM -0400, Steven Rostedt wrote:
> >> On Sun, 9 Jun 2024 10:27:17 +0200
> >> Julia Lawall <[email protected]> wrote:
> >>
> >> > diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> >> > 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. ;-)
>
> Hey, I did try not to keep that a secret :)
> https://lore.kernel.org/all/[email protected]/
>
Heh, I didn't look at that patch very deeply.
-- Steve
On 6/10/24 10:36 PM, Steven Rostedt wrote:
> On Mon, 10 Jun 2024 08:46:42 -0700
> "Paul E. McKenney" <[email protected]> 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) <[email protected]>
>
> 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
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" <[email protected]> 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) <[email protected]>
> >
> > 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
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" <[email protected]> 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) <[email protected]>
>>>>
>>>> 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: <[email protected]> # 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: <[email protected]> # 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: <[email protected]> # after -rc3" could work,
but well, as indicated, that's kinda ugly.
Ciao, Thorsten
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" <[email protected]> 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) <[email protected]>
>> >
>> > 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: <[email protected]> # 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
On Tue, 11 Jun 2024 08:23:11 +0200
Greg KH <[email protected]> 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: <[email protected]> # 3.3.x: a1f84a3: sched: Check for idle
> Cc: <[email protected]> # 3.3.x: 1b9508f: sched: Rate-limit newidle
> Cc: <[email protected]> # 3.3.x: fd21073: sched: Fix affinity logic
> Cc: <[email protected]> # 3.3.x
> Signed-off-by: Ingo Molnar <[email protected]>
>
> 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
On Tue, 11 Jun 2024 10:42:28 +0200
Vlastimil Babka <[email protected]> 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: <[email protected]> # 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
On Tue, Jun 11, 2024 at 10:14:58AM -0400, Steven Rostedt wrote:
> On Tue, 11 Jun 2024 10:42:28 +0200
> Vlastimil Babka <[email protected]> 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: <[email protected]> # 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: [email protected] # 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?
On Wed, 12 Jun 2024 16:09:40 +0200
"Jason A. Donenfeld" <[email protected]> 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: [email protected] # 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