Re: [PATCH v5 5/6] kernfs: Use RCU to access kernfs_node::parent.

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2025-01-29 06:54:33 [-1000], Tejun Heo wrote:
> Hello,
> 
> On Wed, Jan 29, 2025 at 02:23:11PM +0100, Sebastian Andrzej Siewior wrote:
> > > > @@ -64,9 +64,9 @@ static size_t kernfs_depth(struct kernfs_node *from, struct kernfs_node *to)
> > > >  {
> > > >  	size_t depth = 0;
> > > >  
> > > > -	while (to->parent && to != from) {
> > > > +	while (rcu_dereference(to->__parent) && to != from) {
> > > 
> > > Why not use kernfs_parent() here and other places?
> > 
> > Because it is from within RCU section and the other checks are not
> > required. If you prefer this instead, I sure can update it.
> 
> Hmm... I would have gone with using the same accessor everywhere but am not
> sure how strongly I feel about it. I don't think it's useful to worry about
> the overhead of the extra lockdep annotations in debug builds. Ignoring that
> and just considering code readability, what would you do?

It is your call. I would prefer to open code that part that we do only
rely on RCU here but sure understand that you don't care about the
details and want to have only one accessor.

> > > > @@ -226,6 +227,7 @@ int kernfs_path_from_node(struct kernfs_node *to, struct kernfs_node *from,
> > > >  	unsigned long flags;
> > > >  	int ret;
> > > >  
> > > > +	guard(rcu)();
> > > 
> > > Doesn't irqsave imply rcu?
…
> > Also, rcu_dereference() will complain about missing RCU annotation. On
> > PREEMPT_RT rcu_dereference_sched() will complain because irqsave (in
> > this case) will not disable interrupts.
> 
> You know this a lot better than I do. If it's necessary for RT builds, it's
> not redundant.

Good.

> Thanks.

Sebastian