From: Konstantin Osipov Date: May 6 2009 2:35pm Subject: Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780 List-Archive: http://lists.mysql.com/commits/73504 Message-Id: <20090506143508.GB6274@ibbur> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT * Ingo Strüwing [09/05/06 12:43]: > > your patch is bound to solve the present day trouble only unless > > you write an instruction to developers that they can follow > > when writing new code: an instruction how to write "killable" and > > race-free code. > Where shall this instruction got to? Where is the old instruction? The old instruction is now running an own company and an own fork of MySQL database. > I tried to include the instruction at every wait condition. I think that > a developer, who wants to write "killable" code, will look at other > places where similar things are done. So I hope, he will find one of the > repeated comments. But I've already received the comment that these many > repetitions don't seem useful. Now I'm gathering opinions, which place > might be optimal for a single copy. In that regard I can only agree with Rusty Russell's opinion: it would be best if it's simply impossible to use wrong. (http://www.pointy-stick.com/blog/2008/01/09/api-design-rusty-levels/) More practically, a good interface that you don't have to copy-paste from somewhere to know how to use and that's documented in internals manual is IMHO quite adequate. A possibly better interface for enter_cond() would be to replace it with THD::cond_wait() and THD::cond_timedwait(). Inside THD::cond_wait(), we can set thd->current_cond, thd->current_mutex (ideally, first having taking LOCK_delete, but our colleagues from PAE would never let us do that), check for thd->killed, and return if it's set. Then we will invoke pthread_cond_wait() or pthread_cond_timedwait(), then clear the current cond, and return the result of the pthread call. This way we can make sure that in no place we forget to check for thd->killed before falling through into a sleep on a cond, or set thd->current_cond, or clear it. > > When you move the checks for thd->killed into critical section, > > you change the conceptual model of thd->enter_cond(). > > According to the old conceptual model it was unnecessary, since > > thd->awake cancels the wait on the condition (at least such was > > the pitch). > > > It does still cancel the wait on the condition. I don't understand what > you mean with me to "change the conceptual model of thd->enter_cond()". > As far as I understand the "conceptual model of thd->enter_cond()", it > is not affected in its main feature: Making it possible to kick a thread > out of a wait condition in an exceptional way. There are many places in which a thread can block. InnoDB, vio, pthread condition waits, file I/O, long computations. If I tried to describe the current model here's perhaps what I would say: ------------------------------------------------------------- User contract: by sending KILL once, or, in some cases, few times, you can interrupt a running SQL statement, regardless of where it is. Implementation: The implementation uses cooperation between threads. In order to be interruptible, a running thread needs to check for its thd->killed every so often and terminate execution with error if it's set. There is no need to report an error (my_error()) upon discovery of thd->killed: an error is reported once at the top level in dispatch_command(). Killing a thread consists of two parts: sending the message and interrupting the thread if it's blocked on a system call. To send a message, we set thd->killed and my_thread_var->abort in the respective THD. There is no memory barrier or any other synchronization primitive to ensure that the setting is seen in a synchronous fashion -- however, regardless of the memory architecture, we know it will eventually be seen. To interrupt from a blocking I/O we use varying mechanisms that generally are within register/notify paradigm. For the networking, we use the thr_alarm API. Now, it doesn't work on all platforms, so someone, as part of a bugfix, added close_active_vio() to thd->awake, which is a hack (and I even approved it). For condition variables, we use thd->enter_cond() and my_thread_var->current_cond (notice -- thd->enter_cond() is not the only mechanism out there). Plug-ins are bound to use their own implementation. But they can find out if the thread was killed by invoking thd_killed() plugin API function. --------------------------------------------------------------- > way to transport the exception itself. The way, how to check it, may be > seen as a peripheral recommendation. If you think, I'm wrong, please > explain in detail, what is the conceptual model is and why the > (additional) check for thd->killed changes the model. When we bring up the complete picture, it's clear, that by "making it more reliable" we're at best only making one part of it more reliable. The "model" currently is that the connection always needs to check for thd->killed after having taking the mutex but before falling through into a sleep on the respective cond. Once it's done, one check is enough. Whether it's before the call to thd->enter_cond() or after it, is unimportant. I admit I haven't looked carefully enough at the nature of your changes to see whether they deviate from the above or not. But in any case I would first like to make sure we're on the same page on how it should work first. > > thd->killed has never been too reliable. It wasn't designed to > > be. In real life a user can send the signal twice without too much > > trouble. But the way it worked was simple and easy to understand. > > My aim is to make thd->killed more reliable. If only to support > regression testing. Great. Let's kill thd->killed and start using my_thread_var->abort universally, since thd->killed is a duplication of my_thread_var->abort. Let's introduce an API to register a wait on a condition that can be used in plug-ins and replace the current thd->enter_cond() in the server with it. Let's fix all places that read thd->killed without a mutex, to guarantee that there are no memory visibility issues. An interface that I described above is a way to get it done. Then let's kill thr_alarm and start using poll/select in IO instead, that can be interrupted reliably with pthread_signal() on all platforms we currently support. Let's start using joinable threads, instead of detached, so that we can reliably wait on a thread to shutdown, rather than use unreliable "wait until the thread unregisters itself" approach. Sorry, got carried away :) But once again, I would like to make sure we share a common architecture vision of how the kill-ability should be improved, since you say you're on a quest to improve it. > I'm sorry to hear that you don't find it simple and easy to understand > that the check for thd->killed should not (only) be done before > enter_cond(), but (also) after it. Yes, I don't see how thd->enter_cond() is relevant. Let's take event_queue.cc case for example. Could you explain to me how checking thd->killed after thd->enter_cond() is relevant using this example? Thanks, -- kostja