Re: [PATCH 05/14] pidfs: adapt to rhashtable-based simple_xattrs
From: Jan Kara <jack@suse.cz>
Date: 2026-02-27 15:09:21
Also in:
linux-fsdevel, linux-mm, lkml
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
quoted hunk ↗ jump to hunk
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 [off-list ref] SUSE Labs, CR