Killing the Big Kernel Lock
in [Kernel]
|
Prev: Darlehen bieten bei 2% Zins (Union beschränkt Finanzierung)
Next: [PATCH] rt2500usb: improve powersaving reliability
From: Roland Dreier on 31 Mar 2010 18:20 Hi Arnd, Looking at your tree, I see you have commit 753dd249 ("perf_event: use nonseekable_open") that does: > --- a/kernel/perf_event.c > +++ b/kernel/perf_event.c > @@ -2515,6 +2515,8 @@ static int perf_fasync(int fd, struct file *filp, int on) > } > > static const struct file_operations perf_fops = { > + .open = nonseekable_open, > + .llseek = no_llseek, > .release = perf_release, > .read = perf_read, > .poll = perf_poll, But if I understand this correctly, the assignment to .open is at best useless -- these file_operations are only used via anon_inode_getfd() and so there is no possible path that can call the .open method. Or am I missing something? (The same applies to the kvm_main.c changes too) -- Roland Dreier <rolandd(a)cisco.com> || For corporate legal information go to: http://www.cisco.com/web/about/doing_business/legal/cri/index.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 31 Mar 2010 18:30 On Wed, Mar 31, 2010 at 03:11:03PM -0700, Roland Dreier wrote: > Hi Arnd, > > Looking at your tree, I see you have commit 753dd249 ("perf_event: use > nonseekable_open") that does: > > > --- a/kernel/perf_event.c > > +++ b/kernel/perf_event.c > > @@ -2515,6 +2515,8 @@ static int perf_fasync(int fd, struct file *filp, int on) > > } > > > > static const struct file_operations perf_fops = { > > + .open = nonseekable_open, > > + .llseek = no_llseek, > > .release = perf_release, > > .read = perf_read, > > .poll = perf_poll, > > But if I understand this correctly, the assignment to .open is at best > useless -- these file_operations are only used via anon_inode_getfd() > and so there is no possible path that can call the .open method. Or am > I missing something? > > (The same applies to the kvm_main.c changes too) Good point, I'll update that. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 1 Apr 2010 05:00 On Thursday 01 April 2010, Roland Dreier wrote: > Looking at your tree, I see you have commit 753dd249 ("perf_event: use > nonseekable_open") that does: > > > --- a/kernel/perf_event.c > > +++ b/kernel/perf_event.c > > @@ -2515,6 +2515,8 @@ static int perf_fasync(int fd, struct file *filp, int on) > > } > > > > static const struct file_operations perf_fops = { > > + .open = nonseekable_open, > > + .llseek = no_llseek, > > .release = perf_release, > > .read = perf_read, > > .poll = perf_poll, > > But if I understand this correctly, the assignment to .open is at best > useless -- these file_operations are only used via anon_inode_getfd() > and so there is no possible path that can call the .open method. Or am > I missing something? You're right. I did not consider this. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jan Blunck on 8 Apr 2010 16:50 On Sun, Mar 28, Arnd Bergmann wrote: > On Sunday 28 March 2010, Stefan Richter wrote: > > Arnd Bergmann wrote: > > > Your patches look good, but it would be helpful to also set .llseek = no_llseek > > > in the file operations, because that is much easier to grep for than > > > only the nonseekable_open. While it's technically a NOP on the presence of > > > nonseekable_open, it will help that I don't accidentally apply my patch on > > > top of yours. > > > > Sounds like a plan, but (a) if my .llseek = no_llseek and your .llseek = > > default_llseek are not within diff context range, you (or whoever else > > merges mine and yours) only get a compiler warning (Initializer entry > > defined twice) rather than a merge conflict which couldn't be missed, > > (b) there won't be a merge conflict in "BKL removal: mark remaining > > users as 'depends on BKL'". (c) While I don't mind adding more visual > > clutter to ieee1394/*, I prefer terse coding in firewire/*. > > > > How about I put my nonseekable_open additions into a release branch and > > send you a pull request after a few days exposure in linux-next? If you > > do not plan to respin your patch queue soon or at all, I could even let > > you pull a for-arnd branch with a semantically correct merge of yours > > and mine. > > I can probably remember this specific one now, but for other people > doing the same on their subsystems, adding no_llseek may help reduce > the need for coordination. > > > General thoughts: > > > > ".llseek = NULL," so far meant "do the Right Thing on lseek() and > > friends, as far as the fs core can tell". Shouldn't we keep it that > > way? It's as close to other ".method = NULL," as it can get, which > > either mean "silently skip this method if it doesn't matter" (e.g. > > .flush) or "fail attempts to use this method with a fitting errno" (e.g. > > .write). > > My series changes the default from 'default_llseek' to 'generic_file_llseek', That is not that easy. generic_file_llseek() is testing against 'offset < inode->i_sb->s_maxbytes'. This is not necessarily true when you think about directories with random offset cookies. I know that seeking on directories is stupid but don't blame me. > which is almost identical, except for taking the inode mutex instead of the > BKL. Another option that has been discussed before is to make no_llseek > the default, but that might cause more serious problems wiht drivers that > really require seeking. > > Since using default_llseek can only ever make a difference if the driver > actually uses the BKL in any other function, I could go through the > patches again and revert those that do no use the BKL anywhere else. > > > Of course, as we have already seen with infiniband, firewire, ieee1394, > > .llseek = NULL is ambiguous in practice. Does the driver really want to > > use default_llseek, or should it rather use no_llseek and/or > > nonseekable_open, or should it even implement a dummy_llseek() { return > > 0; } which avoids the BKL but preserves ABI behaviour? This needs to be > > resolved for each and every case eventually, regardless of whether or > > when your addition of .llseek = default_llseek enters mainline. > > Yes, that also sounds like a good idea. I believe that Jan actually posted > a patch to do that at some point. Yes, it is in http://git.infradead.org/users/jblunck/linux-2.6.git bkl/default-lseek There are some other patches in that branch that are not upstream yet. Mind to take them for your bkl-removal branch? Cheers, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 8 Apr 2010 17:30
On Thursday 08 April 2010 22:45:45 Jan Blunck wrote: > On Sun, Mar 28, Arnd Bergmann wrote: > > > General thoughts: > > > > > > ".llseek = NULL," so far meant "do the Right Thing on lseek() and > > > friends, as far as the fs core can tell". Shouldn't we keep it that > > > way? It's as close to other ".method = NULL," as it can get, which > > > either mean "silently skip this method if it doesn't matter" (e.g. > > > .flush) or "fail attempts to use this method with a fitting errno" (e.g. > > > .write). > > > > My series changes the default from 'default_llseek' to 'generic_file_llseek', > > That is not that easy. generic_file_llseek() is testing against 'offset < > inode->i_sb->s_maxbytes'. This is not necessarily true when you think about > directories with random offset cookies. I know that seeking on directories is > stupid but don't blame me. Oh, I see. Would it work if we extend generic_file_llseek to only check s_maxbytes if S_ISREG(inode->i_mode)))? > > Yes, that also sounds like a good idea. I believe that Jan actually posted > > a patch to do that at some point. > > Yes, it is in > > http://git.infradead.org/users/jblunck/linux-2.6.git bkl/default-lseek > > There are some other patches in that branch that are not upstream yet. Mind to > take them for your bkl-removal branch? Frederic is now collecting the new patches. Your default-lseek series looks good to me, except for the obvious one that says 'FIXME' in the subject. Maybe Frederic can add your series except for that one as another branch to get pulled into his kill-the-bkl master branch. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |