From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nathan Zimmer Subject: Re: [PATCH v3 resend] procfs: Improve Scaling in proc Date: Fri, 15 Feb 2013 17:39:38 -0600 Message-ID: <511EC73A.4050008@sgi.com> References: <1360961274-24652-1-git-send-email-nzimmer@sgi.com> <20130215141239.99a3bccc.akpm@linux-foundation.org> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Cc: , , , , "Eric W. Biederman" , David Woodhouse , Alexey Dobriyan , "Paul E. McKenney" To: Andrew Morton Return-path: Received: from relay2.sgi.com ([192.48.179.30]:56331 "EHLO relay.sgi.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750733Ab3BOXj0 (ORCPT ); Fri, 15 Feb 2013 18:39:26 -0500 In-Reply-To: <20130215141239.99a3bccc.akpm@linux-foundation.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 02/15/2013 04:12 PM, Andrew Morton wrote: > On Fri, 15 Feb 2013 14:47:54 -0600 > Nathan Zimmer wrote: > >> 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 replaces the lock with the 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/resend of what was orignally suggested by Eric Dumazet some >> time ago. >> >> 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 >> >> ... >> >> diff --git a/fs/proc/generic.c b/fs/proc/generic.c >> index 76ddae8..6896a70 100644 >> --- a/fs/proc/generic.c >> +++ b/fs/proc/generic.c >> @@ -191,13 +191,16 @@ proc_file_read(struct file *file, char __user *buf, size_t nbytes, >> struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode); >> ssize_t rv = -EIO; >> >> - spin_lock(&pde->pde_unload_lock); >> - if (!pde->proc_fops) { >> - spin_unlock(&pde->pde_unload_lock); >> + const struct file_operations *fops; > There's now a stray newline in the definitions section. Noted and corrected, in a few places. >> + 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(); > So what's up with pde_users? Seems that it's atomic_t *and* uses a > form of RCU protection. We can't make it a plain old integer because > it's modified under rcu_read_lock() and we can't move the atomic_inc() > outside rcu_read_lock() because of the synchronization games in > remove_proc_entry()? The intent of pde_users is to let us know when it is safe to clean out the pde_openers. I probably should comment this. >> rv = __proc_file_read(file, buf, nbytes, ppos); >> >> >> ... >> >> @@ -802,37 +809,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. */ >> >> + DECLARE_COMPLETION_ONSTACK(c); > This should have generated a c99-style definition warning. Did your > compiler version not do this? A clear over site on my part. >> + 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--; >> >> ... >> >> 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->f_path.dentry->d_inode); >> 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. >> */ > This comment needs updating. > > However, it doesn't appear to be true any more. With this patch we no > longer set ->fops to NULL in remove_proc_entry(). (What replaced that > logic?) > > So are all these games with local variable `llseek' still needed? > afaict the increment of pde_users will stabilize ->fops? We still are setting de->proc_fops to NULL to prevent new callers. Also we still have to save fops-> since we cannot use fops outside the rcu_read_un/lock. Unless I misunderstood your question. But yes the comment needs to be updated. > >> - llseek = pde->proc_fops->llseek; >> - spin_unlock(&pde->pde_unload_lock); >> + llseek = fops->llseek; >> + rcu_read_unlock(); >> >> if (!llseek) >> llseek = default_llseek; >> @@ -182,15 +176,17 @@ static ssize_t proc_reg_read(struct file *file, char __user *buf, size_t count, >> struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode); >> 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(); > Many dittoes. > >> if (read) >> rv = read(file, buf, count, ppos); >> @@ -204,15 +200,17 @@ static ssize_t proc_reg_write(struct file *file, const char __user *buf, size_t >> struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode); >> 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(); >> >> ... >> Thanks, Nate