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

[Sebastian Andrzej Siewior]

- Zefan Li

On 2024-11-13 13:07:08 [+0100], To Tejun Heo wrote:
> 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.

The kernfs_node is released via RCU. That means if the RCU read section
starts before kernfs_root() then we should always get a stable pointer,
pointing to the same kernfs_root node since it is always the same one.
Even if the `parent' pointer is replaced. Wouldn't we need __rcu
annotation then for the `parent' pointer then?

> > > Thanks.
> 
Sebastian