[PATCH v3 0/4] kernfs: make ->attr.open RCU protected.

[PATCH v3 0/4] kernfs: make ->attr.open RCU protected.

[Imran Khan]

The patches in this version of the patch set are as follows:

PATCH-1: Make kernfs_open_node->attr.open RCU protected.

PATCH-2: Change kernfs_notify_list to llist.

PATCH-3: Introduce interface to access kernfs_open_file_mutex.

PATCH-4: Replace global kernfs_open_file_mutex with hashed mutexes.

Changes since v2:
 - Rebase on tag next-20220510
 - Remove PATCH-1 of v2 because it is present in tag next-20220510
 - Include Acked-by tags from Tejun for the acked patches (PATCH-2 and PATCH-3)


Cover letter for v2:
--------------------------------------------------------------------------

I have not yet received any feedback about v1 of this patchset [2] but
in the meantime an old version of first patch from [3] has been integrated in
linux-next. Functionally first patch in both [2] and [3] are identical.
It's just that [2] has renamed one of the functions to better reflect the fact
that we are no longer using reference counting for kernfs_open_node.

In this version, I have just modified first patch of v1 so that we use the
modified function name as done in [2] and avoid those parts that are already
present in linux-next now. The remaining 4 patches (PATCH-2 to PATCH-5) are
identical in both v1 and v2 albeit v2 has been rebased on tag next-20220503.

Changes since v1:
 - Rebase on tag next-20220503

[2]: https://lore.kernel.org/lkml/20220428055431.3826852-1-imran.f.khan@oracle.com/
[3]: https://lore.kernel.org/lkml/20220324103040.584491-1-imran.f.khan@oracle.com/

Original cover letter
-------------------------------------------------------

This patchset contains subset of patches (after addressing review comments)
discussed at [1]. Since [1] is replacing multiple global locks and since
each of these locks can be removed independently, it was decided that we
should make these changes in parts i.e first get one set of optimizations
integrated and then work on top of those further.

The patches in this change set introduce following changes:

PATCH-1: Remove reference counting for kernfs_open_node.

PATCH-2: Make kernfs_open_node->attr.open RCU protected.

PATCH-3: Change kernfs_notify_list to llist.

PATCH-4: Introduce interface to access kernfs_open_file_mutex.

PATCH-5: Replace global kernfs_open_file_mutex with hashed mutexes.

[1] https://lore.kernel.org/lkml/YmLfxHcekrr89IFl@slm.duckdns.org/

----------------------------------------------------------------

Imran Khan (4):
  kernfs: make ->attr.open RCU protected.
  kernfs: Change kernfs_notify_list to llist.
  kernfs: Introduce interface to access global kernfs_open_file_mutex.
  kernfs: Replace global kernfs_open_file_mutex with hashed mutexes.

 fs/kernfs/file.c            | 237 ++++++++++++++++++++++--------------
 fs/kernfs/kernfs-internal.h |   4 +
 fs/kernfs/mount.c           |  19 +++
 include/linux/kernfs.h      |  61 +++++++++-
 4 files changed, 226 insertions(+), 95 deletions(-)


base-commit: 3bf222d317a20170ee17f082626c1e0f83537e13
-- 
2.30.2

[PATCH v3 1/4] kernfs: make ->attr.open RCU protected.

[Imran Khan]

After removal of kernfs_open_node->refcnt in the previous patch,
kernfs_open_node_lock can be removed as well by making ->attr.open
RCU protected. kernfs_put_open_node can delegate freeing to ->attr.open
to RCU and other readers of ->attr.open can do so under rcu_read_(un)lock.

Suggested by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Imran Khan <imran.f.khan@oracle.com>
---
 fs/kernfs/file.c       | 141 +++++++++++++++++++++++++++--------------
 include/linux/kernfs.h |   2 +-
 2 files changed, 96 insertions(+), 47 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index e3abfa843879..796f27333846 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -23,16 +23,16 @@
  * for each kernfs_node with one or more open files.
  *
  * kernfs_node->attr.open points to kernfs_open_node.  attr.open is
- * protected by kernfs_open_node_lock.
+ * RCU protected.
  *
  * filp->private_data points to seq_file whose ->private points to
  * kernfs_open_file.  kernfs_open_files are chained at
  * kernfs_open_node->files, which is protected by kernfs_open_file_mutex.
  */
-static DEFINE_SPINLOCK(kernfs_open_node_lock);
 static DEFINE_MUTEX(kernfs_open_file_mutex);
 
 struct kernfs_open_node {
+	struct rcu_head		rcu_head;
 	atomic_t		event;
 	wait_queue_head_t	poll;
 	struct list_head	files; /* goes through kernfs_open_file.list */
@@ -51,6 +51,50 @@ struct kernfs_open_node {
 static DEFINE_SPINLOCK(kernfs_notify_lock);
 static struct kernfs_node *kernfs_notify_list = KERNFS_NOTIFY_EOL;
 
+/*
+ * Raw deref RCU protected kn->attr.open.
+ * If both @of->list and @kn->attr.open->files are non empty, we can safely
+ * assume that @of is on @kn->attr.open and hence @kn->attr.open will not vanish
+ * and raw derefeencing is safe here.
+ */
+static struct kernfs_open_node *
+kernfs_deref_on_raw(struct kernfs_open_file *of, struct kernfs_node *kn)
+{
+	struct kernfs_open_node *on;
+
+	if (list_empty(&of->list))
+		return NULL;
+
+	on = rcu_dereference_raw(kn->attr.open);
+
+	if (list_empty(&on->files))
+		return NULL;
+	else
+		return on;
+}
+
+/*
+ * Deref ->attr.open corresponding to @kn while holding kernfs_open_file_mutex.
+ * ->attr.open is modified under kernfs_open_file_mutex. So it can be safely
+ * accessed outside RCU read-side critical section, while holding the mutex.
+ */
+static struct kernfs_open_node *kernfs_deref_on_protected(struct kernfs_node *kn)
+{
+	return rcu_dereference_protected(kn->attr.open,
+					 lockdep_is_held(&kernfs_open_file_mutex));
+}
+
+/*
+ * Check ->attr.open corresponding to @kn while holding kernfs_open_file_mutex.
+ * ->attr.open is modified under kernfs_open_file_mutex. So it can be safely
+ * accessed outside RCU read-side critical section, while holding the mutex.
+ */
+static struct kernfs_open_node *kernfs_check_on_protected(struct kernfs_node *kn)
+{
+	return rcu_dereference_check(kn->attr.open,
+				      lockdep_is_held(&kernfs_open_file_mutex));
+}
+
 static struct kernfs_open_file *kernfs_of(struct file *file)
 {
 	return ((struct seq_file *)file->private_data)->private;
@@ -156,8 +200,12 @@ static void kernfs_seq_stop(struct seq_file *sf, void *v)
 static int kernfs_seq_show(struct seq_file *sf, void *v)
 {
 	struct kernfs_open_file *of = sf->private;
+	struct kernfs_open_node *on = kernfs_deref_on_raw(of, of->kn);
+
+	if (!on)
+		return -EINVAL;
 
-	of->event = atomic_read(&of->kn->attr.open->event);
+	of->event = atomic_read(&unrcu_pointer(on)->event);
 
 	return of->kn->attr.ops->seq_show(sf, v);
 }
@@ -180,6 +228,7 @@ static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 	struct kernfs_open_file *of = kernfs_of(iocb->ki_filp);
 	ssize_t len = min_t(size_t, iov_iter_count(iter), PAGE_SIZE);
 	const struct kernfs_ops *ops;
+	struct kernfs_open_node *on;
 	char *buf;
 
 	buf = of->prealloc_buf;
@@ -201,7 +250,11 @@ static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		goto out_free;
 	}
 
-	of->event = atomic_read(&of->kn->attr.open->event);
+	on = kernfs_deref_on_raw(of, of->kn);
+	if (!on)
+		return -EINVAL;
+
+	of->event = atomic_read(&unrcu_pointer(on)->event);
 	ops = kernfs_ops(of->kn);
 	if (ops->read)
 		len = ops->read(of, buf, len, iocb->ki_pos);
@@ -519,36 +572,29 @@ static int kernfs_get_open_node(struct kernfs_node *kn,
 {
 	struct kernfs_open_node *on, *new_on = NULL;
 
- retry:
 	mutex_lock(&kernfs_open_file_mutex);
-	spin_lock_irq(&kernfs_open_node_lock);
-
-	if (!kn->attr.open && new_on) {
-		kn->attr.open = new_on;
-		new_on = NULL;
-	}
-
-	on = kn->attr.open;
-	if (on)
-		list_add_tail(&of->list, &on->files);
-
-	spin_unlock_irq(&kernfs_open_node_lock);
-	mutex_unlock(&kernfs_open_file_mutex);
+	on = kernfs_deref_on_protected(kn);
 
 	if (on) {
-		kfree(new_on);
+		list_add_tail(&of->list, &on->files);
+		mutex_unlock(&kernfs_open_file_mutex);
 		return 0;
+	} else {
+		/* not there, initialize a new one */
+		new_on = kmalloc(sizeof(*new_on), GFP_KERNEL);
+		if (!new_on) {
+			mutex_unlock(&kernfs_open_file_mutex);
+			return -ENOMEM;
+		}
+		atomic_set(&new_on->event, 1);
+		init_waitqueue_head(&new_on->poll);
+		INIT_LIST_HEAD(&new_on->files);
+		list_add_tail(&of->list, &new_on->files);
+		rcu_assign_pointer(kn->attr.open, new_on);
 	}
+	mutex_unlock(&kernfs_open_file_mutex);
 
-	/* not there, initialize a new one and retry */
-	new_on = kmalloc(sizeof(*new_on), GFP_KERNEL);
-	if (!new_on)
-		return -ENOMEM;
-
-	atomic_set(&new_on->event, 1);
-	init_waitqueue_head(&new_on->poll);
-	INIT_LIST_HEAD(&new_on->files);
-	goto retry;
+	return 0;
 }
 
 /**
@@ -567,24 +613,25 @@ static int kernfs_get_open_node(struct kernfs_node *kn,
 static void kernfs_unlink_open_file(struct kernfs_node *kn,
 				 struct kernfs_open_file *of)
 {
-	struct kernfs_open_node *on = kn->attr.open;
-	unsigned long flags;
+	struct kernfs_open_node *on;
 
 	mutex_lock(&kernfs_open_file_mutex);
-	spin_lock_irqsave(&kernfs_open_node_lock, flags);
+
+	on = kernfs_deref_on_protected(kn);
+	if (!on) {
+		mutex_unlock(&kernfs_open_file_mutex);
+		return;
+	}
 
 	if (of)
 		list_del(&of->list);
 
-	if (list_empty(&on->files))
-		kn->attr.open = NULL;
-	else
-		on = NULL;
+	if (list_empty(&on->files)) {
+		rcu_assign_pointer(kn->attr.open, NULL);
+		kfree_rcu(on, rcu_head);
+	}
 
-	spin_unlock_irqrestore(&kernfs_open_node_lock, flags);
 	mutex_unlock(&kernfs_open_file_mutex);
-
-	kfree(on);
 }
 
 static int kernfs_fop_open(struct inode *inode, struct file *file)
@@ -774,17 +821,16 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
 	 * check under kernfs_open_file_mutex will ensure bailing out if
 	 * ->attr.open became NULL while waiting for the mutex.
 	 */
-	if (!kn->attr.open)
+	if (!rcu_access_pointer(kn->attr.open))
 		return;
 
 	mutex_lock(&kernfs_open_file_mutex);
-	if (!kn->attr.open) {
+	on = kernfs_check_on_protected(kn);
+	if (!on) {
 		mutex_unlock(&kernfs_open_file_mutex);
 		return;
 	}
 
-	on = kn->attr.open;
-
 	list_for_each_entry(of, &on->files, list) {
 		struct inode *inode = file_inode(of->file);
 
@@ -815,7 +861,10 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
 __poll_t kernfs_generic_poll(struct kernfs_open_file *of, poll_table *wait)
 {
 	struct kernfs_node *kn = kernfs_dentry_node(of->file->f_path.dentry);
-	struct kernfs_open_node *on = kn->attr.open;
+	struct kernfs_open_node *on = kernfs_deref_on_raw(of, kn);
+
+	if (!on)
+		return EPOLLERR;
 
 	poll_wait(of->file, &on->poll, wait);
 
@@ -922,13 +971,13 @@ void kernfs_notify(struct kernfs_node *kn)
 		return;
 
 	/* kick poll immediately */
-	spin_lock_irqsave(&kernfs_open_node_lock, flags);
-	on = kn->attr.open;
+	rcu_read_lock();
+	on = rcu_dereference(kn->attr.open);
 	if (on) {
 		atomic_inc(&on->event);
 		wake_up_interruptible(&on->poll);
 	}
-	spin_unlock_irqrestore(&kernfs_open_node_lock, flags);
+	rcu_read_unlock();
 
 	/* schedule work to kick fsnotify */
 	spin_lock_irqsave(&kernfs_notify_lock, flags);
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index e2ae15a6225e..13f54f078a52 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -114,7 +114,7 @@ struct kernfs_elem_symlink {
 
 struct kernfs_elem_attr {
 	const struct kernfs_ops	*ops;
-	struct kernfs_open_node	*open;
+	struct kernfs_open_node __rcu	*open;
 	loff_t			size;
 	struct kernfs_node	*notify_next;	/* for kernfs_notify() */
 };
-- 
2.30.2

[PATCH v3 2/4] kernfs: Change kernfs_notify_list to llist.

[Imran Khan]

At present kernfs_notify_list is implemented as a singly linked
list of kernfs_node(s), where last element points to itself and
value of ->attr.next tells if node is present on the list or not.
Both addition and deletion to list happen under kernfs_notify_lock.

Change kernfs_notify_list to llist so that addition to list can heppen
locklessly. We still need kernfs_notify_lock for consumers (kernfs_notify\
_workfn) because there can be multiple concurrent work items.

Suggested by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Imran Khan <imran.f.khan@oracle.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 fs/kernfs/file.c       | 47 ++++++++++++++++++------------------------
 include/linux/kernfs.h |  2 +-
 2 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 796f27333846..a8d8a9114b51 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -38,18 +38,16 @@ struct kernfs_open_node {
 	struct list_head	files; /* goes through kernfs_open_file.list */
 };
 
-/*
- * kernfs_notify() may be called from any context and bounces notifications
- * through a work item.  To minimize space overhead in kernfs_node, the
- * pending queue is implemented as a singly linked list of kernfs_nodes.
- * The list is terminated with the self pointer so that whether a
- * kernfs_node is on the list or not can be determined by testing the next
- * pointer for NULL.
+/**
+ * attribute_to_node - get kernfs_node object corresponding to a kernfs attribute
+ * @ptr:	&struct kernfs_elem_attr
+ * @type:	struct kernfs_node
+ * @member:	name of member (i.e attr)
  */
-#define KERNFS_NOTIFY_EOL			((void *)&kernfs_notify_list)
+#define attribute_to_node(ptr, type, member)	\
+	container_of(ptr, type, member)
 
-static DEFINE_SPINLOCK(kernfs_notify_lock);
-static struct kernfs_node *kernfs_notify_list = KERNFS_NOTIFY_EOL;
+static LLIST_HEAD(kernfs_notify_list);
 
 /*
  * Raw deref RCU protected kn->attr.open.
@@ -897,18 +895,16 @@ static void kernfs_notify_workfn(struct work_struct *work)
 	struct kernfs_node *kn;
 	struct kernfs_super_info *info;
 	struct kernfs_root *root;
+	struct llist_node *free;
+	struct kernfs_elem_attr *attr;
 repeat:
 	/* pop one off the notify_list */
-	spin_lock_irq(&kernfs_notify_lock);
-	kn = kernfs_notify_list;
-	if (kn == KERNFS_NOTIFY_EOL) {
-		spin_unlock_irq(&kernfs_notify_lock);
+	free = llist_del_first(&kernfs_notify_list);
+	if (free == NULL)
 		return;
-	}
-	kernfs_notify_list = kn->attr.notify_next;
-	kn->attr.notify_next = NULL;
-	spin_unlock_irq(&kernfs_notify_lock);
 
+	attr = llist_entry(free, struct kernfs_elem_attr, notify_next);
+	kn = attribute_to_node(attr, struct kernfs_node, attr);
 	root = kernfs_root(kn);
 	/* kick fsnotify */
 	down_write(&root->kernfs_rwsem);
@@ -964,12 +960,14 @@ static void kernfs_notify_workfn(struct work_struct *work)
 void kernfs_notify(struct kernfs_node *kn)
 {
 	static DECLARE_WORK(kernfs_notify_work, kernfs_notify_workfn);
-	unsigned long flags;
 	struct kernfs_open_node *on;
 
 	if (WARN_ON(kernfs_type(kn) != KERNFS_FILE))
 		return;
 
+	/* Because we are using llist for kernfs_notify_list */
+	WARN_ON_ONCE(in_nmi());
+
 	/* kick poll immediately */
 	rcu_read_lock();
 	on = rcu_dereference(kn->attr.open);
@@ -980,14 +978,9 @@ void kernfs_notify(struct kernfs_node *kn)
 	rcu_read_unlock();
 
 	/* schedule work to kick fsnotify */
-	spin_lock_irqsave(&kernfs_notify_lock, flags);
-	if (!kn->attr.notify_next) {
-		kernfs_get(kn);
-		kn->attr.notify_next = kernfs_notify_list;
-		kernfs_notify_list = kn;
-		schedule_work(&kernfs_notify_work);
-	}
-	spin_unlock_irqrestore(&kernfs_notify_lock, flags);
+	kernfs_get(kn);
+	llist_add(&kn->attr.notify_next, &kernfs_notify_list);
+	schedule_work(&kernfs_notify_work);
 }
 EXPORT_SYMBOL_GPL(kernfs_notify);
 
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 13f54f078a52..2dd9c8df0f4f 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -116,7 +116,7 @@ struct kernfs_elem_attr {
 	const struct kernfs_ops	*ops;
 	struct kernfs_open_node __rcu	*open;
 	loff_t			size;
-	struct kernfs_node	*notify_next;	/* for kernfs_notify() */
+	struct llist_node	notify_next;	/* for kernfs_notify() */
 };
 
 /*
-- 
2.30.2

[PATCH v3 3/4] kernfs: Introduce interface to access global kernfs_open_file_mutex.

[Imran Khan]

This allows to change underlying mutex locking, without needing to change
the users of the lock. For example next patch modifies this interface to
use hashed mutexes in place of a single global kernfs_open_file_mutex.

Signed-off-by: Imran Khan <imran.f.khan@oracle.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 fs/kernfs/file.c | 50 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index a8d8a9114b51..9000c85ce1e1 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -49,6 +49,22 @@ struct kernfs_open_node {
 
 static LLIST_HEAD(kernfs_notify_list);
 
+static inline struct mutex *kernfs_open_file_mutex_ptr(struct kernfs_node *kn)
+{
+	return &kernfs_open_file_mutex;
+}
+
+static inline struct mutex *kernfs_open_file_mutex_lock(struct kernfs_node *kn)
+{
+	struct mutex *lock;
+
+	lock = kernfs_open_file_mutex_ptr(kn);
+
+	mutex_lock(lock);
+
+	return lock;
+}
+
 /*
  * Raw deref RCU protected kn->attr.open.
  * If both @of->list and @kn->attr.open->files are non empty, we can safely
@@ -79,7 +95,7 @@ kernfs_deref_on_raw(struct kernfs_open_file *of, struct kernfs_node *kn)
 static struct kernfs_open_node *kernfs_deref_on_protected(struct kernfs_node *kn)
 {
 	return rcu_dereference_protected(kn->attr.open,
-					 lockdep_is_held(&kernfs_open_file_mutex));
+				 lockdep_is_held(kernfs_open_file_mutex_ptr(kn)));
 }
 
 /*
@@ -90,7 +106,7 @@ static struct kernfs_open_node *kernfs_deref_on_protected(struct kernfs_node *kn
 static struct kernfs_open_node *kernfs_check_on_protected(struct kernfs_node *kn)
 {
 	return rcu_dereference_check(kn->attr.open,
-				      lockdep_is_held(&kernfs_open_file_mutex));
+				 lockdep_is_held(kernfs_open_file_mutex_ptr(kn)));
 }
 
 static struct kernfs_open_file *kernfs_of(struct file *file)
@@ -569,19 +585,20 @@ static int kernfs_get_open_node(struct kernfs_node *kn,
 				struct kernfs_open_file *of)
 {
 	struct kernfs_open_node *on, *new_on = NULL;
+	struct mutex *mutex = NULL;
 
-	mutex_lock(&kernfs_open_file_mutex);
+	mutex = kernfs_open_file_mutex_lock(kn);
 	on = kernfs_deref_on_protected(kn);
 
 	if (on) {
 		list_add_tail(&of->list, &on->files);
-		mutex_unlock(&kernfs_open_file_mutex);
+		mutex_unlock(mutex);
 		return 0;
 	} else {
 		/* not there, initialize a new one */
 		new_on = kmalloc(sizeof(*new_on), GFP_KERNEL);
 		if (!new_on) {
-			mutex_unlock(&kernfs_open_file_mutex);
+			mutex_unlock(mutex);
 			return -ENOMEM;
 		}
 		atomic_set(&new_on->event, 1);
@@ -590,7 +607,7 @@ static int kernfs_get_open_node(struct kernfs_node *kn,
 		list_add_tail(&of->list, &new_on->files);
 		rcu_assign_pointer(kn->attr.open, new_on);
 	}
-	mutex_unlock(&kernfs_open_file_mutex);
+	mutex_unlock(mutex);
 
 	return 0;
 }
@@ -612,12 +629,13 @@ static void kernfs_unlink_open_file(struct kernfs_node *kn,
 				 struct kernfs_open_file *of)
 {
 	struct kernfs_open_node *on;
+	struct mutex *mutex = NULL;
 
-	mutex_lock(&kernfs_open_file_mutex);
+	mutex = kernfs_open_file_mutex_lock(kn);
 
 	on = kernfs_deref_on_protected(kn);
 	if (!on) {
-		mutex_unlock(&kernfs_open_file_mutex);
+		mutex_unlock(mutex);
 		return;
 	}
 
@@ -629,7 +647,7 @@ static void kernfs_unlink_open_file(struct kernfs_node *kn,
 		kfree_rcu(on, rcu_head);
 	}
 
-	mutex_unlock(&kernfs_open_file_mutex);
+	mutex_unlock(mutex);
 }
 
 static int kernfs_fop_open(struct inode *inode, struct file *file)
@@ -771,7 +789,7 @@ static void kernfs_release_file(struct kernfs_node *kn,
 	 * here because drain path may be called from places which can
 	 * cause circular dependency.
 	 */
-	lockdep_assert_held(&kernfs_open_file_mutex);
+	lockdep_assert_held(kernfs_open_file_mutex_ptr(kn));
 
 	if (!of->released) {
 		/*
@@ -788,11 +806,12 @@ static int kernfs_fop_release(struct inode *inode, struct file *filp)
 {
 	struct kernfs_node *kn = inode->i_private;
 	struct kernfs_open_file *of = kernfs_of(filp);
+	struct mutex *mutex = NULL;
 
 	if (kn->flags & KERNFS_HAS_RELEASE) {
-		mutex_lock(&kernfs_open_file_mutex);
+		mutex = kernfs_open_file_mutex_lock(kn);
 		kernfs_release_file(kn, of);
-		mutex_unlock(&kernfs_open_file_mutex);
+		mutex_unlock(mutex);
 	}
 
 	kernfs_unlink_open_file(kn, of);
@@ -807,6 +826,7 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
 {
 	struct kernfs_open_node *on;
 	struct kernfs_open_file *of;
+	struct mutex *mutex = NULL;
 
 	if (!(kn->flags & (KERNFS_HAS_MMAP | KERNFS_HAS_RELEASE)))
 		return;
@@ -822,10 +842,10 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
 	if (!rcu_access_pointer(kn->attr.open))
 		return;
 
-	mutex_lock(&kernfs_open_file_mutex);
+	mutex = kernfs_open_file_mutex_lock(kn);
 	on = kernfs_check_on_protected(kn);
 	if (!on) {
-		mutex_unlock(&kernfs_open_file_mutex);
+		mutex_unlock(mutex);
 		return;
 	}
 
@@ -839,7 +859,7 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
 			kernfs_release_file(kn, of);
 	}
 
-	mutex_unlock(&kernfs_open_file_mutex);
+	mutex_unlock(mutex);
 }
 
 /*
-- 
2.30.2

[PATCH v3 4/4] kernfs: Replace global kernfs_open_file_mutex with hashed mutexes.

[Imran Khan]

In current kernfs design a single mutex, kernfs_open_file_mutex, protects
the list of kernfs_open_file instances corresponding to a sysfs attribute.
So even if different tasks are opening or closing different sysfs files
they can contend on osq_lock of this mutex. The contention is more apparent
in large scale systems with few hundred CPUs where most of the CPUs have
running tasks that are opening, accessing or closing sysfs files at any
point of time.

Using hashed mutexes in place of a single global mutex, can significantly
reduce contention around global mutex and hence can provide better
scalability. Moreover as these hashed mutexes are not part of kernfs_node
objects we will not see any singnificant change in memory utilization of
kernfs based file systems like sysfs, cgroupfs etc.

Modify interface introduced in previous patch to make use of hashed
mutexes. Use kernfs_node address as hashing key.

Signed-off-by: Imran Khan <imran.f.khan@oracle.com>
---
 fs/kernfs/file.c            | 17 ++---------
 fs/kernfs/kernfs-internal.h |  4 +++
 fs/kernfs/mount.c           | 19 +++++++++++++
 include/linux/kernfs.h      | 57 +++++++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 14 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 9000c85ce1e1..175c9f53284f 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -18,19 +18,6 @@
 
 #include "kernfs-internal.h"
 
-/*
- * There's one kernfs_open_file for each open file and one kernfs_open_node
- * for each kernfs_node with one or more open files.
- *
- * kernfs_node->attr.open points to kernfs_open_node.  attr.open is
- * RCU protected.
- *
- * filp->private_data points to seq_file whose ->private points to
- * kernfs_open_file.  kernfs_open_files are chained at
- * kernfs_open_node->files, which is protected by kernfs_open_file_mutex.
- */
-static DEFINE_MUTEX(kernfs_open_file_mutex);
-
 struct kernfs_open_node {
 	struct rcu_head		rcu_head;
 	atomic_t		event;
@@ -51,7 +38,9 @@ static LLIST_HEAD(kernfs_notify_list);
 
 static inline struct mutex *kernfs_open_file_mutex_ptr(struct kernfs_node *kn)
 {
-	return &kernfs_open_file_mutex;
+	int idx = hash_ptr(kn, NR_KERNFS_LOCK_BITS);
+
+	return &kernfs_locks->open_file_mutex[idx];
 }
 
 static inline struct mutex *kernfs_open_file_mutex_lock(struct kernfs_node *kn)
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index eeaa779b929c..3ae214d02d44 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -164,4 +164,8 @@ void kernfs_drain_open_files(struct kernfs_node *kn);
  */
 extern const struct inode_operations kernfs_symlink_iops;
 
+/*
+ * kernfs locks
+ */
+extern struct kernfs_global_locks *kernfs_locks;
 #endif	/* __KERNFS_INTERNAL_H */
diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index cfa79715fc1a..d0859f72d2d6 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -20,6 +20,7 @@
 #include "kernfs-internal.h"
 
 struct kmem_cache *kernfs_node_cache, *kernfs_iattrs_cache;
+struct kernfs_global_locks *kernfs_locks;
 
 static int kernfs_sop_show_options(struct seq_file *sf, struct dentry *dentry)
 {
@@ -387,6 +388,22 @@ void kernfs_kill_sb(struct super_block *sb)
 	kfree(info);
 }
 
+static void __init kernfs_mutex_init(void)
+{
+	int count;
+
+	for (count = 0; count < NR_KERNFS_LOCKS; count++)
+		mutex_init(&kernfs_locks->open_file_mutex[count]);
+}
+
+static void __init kernfs_lock_init(void)
+{
+	kernfs_locks = kmalloc(sizeof(struct kernfs_global_locks), GFP_KERNEL);
+	WARN_ON(!kernfs_locks);
+
+	kernfs_mutex_init();
+}
+
 void __init kernfs_init(void)
 {
 	kernfs_node_cache = kmem_cache_create("kernfs_node_cache",
@@ -397,4 +414,6 @@ void __init kernfs_init(void)
 	kernfs_iattrs_cache  = kmem_cache_create("kernfs_iattrs_cache",
 					      sizeof(struct kernfs_iattrs),
 					      0, SLAB_PANIC, NULL);
+
+	kernfs_lock_init();
 }
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 2dd9c8df0f4f..13e703f615f7 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -18,6 +18,7 @@
 #include <linux/uidgid.h>
 #include <linux/wait.h>
 #include <linux/rwsem.h>
+#include <linux/cache.h>
 
 struct file;
 struct dentry;
@@ -34,6 +35,62 @@ struct kernfs_fs_context;
 struct kernfs_open_node;
 struct kernfs_iattrs;
 
+/*
+ * NR_KERNFS_LOCK_BITS determines size (NR_KERNFS_LOCKS) of hash
+ * table of locks.
+ * Having a small hash table would impact scalability, since
+ * more and more kernfs_node objects will end up using same lock
+ * and having a very large hash table would waste memory.
+ *
+ * At the moment size of hash table of locks is being set based on
+ * the number of CPUs as follows:
+ *
+ * NR_CPU      NR_KERNFS_LOCK_BITS      NR_KERNFS_LOCKS
+ *   1                  1                       2
+ *  2-3                 2                       4
+ *  4-7                 4                       16
+ *  8-15                6                       64
+ *  16-31               8                       256
+ *  32 and more         10                      1024
+ *
+ * The above relation between NR_CPU and number of locks is based
+ * on some internal experimentation which involved booting qemu
+ * with different values of smp, performing some sysfs operations
+ * on all CPUs and observing how increase in number of locks impacts
+ * completion time of these sysfs operations on each CPU.
+ */
+#ifdef CONFIG_SMP
+#define NR_KERNFS_LOCK_BITS (2 * (ilog2(NR_CPUS < 32 ? NR_CPUS : 32)))
+#else
+#define NR_KERNFS_LOCK_BITS     1
+#endif
+
+#define NR_KERNFS_LOCKS     (1 << NR_KERNFS_LOCK_BITS)
+
+/*
+ * There's one kernfs_open_file for each open file and one kernfs_open_node
+ * for each kernfs_node with one or more open files.
+ *
+ * filp->private_data points to seq_file whose ->private points to
+ * kernfs_open_file.
+ *
+ * kernfs_open_files are chained at kernfs_open_node->files, which is
+ * protected by kernfs_global_locks.open_file_mutex[i].
+ *
+ * To reduce possible contention in sysfs access, arising due to single
+ * locks, use an array of locks (e.g. open_file_mutex) and use kernfs_node
+ * object address as hash keys to get the index of these locks.
+ *
+ * Hashed mutexes are safe to use here because operations using these don't
+ * rely on global exclusion.
+ *
+ * In future we intend to replace other global locks with hashed ones as well.
+ * kernfs_global_locks acts as a holder for all such hash tables.
+ */
+struct kernfs_global_locks {
+	struct mutex open_file_mutex[NR_KERNFS_LOCKS];
+};
+
 enum kernfs_node_type {
 	KERNFS_DIR		= 0x0001,
 	KERNFS_FILE		= 0x0002,
-- 
2.30.2

Re: [PATCH v3 1/4] kernfs: make ->attr.open RCU protected.

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6165 bytes --]

CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220511095157.478522-2-imran.f.khan@oracle.com>
References: <20220511095157.478522-2-imran.f.khan@oracle.com>
TO: Imran Khan <imran.f.khan@oracle.com>

Hi Imran,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 3bf222d317a20170ee17f082626c1e0f83537e13]

url:    https://github.com/intel-lab-lkp/linux/commits/Imran-Khan/kernfs-make-attr-open-RCU-protected/20220511-175730
base:   3bf222d317a20170ee17f082626c1e0f83537e13
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20220514/202205141708.2SQYmW1K-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
fs/kernfs/file.c:282 kernfs_file_read_iter() warn: inconsistent returns '&of->mutex'.

Old smatch warnings:
fs/kernfs/file.c:255 kernfs_file_read_iter() warn: possible memory leak of 'buf'
fs/kernfs/file.c:282 kernfs_file_read_iter() warn: possible memory leak of 'buf'

vim +282 fs/kernfs/file.c

414985ae23c031 Tejun Heo         2013-11-28  219  
414985ae23c031 Tejun Heo         2013-11-28  220  /*
414985ae23c031 Tejun Heo         2013-11-28  221   * As reading a bin file can have side-effects, the exact offset and bytes
414985ae23c031 Tejun Heo         2013-11-28  222   * specified in read(2) call should be passed to the read callback making
414985ae23c031 Tejun Heo         2013-11-28  223   * it difficult to use seq_file.  Implement simplistic custom buffering for
414985ae23c031 Tejun Heo         2013-11-28  224   * bin files.
414985ae23c031 Tejun Heo         2013-11-28  225   */
4eaad21a6ac986 Christoph Hellwig 2021-01-20  226  static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
414985ae23c031 Tejun Heo         2013-11-28  227  {
4eaad21a6ac986 Christoph Hellwig 2021-01-20  228  	struct kernfs_open_file *of = kernfs_of(iocb->ki_filp);
4eaad21a6ac986 Christoph Hellwig 2021-01-20  229  	ssize_t len = min_t(size_t, iov_iter_count(iter), PAGE_SIZE);
414985ae23c031 Tejun Heo         2013-11-28  230  	const struct kernfs_ops *ops;
302de586c74f7b Imran Khan        2022-05-11  231  	struct kernfs_open_node *on;
414985ae23c031 Tejun Heo         2013-11-28  232  	char *buf;
414985ae23c031 Tejun Heo         2013-11-28  233  
4ef67a8c95f32e NeilBrown         2014-10-14  234  	buf = of->prealloc_buf;
e4234a1fc343ca Chris Wilson      2016-03-31  235  	if (buf)
e4234a1fc343ca Chris Wilson      2016-03-31  236  		mutex_lock(&of->prealloc_mutex);
e4234a1fc343ca Chris Wilson      2016-03-31  237  	else
414985ae23c031 Tejun Heo         2013-11-28  238  		buf = kmalloc(len, GFP_KERNEL);
414985ae23c031 Tejun Heo         2013-11-28  239  	if (!buf)
414985ae23c031 Tejun Heo         2013-11-28  240  		return -ENOMEM;
414985ae23c031 Tejun Heo         2013-11-28  241  
414985ae23c031 Tejun Heo         2013-11-28  242  	/*
4ef67a8c95f32e NeilBrown         2014-10-14  243  	 * @of->mutex nests outside active ref and is used both to ensure that
e4234a1fc343ca Chris Wilson      2016-03-31  244  	 * the ops aren't called concurrently for the same open file.
414985ae23c031 Tejun Heo         2013-11-28  245  	 */
414985ae23c031 Tejun Heo         2013-11-28  246  	mutex_lock(&of->mutex);
c637b8acbe079e Tejun Heo         2013-12-11  247  	if (!kernfs_get_active(of->kn)) {
414985ae23c031 Tejun Heo         2013-11-28  248  		len = -ENODEV;
414985ae23c031 Tejun Heo         2013-11-28  249  		mutex_unlock(&of->mutex);
414985ae23c031 Tejun Heo         2013-11-28  250  		goto out_free;
414985ae23c031 Tejun Heo         2013-11-28  251  	}
414985ae23c031 Tejun Heo         2013-11-28  252  
302de586c74f7b Imran Khan        2022-05-11  253  	on = kernfs_deref_on_raw(of, of->kn);
302de586c74f7b Imran Khan        2022-05-11  254  	if (!on)
302de586c74f7b Imran Khan        2022-05-11  255  		return -EINVAL;
302de586c74f7b Imran Khan        2022-05-11  256  
302de586c74f7b Imran Khan        2022-05-11  257  	of->event = atomic_read(&unrcu_pointer(on)->event);
324a56e16e44ba Tejun Heo         2013-12-11  258  	ops = kernfs_ops(of->kn);
414985ae23c031 Tejun Heo         2013-11-28  259  	if (ops->read)
4eaad21a6ac986 Christoph Hellwig 2021-01-20  260  		len = ops->read(of, buf, len, iocb->ki_pos);
414985ae23c031 Tejun Heo         2013-11-28  261  	else
414985ae23c031 Tejun Heo         2013-11-28  262  		len = -EINVAL;
414985ae23c031 Tejun Heo         2013-11-28  263  
e4234a1fc343ca Chris Wilson      2016-03-31  264  	kernfs_put_active(of->kn);
e4234a1fc343ca Chris Wilson      2016-03-31  265  	mutex_unlock(&of->mutex);
e4234a1fc343ca Chris Wilson      2016-03-31  266  
414985ae23c031 Tejun Heo         2013-11-28  267  	if (len < 0)
e4234a1fc343ca Chris Wilson      2016-03-31  268  		goto out_free;
414985ae23c031 Tejun Heo         2013-11-28  269  
4eaad21a6ac986 Christoph Hellwig 2021-01-20  270  	if (copy_to_iter(buf, len, iter) != len) {
414985ae23c031 Tejun Heo         2013-11-28  271  		len = -EFAULT;
e4234a1fc343ca Chris Wilson      2016-03-31  272  		goto out_free;
414985ae23c031 Tejun Heo         2013-11-28  273  	}
414985ae23c031 Tejun Heo         2013-11-28  274  
4eaad21a6ac986 Christoph Hellwig 2021-01-20  275  	iocb->ki_pos += len;
414985ae23c031 Tejun Heo         2013-11-28  276  
414985ae23c031 Tejun Heo         2013-11-28  277   out_free:
e4234a1fc343ca Chris Wilson      2016-03-31  278  	if (buf == of->prealloc_buf)
e4234a1fc343ca Chris Wilson      2016-03-31  279  		mutex_unlock(&of->prealloc_mutex);
e4234a1fc343ca Chris Wilson      2016-03-31  280  	else
414985ae23c031 Tejun Heo         2013-11-28  281  		kfree(buf);
414985ae23c031 Tejun Heo         2013-11-28 @282  	return len;
414985ae23c031 Tejun Heo         2013-11-28  283  }
414985ae23c031 Tejun Heo         2013-11-28  284  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 1/4] kernfs: make ->attr.open RCU protected.

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 5130 bytes --]

Hi Imran,

url:    https://github.com/intel-lab-lkp/linux/commits/Imran-Khan/kernfs-make-attr-open-RCU-protected/20220511-175730
base:   3bf222d317a20170ee17f082626c1e0f83537e13
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20220514/202205141708.2SQYmW1K-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
fs/kernfs/file.c:282 kernfs_file_read_iter() warn: inconsistent returns '&of->mutex'.

Old smatch warnings:
fs/kernfs/file.c:255 kernfs_file_read_iter() warn: possible memory leak of 'buf'
fs/kernfs/file.c:282 kernfs_file_read_iter() warn: possible memory leak of 'buf'

vim +282 fs/kernfs/file.c

4eaad21a6ac986 Christoph Hellwig 2021-01-20  226  static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
414985ae23c031 Tejun Heo         2013-11-28  227  {
4eaad21a6ac986 Christoph Hellwig 2021-01-20  228  	struct kernfs_open_file *of = kernfs_of(iocb->ki_filp);
4eaad21a6ac986 Christoph Hellwig 2021-01-20  229  	ssize_t len = min_t(size_t, iov_iter_count(iter), PAGE_SIZE);
414985ae23c031 Tejun Heo         2013-11-28  230  	const struct kernfs_ops *ops;
302de586c74f7b Imran Khan        2022-05-11  231  	struct kernfs_open_node *on;
414985ae23c031 Tejun Heo         2013-11-28  232  	char *buf;
414985ae23c031 Tejun Heo         2013-11-28  233  
4ef67a8c95f32e NeilBrown         2014-10-14  234  	buf = of->prealloc_buf;
e4234a1fc343ca Chris Wilson      2016-03-31  235  	if (buf)
e4234a1fc343ca Chris Wilson      2016-03-31  236  		mutex_lock(&of->prealloc_mutex);
e4234a1fc343ca Chris Wilson      2016-03-31  237  	else
414985ae23c031 Tejun Heo         2013-11-28  238  		buf = kmalloc(len, GFP_KERNEL);
414985ae23c031 Tejun Heo         2013-11-28  239  	if (!buf)
414985ae23c031 Tejun Heo         2013-11-28  240  		return -ENOMEM;
414985ae23c031 Tejun Heo         2013-11-28  241  
414985ae23c031 Tejun Heo         2013-11-28  242  	/*
4ef67a8c95f32e NeilBrown         2014-10-14  243  	 * @of->mutex nests outside active ref and is used both to ensure that
e4234a1fc343ca Chris Wilson      2016-03-31  244  	 * the ops aren't called concurrently for the same open file.
414985ae23c031 Tejun Heo         2013-11-28  245  	 */
414985ae23c031 Tejun Heo         2013-11-28  246  	mutex_lock(&of->mutex);
c637b8acbe079e Tejun Heo         2013-12-11  247  	if (!kernfs_get_active(of->kn)) {
414985ae23c031 Tejun Heo         2013-11-28  248  		len = -ENODEV;
414985ae23c031 Tejun Heo         2013-11-28  249  		mutex_unlock(&of->mutex);
414985ae23c031 Tejun Heo         2013-11-28  250  		goto out_free;
414985ae23c031 Tejun Heo         2013-11-28  251  	}
414985ae23c031 Tejun Heo         2013-11-28  252  
302de586c74f7b Imran Khan        2022-05-11  253  	on = kernfs_deref_on_raw(of, of->kn);
302de586c74f7b Imran Khan        2022-05-11  254  	if (!on)
302de586c74f7b Imran Khan        2022-05-11  255  		return -EINVAL;

	len = -EINVAL;
	goto out_free;

302de586c74f7b Imran Khan        2022-05-11  256  
302de586c74f7b Imran Khan        2022-05-11  257  	of->event = atomic_read(&unrcu_pointer(on)->event);
324a56e16e44ba Tejun Heo         2013-12-11  258  	ops = kernfs_ops(of->kn);
414985ae23c031 Tejun Heo         2013-11-28  259  	if (ops->read)
4eaad21a6ac986 Christoph Hellwig 2021-01-20  260  		len = ops->read(of, buf, len, iocb->ki_pos);
414985ae23c031 Tejun Heo         2013-11-28  261  	else
414985ae23c031 Tejun Heo         2013-11-28  262  		len = -EINVAL;
414985ae23c031 Tejun Heo         2013-11-28  263  
e4234a1fc343ca Chris Wilson      2016-03-31  264  	kernfs_put_active(of->kn);
e4234a1fc343ca Chris Wilson      2016-03-31  265  	mutex_unlock(&of->mutex);
e4234a1fc343ca Chris Wilson      2016-03-31  266  
414985ae23c031 Tejun Heo         2013-11-28  267  	if (len < 0)
e4234a1fc343ca Chris Wilson      2016-03-31  268  		goto out_free;
414985ae23c031 Tejun Heo         2013-11-28  269  
4eaad21a6ac986 Christoph Hellwig 2021-01-20  270  	if (copy_to_iter(buf, len, iter) != len) {
414985ae23c031 Tejun Heo         2013-11-28  271  		len = -EFAULT;
e4234a1fc343ca Chris Wilson      2016-03-31  272  		goto out_free;
414985ae23c031 Tejun Heo         2013-11-28  273  	}
414985ae23c031 Tejun Heo         2013-11-28  274  
4eaad21a6ac986 Christoph Hellwig 2021-01-20  275  	iocb->ki_pos += len;
414985ae23c031 Tejun Heo         2013-11-28  276  
414985ae23c031 Tejun Heo         2013-11-28  277   out_free:
e4234a1fc343ca Chris Wilson      2016-03-31  278  	if (buf == of->prealloc_buf)
e4234a1fc343ca Chris Wilson      2016-03-31  279  		mutex_unlock(&of->prealloc_mutex);
e4234a1fc343ca Chris Wilson      2016-03-31  280  	else
414985ae23c031 Tejun Heo         2013-11-28  281  		kfree(buf);
414985ae23c031 Tejun Heo         2013-11-28 @282  	return len;
414985ae23c031 Tejun Heo         2013-11-28  283  }

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp