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

[Jan Kara <jack@suse.cz>]

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?

								Honza


>  void pidfs_free_pid(struct pid *pid)
>  {
> -	struct pidfs_attr *attr __free(kfree) = no_free_ptr(pid->attr);
> -	struct simple_xattrs *xattrs __free(kfree) = NULL;
> +	struct pidfs_attr *attr = pid->attr;
>  
>  	/*
>  	 * Any dentry must've been wiped from the pid by now.
> @@ -169,9 +196,10 @@ void pidfs_free_pid(struct pid *pid)
>  	if (IS_ERR(attr))
>  		return;
>  
> -	xattrs = no_free_ptr(attr->xattrs);
> -	if (xattrs)
> -		simple_xattrs_free(xattrs, NULL);
> +	if (likely(!attr->xattrs))
> +		kfree(attr);
> +	else if (llist_add(&attr->pidfs_llist, &pidfs_free_list))
> +		schedule_work(&pidfs_free_work);
>  }
>  
>  #ifdef CONFIG_PROC_FS
> @@ -998,7 +1026,7 @@ static int pidfs_xattr_get(const struct xattr_handler *handler,
>  
>  	xattrs = READ_ONCE(attr->xattrs);
>  	if (!xattrs)
> -		return 0;
> +		return -ENODATA;
>  
>  	name = xattr_full_name(handler, suffix);
>  	return simple_xattr_get(xattrs, name, value, size);
> @@ -1018,22 +1046,16 @@ static int pidfs_xattr_set(const struct xattr_handler *handler,
>  	/* Ensure we're the only one to set @attr->xattrs. */
>  	WARN_ON_ONCE(!inode_is_locked(inode));
>  
> -	xattrs = READ_ONCE(attr->xattrs);
> -	if (!xattrs) {
> -		xattrs = kmem_cache_zalloc(pidfs_xattr_cachep, GFP_KERNEL);
> -		if (!xattrs)
> -			return -ENOMEM;
> -
> -		simple_xattrs_init(xattrs);
> -		smp_store_release(&pid->attr->xattrs, xattrs);
> -	}
> +	xattrs = simple_xattrs_lazy_alloc(&attr->xattrs, value, flags);
> +	if (IS_ERR_OR_NULL(xattrs))
> +		return PTR_ERR(xattrs);
>  
>  	name = xattr_full_name(handler, suffix);
>  	old_xattr = simple_xattr_set(xattrs, name, value, size, flags);
>  	if (IS_ERR(old_xattr))
>  		return PTR_ERR(old_xattr);
>  
> -	simple_xattr_free(old_xattr);
> +	simple_xattr_free_rcu(old_xattr);
>  	return 0;
>  }
>  
> @@ -1108,11 +1130,6 @@ void __init pidfs_init(void)
>  					 (SLAB_HWCACHE_ALIGN | SLAB_RECLAIM_ACCOUNT |
>  					  SLAB_ACCOUNT | SLAB_PANIC), NULL);
>  
> -	pidfs_xattr_cachep = kmem_cache_create("pidfs_xattr_cache",
> -					       sizeof(struct simple_xattrs), 0,
> -					       (SLAB_HWCACHE_ALIGN | SLAB_RECLAIM_ACCOUNT |
> -						SLAB_ACCOUNT | SLAB_PANIC), NULL);
> -
>  	pidfs_mnt = kern_mount(&pidfs_type);
>  	if (IS_ERR(pidfs_mnt))
>  		panic("Failed to mount pidfs pseudo filesystem");
> 
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR