Re: [PATCH 05/14] pidfs: adapt to rhashtable-based simple_xattrs

[Jan Kara <jack@suse.cz>]

On Fri 27-02-26 16:09:15, Jan Kara wrote:
> On Mon 16-02-26 14:32:01, Christian Brauner wrote:
> > Adapt pidfs to use the rhashtable-based xattr path by switching from a
> > dedicated slab cache to simple_xattrs_alloc().
> > 
> > Previously pidfs used a custom kmem_cache (pidfs_xattr_cachep) that
> > allocated a struct containing an embedded simple_xattrs plus
> > simple_xattrs_init(). Replace this with simple_xattrs_alloc() which
> > combines kzalloc + rhashtable_init, and drop the dedicated slab cache
> > entirely.
> > 
> > Use simple_xattr_free_rcu() for replaced xattr entries to allow
> > concurrent RCU readers to finish.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
> One question below:
> 
> > +static LLIST_HEAD(pidfs_free_list);
> > +
> > +static void pidfs_free_attr_work(struct work_struct *work)
> > +{
> > +	struct pidfs_attr *attr, *next;
> > +	struct llist_node *head;
> > +
> > +	head = llist_del_all(&pidfs_free_list);
> > +	llist_for_each_entry_safe(attr, next, head, pidfs_llist) {
> > +		struct simple_xattrs *xattrs = attr->xattrs;
> > +
> > +		if (xattrs) {
> > +			simple_xattrs_free(xattrs, NULL);
> > +			kfree(xattrs);
> > +		}
> > +		kfree(attr);
> > +	}
> > +}
> > +
> > +static DECLARE_WORK(pidfs_free_work, pidfs_free_attr_work);
> > +
> 
> So you bother with postponing the freeing to a scheduled work because
> put_pid() can be called from a context where acquiring rcu to iterate
> rhashtable would not be possible? Frankly I have hard time imagining such
> context (where previous rbtree code wouldn't have issues as well), in
> particular because AFAIR rcu is safe to arbitrarily nest. What am I
> missing?

Ah, I've now found out rhashtable_free_and_destroy() can sleep and that's
likely the reason. OK. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR