Re: [PATCH v2 2/2] cgroup, kernfs: Move cgroup to the RCU interface for name lookups

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

On 2024-11-13 08:43:32 [+0100], To Tejun Heo wrote:
> On 2024-11-12 08:59:16 [-1000], Tejun Heo wrote:
> > Hello,
> 
> Hi,
> 
> > On Tue, Nov 12, 2024 at 04:52:39PM +0100, Sebastian Andrzej Siewior wrote:
> > ...
> > >  /**
> > > - * pr_cont_kernfs_name - pr_cont name of a kernfs_node
> > > + * pr_cont_kernfs_name_rcu - pr_cont name of a kernfs_node
> > >   * @kn: kernfs_node of interest
> > >   *
> > > - * This function can be called from any context.
> > > + * This function can be called from any context. The root node must be with
> > > + * KERNFS_ROOT_SAME_PARENT.
> > >   */
> > > -void pr_cont_kernfs_name(struct kernfs_node *kn)
> > > +void pr_cont_kernfs_name_rcu(struct kernfs_node *kn)
> > 
> > Having to split the interface all the way up isn't great. While there are
> > also downsides, I wonder whether a better approach here is just making the
> > backend function (kernfs_path_from_node()) automatically use RCU locking if
> > the flag is set rather than propagating the difference by splitting the
> > interface. The distinction doesn't mean anything to most users after all.
> 
> Indeed.

Now I see what the problems are. If we merge both into one, then I get
this:
| int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
| {
|         struct kernfs_root *root;
|         bool rcu_lookup;
|
|         if (!kn)
|                 return strscpy(buf, "(null)", buflen);
|
|         root = kernfs_root(kn);

This is the tricky part. For KERNFS_ROOT_INVARIANT_PARENT I don't worry
that the parent goes away and I need it to get a reference to the
kernfs_root node. For the !KERNFS_ROOT_INVARIANT_PARENT I need the lock
for kernfs_root() so I put the guard/ lock at the top.

I think that is why you suggested the two functions (or this is what I
understood). Looking at the remaining bits:

|         rcu_lookup = root->flags & KERNFS_ROOT_INVARIANT_PARENT;
|         if (rcu_lookup) {
|                 guard(rcu)();
|                 return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
|         }
|         guard(read_lock_irqsave)(&kernfs_rename_lock);
|         return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
| }

This could collapse into the RCU version because read_lock_irqsave()
implies RCU protection. And since ->name is always RCU assigned/
deallocated I don't really need the lock here, RCU would be enough.
Except for the parent. The kn->parent does not matter here (it should be
always be != NULL if assigned), the problematic part is kernfs_root()
which checks the parent for the root node.

To make this simple I could avoid kernfs_root lookup and just have:
| int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
| {       
|         if (!kn)
|                 return strscpy(buf, "(null)", buflen);
|         
|         guard(rcu)();
|         return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
| }                             

That is the easy part. kernfs_path_from_node() is different as it
requires the parent pointer. In order to distinguish the RCU from the
non-RCU version I need kernfs_root for the flag and depending on it, the
lock so the parent does not go away.

Would it work to add the pointer to kernfs_root into kernfs_node? This
would shrink kernfs_elem_dir by a pointer but the union would remain the
same size due to kernfs_elem_attr so the struct would grow.

> > Thanks.

Sebastian