List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:May 6 2009 2:35pm
Subject:Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780
View as plain text  
* Ingo Strüwing <Ingo.Struewing@stripped> [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
Thread
bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Struewing25 Feb
  • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Davi Arnaut5 May
    • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing5 May
      • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Konstantin Osipov5 May
        • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing6 May
          • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Konstantin Osipov6 May
            • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing6 May
              • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Konstantin Osipov6 May
                • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing7 May
                  • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing12 May
              • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Davi Arnaut6 May
      • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Davi Arnaut6 May
        • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing6 May
          • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Konstantin Osipov6 May
            • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing7 May
          • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Davi Arnaut7 May