From: Nathan Zimmer Date: Wed, 20 Mar 2013 04:08:31 +0000 (+1100) Subject: procfs: improve scaling in proc X-Git-Tag: next-20130321~2^2~96 X-Git-Url: https://git.karo-electronics.de/?a=commitdiff_plain;h=fd2227aacbf8782cc7f67c1522b11f2f98fcbc5c;p=karo-tx-linux.git procfs: improve scaling in proc I am currently tracking a hotlock reported by a customer on a large system, 512 cores. I am currently running 3.8-rc7 but the issue looks like it has been this way for a very long time. The offending lock is proc_dir_entry->pde_unload_lock. This patch converts the lock to use rcu. However the pde_openers list still is controlled by a spin lock. I tested on a 4096 machine and the lock doesn't seem hot at least according to perf. This is a refresh of what was orignally suggested by Eric Dumazet some time ago. I have also taken in some comments from Andrew and several other people whose names escape me but I am quite grateful too. Supporting numbers, lower is better, they are from the test I posted earlier. cpuinfo baseline Rcu tasks read-sec read-sec 1 0.0141 0.0141 2 0.0140 0.0142 4 0.0140 0.0141 8 0.0145 0.0140 16 0.0553 0.0168 32 0.1688 0.0549 64 0.5017 0.1690 128 1.7005 0.5038 256 5.2513 2.0804 512 8.0529 3.0162 Signed-off-by: Nathan Zimmer Cc: "Eric W. Biederman" Cc: Eric Dumazet Cc: Alexander Viro Cc: David Woodhouse Cc: Alexey Dobriyan Cc: "Paul E. McKenney" Signed-off-by: Andrew Morton --- diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 4b3b3ffb52f1..c5450183ca78 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c @@ -181,14 +181,16 @@ proc_file_read(struct file *file, char __user *buf, size_t nbytes, { struct proc_dir_entry *pde = PDE(file_inode(file)); ssize_t rv = -EIO; + const struct file_operations *fops; - spin_lock(&pde->pde_unload_lock); - if (!pde->proc_fops) { - spin_unlock(&pde->pde_unload_lock); + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); + if (!fops) { + rcu_read_unlock(); return rv; } - pde->pde_users++; - spin_unlock(&pde->pde_unload_lock); + atomic_inc(&pde->pde_users); + rcu_read_unlock(); rv = __proc_file_read(file, buf, nbytes, ppos); @@ -204,13 +206,16 @@ proc_file_write(struct file *file, const char __user *buffer, ssize_t rv = -EIO; if (pde->write_proc) { - spin_lock(&pde->pde_unload_lock); - if (!pde->proc_fops) { - spin_unlock(&pde->pde_unload_lock); + const struct file_operations *fops; + + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); + if (!fops) { + rcu_read_unlock(); return rv; } - pde->pde_users++; - spin_unlock(&pde->pde_unload_lock); + atomic_inc(&pde->pde_users); + rcu_read_unlock(); /* FIXME: does this routine need ppos? probably... */ rv = pde->write_proc(file, buffer, count, pde->data); @@ -542,7 +547,7 @@ static int proc_register(struct proc_dir_entry * dir, struct proc_dir_entry * dp if (S_ISDIR(dp->mode)) { if (dp->proc_iops == NULL) { - dp->proc_fops = &proc_dir_operations; + RCU_INIT_POINTER(dp->proc_fops, &proc_dir_operations); dp->proc_iops = &proc_dir_inode_operations; } dir->nlink++; @@ -551,7 +556,7 @@ static int proc_register(struct proc_dir_entry * dir, struct proc_dir_entry * dp dp->proc_iops = &proc_link_inode_operations; } else if (S_ISREG(dp->mode)) { if (dp->proc_fops == NULL) - dp->proc_fops = &proc_file_operations; + RCU_INIT_POINTER(dp->proc_fops, &proc_file_operations); if (dp->proc_iops == NULL) dp->proc_iops = &proc_file_inode_operations; } @@ -604,7 +609,8 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent, ent->mode = mode; ent->nlink = nlink; atomic_set(&ent->count, 1); - spin_lock_init(&ent->pde_unload_lock); + atomic_set(&ent->pde_users, 1); + spin_lock_init(&ent->pde_openers_lock); INIT_LIST_HEAD(&ent->pde_openers); out: return ent; @@ -728,7 +734,7 @@ struct proc_dir_entry *proc_create_data(const char *name, umode_t mode, pde = __proc_create(&parent, name, mode, nlink); if (!pde) goto out; - pde->proc_fops = proc_fops; + rcu_assign_pointer(pde->proc_fops, proc_fops); pde->data = data; if (proc_register(parent, pde) < 0) goto out_free; @@ -764,6 +770,7 @@ void remove_proc_entry(const char *name, struct proc_dir_entry *parent) struct proc_dir_entry *de = NULL; const char *fn = name; unsigned int len; + DECLARE_COMPLETION_ONSTACK(c); spin_lock(&proc_subdir_lock); if (__xlate_proc_name(name, &parent, &fn) != 0) { @@ -786,37 +793,30 @@ void remove_proc_entry(const char *name, struct proc_dir_entry *parent) return; } - spin_lock(&de->pde_unload_lock); /* * Stop accepting new callers into module. If you're * dynamically allocating ->proc_fops, save a pointer somewhere. */ - de->proc_fops = NULL; - /* Wait until all existing callers into module are done. */ - if (de->pde_users > 0) { - DECLARE_COMPLETION_ONSTACK(c); - - if (!de->pde_unload_completion) - de->pde_unload_completion = &c; - - spin_unlock(&de->pde_unload_lock); - + rcu_assign_pointer(de->proc_fops, NULL); + synchronize_rcu(); + /* + * Wait until all existing callers into module are done. + * Once pde_users hits zero we are free to clean out pde_openers. + */ + de->pde_unload_completion = &c; + if (!atomic_dec_and_test(&de->pde_users)) wait_for_completion(de->pde_unload_completion); - spin_lock(&de->pde_unload_lock); - } - + spin_lock(&de->pde_openers_lock); while (!list_empty(&de->pde_openers)) { struct pde_opener *pdeo; pdeo = list_first_entry(&de->pde_openers, struct pde_opener, lh); list_del(&pdeo->lh); - spin_unlock(&de->pde_unload_lock); pdeo->release(pdeo->inode, pdeo->file); kfree(pdeo); - spin_lock(&de->pde_unload_lock); } - spin_unlock(&de->pde_unload_lock); + spin_unlock(&de->pde_openers_lock); if (S_ISDIR(de->mode)) parent->nlink--; diff --git a/fs/proc/inode.c b/fs/proc/inode.c index a86aebc9ba7c..6cccc4d7a106 100644 --- a/fs/proc/inode.c +++ b/fs/proc/inode.c @@ -129,46 +129,41 @@ static const struct super_operations proc_sops = { .show_options = proc_show_options, }; -static void __pde_users_dec(struct proc_dir_entry *pde) -{ - pde->pde_users--; - if (pde->pde_unload_completion && pde->pde_users == 0) - complete(pde->pde_unload_completion); -} - void pde_users_dec(struct proc_dir_entry *pde) { - spin_lock(&pde->pde_unload_lock); - __pde_users_dec(pde); - spin_unlock(&pde->pde_unload_lock); + if (atomic_dec_and_test(&pde->pde_users) && pde->pde_unload_completion) + complete(pde->pde_unload_completion); } static loff_t proc_reg_llseek(struct file *file, loff_t offset, int whence) { + const struct file_operations *fops; struct proc_dir_entry *pde = PDE(file_inode(file)); loff_t rv = -EINVAL; loff_t (*llseek)(struct file *, loff_t, int); - spin_lock(&pde->pde_unload_lock); + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); /* * remove_proc_entry() is going to delete PDE (as part of module * cleanup sequence). No new callers into module allowed. */ - if (!pde->proc_fops) { - spin_unlock(&pde->pde_unload_lock); + if (!fops) { + rcu_read_unlock(); return rv; } /* * Bump refcount so that remove_proc_entry will wail for ->llseek to * complete. */ - pde->pde_users++; + atomic_inc(&pde->pde_users); + /* - * Save function pointer under lock, to protect against ->proc_fops - * NULL'ifying right after ->pde_unload_lock is dropped. + * Save function pointer under rcu lock, to protect against + * ->proc_fops NULL'ifying by remove_proc_entry. */ - llseek = pde->proc_fops->llseek; - spin_unlock(&pde->pde_unload_lock); + llseek = fops->llseek; + rcu_read_unlock(); if (!llseek) llseek = default_llseek; @@ -183,15 +178,17 @@ static ssize_t proc_reg_read(struct file *file, char __user *buf, size_t count, struct proc_dir_entry *pde = PDE(file_inode(file)); ssize_t rv = -EIO; ssize_t (*read)(struct file *, char __user *, size_t, loff_t *); + const struct file_operations *fops; - spin_lock(&pde->pde_unload_lock); - if (!pde->proc_fops) { - spin_unlock(&pde->pde_unload_lock); + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); + if (!fops) { + rcu_read_unlock(); return rv; } - pde->pde_users++; - read = pde->proc_fops->read; - spin_unlock(&pde->pde_unload_lock); + atomic_inc(&pde->pde_users); + read = fops->read; + rcu_read_unlock(); if (read) rv = read(file, buf, count, ppos); @@ -205,15 +202,17 @@ static ssize_t proc_reg_write(struct file *file, const char __user *buf, size_t struct proc_dir_entry *pde = PDE(file_inode(file)); ssize_t rv = -EIO; ssize_t (*write)(struct file *, const char __user *, size_t, loff_t *); + const struct file_operations *fops; - spin_lock(&pde->pde_unload_lock); - if (!pde->proc_fops) { - spin_unlock(&pde->pde_unload_lock); + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); + if (!fops) { + rcu_read_unlock(); return rv; } - pde->pde_users++; - write = pde->proc_fops->write; - spin_unlock(&pde->pde_unload_lock); + atomic_inc(&pde->pde_users); + write = fops->write; + rcu_read_unlock(); if (write) rv = write(file, buf, count, ppos); @@ -227,15 +226,17 @@ static unsigned int proc_reg_poll(struct file *file, struct poll_table_struct *p struct proc_dir_entry *pde = PDE(file_inode(file)); unsigned int rv = DEFAULT_POLLMASK; unsigned int (*poll)(struct file *, struct poll_table_struct *); + const struct file_operations *fops; - spin_lock(&pde->pde_unload_lock); - if (!pde->proc_fops) { - spin_unlock(&pde->pde_unload_lock); + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); + if (!fops) { + rcu_read_unlock(); return rv; } - pde->pde_users++; - poll = pde->proc_fops->poll; - spin_unlock(&pde->pde_unload_lock); + atomic_inc(&pde->pde_users); + poll = fops->poll; + rcu_read_unlock(); if (poll) rv = poll(file, pts); @@ -249,15 +250,17 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne struct proc_dir_entry *pde = PDE(file_inode(file)); long rv = -ENOTTY; long (*ioctl)(struct file *, unsigned int, unsigned long); + const struct file_operations *fops; - spin_lock(&pde->pde_unload_lock); - if (!pde->proc_fops) { - spin_unlock(&pde->pde_unload_lock); + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); + if (!fops) { + rcu_read_unlock(); return rv; } - pde->pde_users++; - ioctl = pde->proc_fops->unlocked_ioctl; - spin_unlock(&pde->pde_unload_lock); + atomic_inc(&pde->pde_users); + ioctl = fops->unlocked_ioctl; + rcu_read_unlock(); if (ioctl) rv = ioctl(file, cmd, arg); @@ -272,15 +275,17 @@ static long proc_reg_compat_ioctl(struct file *file, unsigned int cmd, unsigned struct proc_dir_entry *pde = PDE(file_inode(file)); long rv = -ENOTTY; long (*compat_ioctl)(struct file *, unsigned int, unsigned long); + const struct file_operations *fops; - spin_lock(&pde->pde_unload_lock); - if (!pde->proc_fops) { - spin_unlock(&pde->pde_unload_lock); + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); + if (!fops) { + rcu_read_unlock(); return rv; } - pde->pde_users++; + atomic_inc(&pde->pde_users); compat_ioctl = pde->proc_fops->compat_ioctl; - spin_unlock(&pde->pde_unload_lock); + rcu_read_unlock(); if (compat_ioctl) rv = compat_ioctl(file, cmd, arg); @@ -295,15 +300,17 @@ static int proc_reg_mmap(struct file *file, struct vm_area_struct *vma) struct proc_dir_entry *pde = PDE(file_inode(file)); int rv = -EIO; int (*mmap)(struct file *, struct vm_area_struct *); + const struct file_operations *fops; - spin_lock(&pde->pde_unload_lock); - if (!pde->proc_fops) { - spin_unlock(&pde->pde_unload_lock); + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); + if (!fops) { + rcu_read_unlock(); return rv; } - pde->pde_users++; + atomic_inc(&pde->pde_users); mmap = pde->proc_fops->mmap; - spin_unlock(&pde->pde_unload_lock); + rcu_read_unlock(); if (mmap) rv = mmap(file, vma); @@ -319,6 +326,7 @@ static int proc_reg_open(struct inode *inode, struct file *file) int (*open)(struct inode *, struct file *); int (*release)(struct inode *, struct file *); struct pde_opener *pdeo; + const struct file_operations *fops; /* * What for, you ask? Well, we can have open, rmmod, remove_proc_entry @@ -334,32 +342,33 @@ static int proc_reg_open(struct inode *inode, struct file *file) if (!pdeo) return -ENOMEM; - spin_lock(&pde->pde_unload_lock); - if (!pde->proc_fops) { - spin_unlock(&pde->pde_unload_lock); + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); + if (!fops) { + rcu_read_unlock(); kfree(pdeo); return -ENOENT; } - pde->pde_users++; - open = pde->proc_fops->open; - release = pde->proc_fops->release; - spin_unlock(&pde->pde_unload_lock); + atomic_inc(&pde->pde_users); + open = fops->open; + release = fops->release; + rcu_read_lock(); if (open) rv = open(inode, file); - spin_lock(&pde->pde_unload_lock); if (rv == 0 && release) { /* To know what to release. */ pdeo->inode = inode; pdeo->file = file; /* Strictly for "too late" ->release in proc_reg_release(). */ pdeo->release = release; + spin_lock(&pde->pde_openers_lock); list_add(&pdeo->lh, &pde->pde_openers); + spin_unlock(&pde->pde_openers_lock); } else kfree(pdeo); - __pde_users_dec(pde); - spin_unlock(&pde->pde_unload_lock); + pde_users_dec(pde); return rv; } @@ -368,10 +377,14 @@ static struct pde_opener *find_pde_opener(struct proc_dir_entry *pde, { struct pde_opener *pdeo; + spin_lock(&pde->pde_openers_lock); list_for_each_entry(pdeo, &pde->pde_openers, lh) { - if (pdeo->inode == inode && pdeo->file == file) + if (pdeo->inode == inode && pdeo->file == file) { + spin_unlock(&pde->pde_openers_lock); return pdeo; + } } + spin_unlock(&pde->pde_openers_lock); return NULL; } @@ -381,10 +394,12 @@ static int proc_reg_release(struct inode *inode, struct file *file) int rv = 0; int (*release)(struct inode *, struct file *); struct pde_opener *pdeo; + const struct file_operations *fops; - spin_lock(&pde->pde_unload_lock); pdeo = find_pde_opener(pde, inode, file); - if (!pde->proc_fops) { + rcu_read_lock(); + fops = rcu_dereference(pde->proc_fops); + if (!fops) { /* * Can't simply exit, __fput() will think that everything is OK, * and move on to freeing struct file. remove_proc_entry() will @@ -394,21 +409,23 @@ static int proc_reg_release(struct inode *inode, struct file *file) * But if opener is removed from list, who will ->release it? */ if (pdeo) { + spin_lock(&pde->pde_openers_lock); list_del(&pdeo->lh); - spin_unlock(&pde->pde_unload_lock); + spin_unlock(&pde->pde_openers_lock); rv = pdeo->release(inode, file); kfree(pdeo); - } else - spin_unlock(&pde->pde_unload_lock); + } return rv; } - pde->pde_users++; - release = pde->proc_fops->release; + atomic_inc(&pde->pde_users); + release = fops->release; + rcu_read_unlock(); if (pdeo) { + spin_lock(&pde->pde_openers_lock); list_del(&pdeo->lh); - kfree(pdeo); + spin_unlock(&pde->pde_openers_lock); } - spin_unlock(&pde->pde_unload_lock); + kfree(pdeo); if (release) rv = release(inode, file); diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h index 8307f2f94d86..bd828e095359 100644 --- a/include/linux/proc_fs.h +++ b/include/linux/proc_fs.h @@ -68,16 +68,16 @@ struct proc_dir_entry { * If you're allocating ->proc_fops dynamically, save a pointer * somewhere. */ - const struct file_operations *proc_fops; + const struct file_operations __rcu *proc_fops; struct proc_dir_entry *next, *parent, *subdir; void *data; read_proc_t *read_proc; write_proc_t *write_proc; atomic_t count; /* use count */ - int pde_users; /* number of callers into module in progress */ + atomic_t pde_users; /* number of callers into module in progress */ struct completion *pde_unload_completion; + spinlock_t pde_openers_lock; struct list_head pde_openers; /* who did ->open, but not ->release */ - spinlock_t pde_unload_lock; /* proc_fops checks and pde_users bumps */ u8 namelen; char name[]; };