From hsu@FreeBSD.org Tue Dec 3 19:20:32 2002 Date: Tue, 03 Dec 2002 12:39:12 -0800 From: Jeffrey Hsu To: rwatson@freebsd.org, jlemon@freebsd.org, jhb@freebsd.org Subject: fixes for file descriptor locking in knote_attach() I really dislike the file descriptor locking that was added to knote_attach() because the locking approach there is too fine-grain (and buggy too). I think a kqueue subsystem lock would be more fitting. However, in the meantime, the following patch almost makes what was done to there palletable. It removes the bogus locks and unlocks around free(), which doesn't sleep and so doesn't need those locks and unlocks. It eliminates the retry loop, which did unnecessary computation, and, was not safe from compiler code movement and optimization either. Finally, it adds a memory reload where needed after possibly sleeping. The patch is attached in both context format and unidiff format. Jeffrey [Part 2, "knote_attach.diff.context" Text/PLAIN 188 lines] [Unable to print this part] [Part 3, "knote_attach.diff.u" Text/PLAIN 100 lines] [Unable to print this part] From jhb@FreeBSD.org Tue Dec 3 19:20:32 2002 Date: Tue, 03 Dec 2002 16:37:31 -0500 (EST) From: John Baldwin To: Jeffrey Hsu Cc: jlemon@freebsd.org, rwatson@freebsd.org Subject: RE: fixes for file descriptor locking in knote_attach() On 03-Dec-2002 Jeffrey Hsu wrote: > I really dislike the file descriptor locking that was added to > knote_attach() because the locking approach there is too fine-grain > (and buggy too). I think a kqueue subsystem lock would be more > fitting. However, in the meantime, the following patch almost makes > what was done to there palletable. It removes the bogus locks and > unlocks around free(), which doesn't sleep and so doesn't need those > locks and unlocks. It eliminates the retry loop, which did unnecessary > computation, and, was not safe from compiler code movement and > optimization either. Finally, it adds a memory reload where needed > after possibly sleeping. The patch is attached in both context > format and unidiff format. Do you really need to use explicit volatile casts to get the compiler to reload? If so, we are in lots of trouble as there will be tons of code broken. A better approach would be to somehow mark lock acquire and release functions (as well as tsleep and cv_*wait) so that it doesn't assume values are valid across them. -- John Baldwin <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ From hsu@FreeBSD.org Tue Dec 3 19:20:32 2002 Date: Tue, 03 Dec 2002 14:16:51 -0800 From: Jeffrey Hsu To: John Baldwin Cc: jlemon@freebsd.org, rwatson@freebsd.org Subject: Re: fixes for file descriptor locking in knote_attach() > Do you really need to use explicit volatile casts to get the > compiler to reload? Yes. It's either that or declare the field as volatile. Alternatively, we could turn off compiler optimizations. > If so, we are in lots of trouble as there will be tons of code broken. The reload is needed here---that much is agreed upon. The way to force the compiler to reload is to use a volatile. One can use side-effects to force the compiler to be conservative, that is, passing a pointer to a subroutine will case the compiler to reload when next de-referencing that pointer. > A better approach would be to > somehow mark lock acquire and release functions (as well as tsleep > and cv_*wait) so that it doesn't assume values are valid across > them. This could be accomplished sort-of with the side-effect kludge mentioned above, but, one would have to specify all the variables to invalidate, forcing the burden back on the programmer. The problem is that register allocation and common sub-expression elimination are valid and commonly used compiler techniques. Turning those off on every lock acquire and release would really pessimize the code unnecessarily. John Mashey talks about similiar experiences having to make a pass through their code at SGI to make their OS safe for modern compilers at http://yarchive.net/comp/volatile.html. He talks mainly about interrupt service routines, but in general [1], a variable should be declared as volatile if it is a memory-mapped I/O port the variable is shared between multiple concurrent processes it is modified by an interrupt service routine or it is an automatic object declared in a function that calls setjmp and whose value is changed between the call to setjmp and a corresponding call to longjmp Jeffrey [1] http://www.programmersheaven.com/articles/pathak/article1.htm From jhb@FreeBSD.org Tue Dec 3 19:20:32 2002 Date: Tue, 03 Dec 2002 18:52:56 -0500 (EST) From: John Baldwin To: Jeffrey Hsu Cc: rwatson@freebsd.org, jlemon@freebsd.org Subject: Re: fixes for file descriptor locking in knote_attach() On 03-Dec-2002 Jeffrey Hsu wrote: > > Do you really need to use explicit volatile casts to get the > > compiler to reload? > > Yes. It's either that or declare the field as volatile. Alternatively, > we could turn off compiler optimizations. > > > If so, we are in lots of trouble as there will be tons of code broken. > > The reload is needed here---that much is agreed upon. The way to force > the compiler to reload is to use a volatile. One can use side-effects > to force the compiler to be conservative, that is, passing a pointer to > a subroutine will case the compiler to reload when next de-referencing > that pointer. > > > A better approach would be to > > somehow mark lock acquire and release functions (as well as tsleep > > and cv_*wait) so that it doesn't assume values are valid across > > them. > > This could be accomplished sort-of with the side-effect kludge mentioned > above, but, one would have to specify all the variables to invalidate, > forcing the burden back on the programmer. > > The problem is that register allocation and common sub-expression > elimination are valid and commonly used compiler techniques. Turning > those off on every lock acquire and release would really pessimize > the code unnecessarily. John Mashey talks about similiar experiences > having to make a pass through their code at SGI to make their OS safe for > modern compilers at http://yarchive.net/comp/volatile.html. He talks > mainly about interrupt service routines, but in general [1], a variable > should be declared as volatile if > > it is a memory-mapped I/O port > the variable is shared between multiple concurrent processes > it is modified by an interrupt service routine > or it is an automatic object declared in a function that calls setjmp and > whose value is changed between the call to setjmp and a corresponding call to > longjmp Egads, then there are reams of things that need to be volatile now. Like almost all of struct proc and friends. :( Basically anything protected by a lock now needs to be volatile since if it didn't need to be volatile it wouldn't need a lock. :( -- John Baldwin <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ From hsu@FreeBSD.org Tue Dec 3 19:20:33 2002 Date: Tue, 03 Dec 2002 16:18:44 -0800 From: Jeffrey Hsu To: John Baldwin Cc: rwatson@freebsd.org, jlemon@freebsd.org Subject: Re: fixes for file descriptor locking in knote_attach() > Egads, then there are reams of things that need to be volatile now. I did some experimentation with gcc 3.2.1 on x86. On this architecture and this compiler release, we're generally safe, by a quirk of the implementation. The compiler will not elide the second test unless the intervening function call is not inlined. In other words, the following code will generate 2 tests for s->i: struct somestruct { int i; }; int f(struct somestruct *s) { if (s->i == 0) { g(); if (s->i == 0) { printf("hi\n"); } } } But if the function g() is in visible and does not touch somestruct, then the second test will be omitted unless it is declared as volatile. (A simple cast will not do, though arguably it should. I'll modify that part of my patch accordingly.) However, since mtx_lock() and mtx_unlock() are in their own files, we're generally saved, except possibly in kern_mutex.c. So, there's no need to get alarmed at this point. Jeffrey