[PATCH v5 1/2] kernfs: use hashed mutex and spinlock in place of global ones.

From: Imran Khan
Date: Sat Feb 05 2022 - 21:18:15 EST

Next message: Andrey Smirnov: "[PATCH] platform/x86: Add Steam Deck driver"
Previous message: Wei Yang: "Re: [PATCH] mm/page_alloc: add zone to zonelist if populated"
In reply to: Imran Khan: "[PATCH v5 2/2] kernfs: Replace per-fs global rwsem with per-fs hashed rwsem."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Right now a global mutex (kernfs_open_file_mutex) protects 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. Since each list of kernfs_open_file belongs to a
kernfs_open_node instance which in turn corresponds to one kernfs_node,
moving global kernfs_open_file_mutex within kernfs_node would sound like
fixing this contention but it has unwanted side effect of bloating up
kernfs_node size and hence kobject memory usage.

Also since kernfs_node->attr.open points to kernfs_open_node instance
corresponding to the kernfs_node, we can use a kernfs_node specific
spinlock in place of current global spinlock i.e kernfs_open_node_lock.
But this approach will increase kobject memory usage as well.

Use per-fs hashed locks in place of above mentioned global locks to reduce
kernfs access contention without increasing kobject memory usage.

Signed-off-by: Imran Khan <imran.f.khan@xxxxxxxxxx>
---
fs/kernfs/dir.c | 5 +++
fs/kernfs/file.c | 61 ++++++++++++++++---------------------
fs/kernfs/kernfs-internal.h | 51 +++++++++++++++++++++++++++++++
include/linux/kernfs.h | 39 ++++++++++++++++++++++++
4 files changed, 122 insertions(+), 34 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index e6d9772ddb4ca..d26fb3bffda92 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -909,6 +909,7 @@ struct kernfs_root *kernfs_create_root(struct kernfs_syscall_ops *scops,
{
struct kernfs_root *root;
struct kernfs_node *kn;
+ int lock_count;

root = kzalloc(sizeof(*root), GFP_KERNEL);
if (!root)
@@ -916,6 +917,10 @@ struct kernfs_root *kernfs_create_root(struct kernfs_syscall_ops *scops,

idr_init(&root->ino_idr);
init_rwsem(&root->kernfs_rwsem);
+ for (lock_count = 0; lock_count < NR_KERNFS_LOCKS; lock_count++) {
+ spin_lock_init(&root->open_node_locks[lock_count].lock);
+ mutex_init(&root->open_file_mutex[lock_count].lock);
+ }
INIT_LIST_HEAD(&root->supers);

/*
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 9414a7a60a9f4..018d038b72fdd 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -18,20 +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
- * protected by kernfs_open_node_lock.
- *
- * 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 {
atomic_t refcnt;
atomic_t event;
@@ -524,10 +510,11 @@ 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;
+ spinlock_t *lock = NULL;
retry:
- mutex_lock(&kernfs_open_file_mutex);
- spin_lock_irq(&kernfs_open_node_lock);
+ mutex = kernfs_open_file_mutex_lock(kn);
+ lock = kernfs_open_node_lock(kn);

if (!kn->attr.open && new_on) {
kn->attr.open = new_on;
@@ -540,8 +527,8 @@ static int kernfs_get_open_node(struct kernfs_node *kn,
list_add_tail(&of->list, &on->files);
}

- spin_unlock_irq(&kernfs_open_node_lock);
- mutex_unlock(&kernfs_open_file_mutex);
+ spin_unlock_irq(lock);
+ mutex_unlock(mutex);

if (on) {
kfree(new_on);
@@ -575,10 +562,14 @@ static void kernfs_put_open_node(struct kernfs_node *kn,
struct kernfs_open_file *of)
{
struct kernfs_open_node *on = kn->attr.open;
+ struct mutex *mutex = NULL;
+ spinlock_t *lock = NULL;
unsigned long flags;

- mutex_lock(&kernfs_open_file_mutex);
- spin_lock_irqsave(&kernfs_open_node_lock, flags);
+ mutex = kernfs_open_file_mutex_lock(kn);
+ lock = kernfs_open_node_lock_ptr(kn);
+
+ spin_lock_irqsave(lock, flags);

if (of)
list_del(&of->list);
@@ -588,8 +579,8 @@ static void kernfs_put_open_node(struct kernfs_node *kn,
else
on = NULL;

- spin_unlock_irqrestore(&kernfs_open_node_lock, flags);
- mutex_unlock(&kernfs_open_file_mutex);
+ spin_unlock_irqrestore(lock, flags);
+ mutex_unlock(mutex);

kfree(on);
}
@@ -729,11 +720,11 @@ static void kernfs_release_file(struct kernfs_node *kn,
/*
* @of is guaranteed to have no other file operations in flight and
* we just want to synchronize release and drain paths.
- * @kernfs_open_file_mutex is enough. @of->mutex can't be used
+ * @open_file_mutex is enough. @of->mutex can't be used
* 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) {
/*
@@ -750,11 +741,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 *lock = NULL;

if (kn->flags & KERNFS_HAS_RELEASE) {
- mutex_lock(&kernfs_open_file_mutex);
+ lock = kernfs_open_file_mutex_lock(kn);
kernfs_release_file(kn, of);
- mutex_unlock(&kernfs_open_file_mutex);
+ mutex_unlock(lock);
}

kernfs_put_open_node(kn, of);
@@ -769,19 +761,21 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
{
struct kernfs_open_node *on;
struct kernfs_open_file *of;
+ struct mutex *mutex = NULL;
+ spinlock_t *lock = NULL;

if (!(kn->flags & (KERNFS_HAS_MMAP | KERNFS_HAS_RELEASE)))
return;

- spin_lock_irq(&kernfs_open_node_lock);
+ lock = kernfs_open_node_lock(kn);
on = kn->attr.open;
if (on)
atomic_inc(&on->refcnt);
- spin_unlock_irq(&kernfs_open_node_lock);
+ spin_unlock_irq(lock);
if (!on)
return;

- mutex_lock(&kernfs_open_file_mutex);
+ mutex = kernfs_open_file_mutex_lock(kn);

list_for_each_entry(of, &on->files, list) {
struct inode *inode = file_inode(of->file);
@@ -793,8 +787,7 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
kernfs_release_file(kn, of);
}

- mutex_unlock(&kernfs_open_file_mutex);
-
+ mutex_unlock(mutex);
kernfs_put_open_node(kn, NULL);
}

@@ -922,13 +915,13 @@ void kernfs_notify(struct kernfs_node *kn)
return;

/* kick poll immediately */
- spin_lock_irqsave(&kernfs_open_node_lock, flags);
+ spin_lock_irqsave(kernfs_open_node_lock_ptr(kn), flags);
on = kn->attr.open;
if (on) {
atomic_inc(&on->event);
wake_up_interruptible(&on->poll);
}
- spin_unlock_irqrestore(&kernfs_open_node_lock, flags);
+ spin_unlock_irqrestore(kernfs_open_node_lock_ptr(kn), flags);

/* schedule work to kick fsnotify */
spin_lock_irqsave(&kernfs_notify_lock, flags);
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index f9cc912c31e1b..cc49a6cd94154 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -31,6 +31,7 @@ struct kernfs_iattrs {
atomic_t user_xattr_size;
};

+
/* +1 to avoid triggering overflow warning when negating it */
#define KN_DEACTIVATED_BIAS (INT_MIN + 1)

@@ -147,4 +148,54 @@ void kernfs_drain_open_files(struct kernfs_node *kn);
*/
extern const struct inode_operations kernfs_symlink_iops;

+static inline spinlock_t *kernfs_open_node_lock_ptr(struct kernfs_node *kn)
+{
+ struct kernfs_root *root;
+ int idx = hash_ptr(kn, NR_KERNFS_LOCK_BITS);
+
+ root = kernfs_root(kn);
+
+ return &root->open_node_locks[idx].lock;
+}
+
+static inline spinlock_t *kernfs_open_node_lock(struct kernfs_node *kn)
+{
+ struct kernfs_root *root;
+ spinlock_t *lock;
+ int idx = hash_ptr(kn, NR_KERNFS_LOCK_BITS);
+
+ root = kernfs_root(kn);
+
+ lock = &root->open_node_locks[idx].lock;
+
+ spin_lock_irq(lock);
+
+ return lock;
+}
+
+static inline struct mutex *kernfs_open_file_mutex_ptr(struct kernfs_node *kn)
+{
+ struct kernfs_root *root;
+ int idx = hash_ptr(kn, NR_KERNFS_LOCK_BITS);
+
+ root = kernfs_root(kn);
+
+ return &root->open_file_mutex[idx].lock;
+}
+
+static inline struct mutex *kernfs_open_file_mutex_lock(struct kernfs_node *kn)
+{
+ struct kernfs_root *root;
+ struct mutex *lock;
+ int idx = hash_ptr(kn, NR_KERNFS_LOCK_BITS);
+
+ root = kernfs_root(kn);
+
+ lock = &root->open_file_mutex[idx].lock;
+
+ mutex_lock(lock);
+
+ return lock;
+}
+
#endif /* __KERNFS_INTERNAL_H */
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 861c4f0f8a29f..5bf9f02ce9dce 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -18,6 +18,8 @@
#include <linux/uidgid.h>
#include <linux/wait.h>
#include <linux/rwsem.h>
+#include <linux/spinlock.h>
+#include <linux/cache.h>

struct file;
struct dentry;
@@ -34,6 +36,40 @@ 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
+ */
+#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)
+
+struct kernfs_open_node_lock {
+ spinlock_t lock;
+} ____cacheline_aligned_in_smp;
+
+struct kernfs_open_file_mutex {
+ struct mutex lock;
+} ____cacheline_aligned_in_smp;
+
enum kernfs_node_type {
KERNFS_DIR = 0x0001,
KERNFS_FILE = 0x0002,
@@ -90,6 +126,7 @@ enum kernfs_root_flag {
KERNFS_ROOT_SUPPORT_USER_XATTR = 0x0008,
};

+
/* type-specific structures for kernfs_node union members */
struct kernfs_elem_dir {
unsigned long subdirs;
@@ -201,6 +238,8 @@ struct kernfs_root {

wait_queue_head_t deactivate_waitq;
struct rw_semaphore kernfs_rwsem;
+ struct kernfs_open_node_lock open_node_locks[NR_KERNFS_LOCKS];
+ struct kernfs_open_file_mutex open_file_mutex[NR_KERNFS_LOCKS];
};

struct kernfs_open_file {
--
2.30.2

Next message: Andrey Smirnov: "[PATCH] platform/x86: Add Steam Deck driver"
Previous message: Wei Yang: "Re: [PATCH] mm/page_alloc: add zone to zonelist if populated"
In reply to: Imran Khan: "[PATCH v5 2/2] kernfs: Replace per-fs global rwsem with per-fs hashed rwsem."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]