The changes to the CALLOUT_PROCESS flag are not necessary for the fix. I left them in just for consistency. I used the flag during debugging, so it was important for me that it is set and removed at proper places.

Hi,

The title of this patch is a bit confusing to me.

callout_stop() is not allowed to drain, because that will cause a sleep and callout_stop() is a non-blocking function.

Do you mean callout_async_drain() or callout_drain() ?

Instead of introducing another flag, CALLOUT_RUNNING, you should be able to deduce this state from the cc_xxx() variables ???

How would your patch look for hps_head?
https://svnweb.freebsd.org/base/projects/hps_head/sys/kern/kern_timeout.c?annotate=299262

Is this a bug or new feature? Do you need to update the callout manual page?

--HPS

I agree with this change spirit:

callout_async_drain()/callout_stop() should not return success when the callout is actually running, even if the scheduled one has be successfully cancelled. Otherwise this is indeed confusing for callout_async_drain()/callout_stop() users (like me).

Would be nice to have:

• Make it clear in callout man page

I tried to introduced this behavior (see below) but I prefer your approach (clearer):

callout_stop() should return 0 when the callout is currently being serviced and indeed unstoppable.
https://reviews.freebsd.org/D3078?vs=on&id=8254&whitespace=ignore-most#toc

I am going to test this change especially against TCP timer callouts

callout_stop() should return 0 when the callout is currently being serviced and indeed unstoppable.

Hi,

I think this approach will add extra complications to the code.

I suggest to use:

static void
dummy_drain(void *arg)
{
}

ret = callout_async_drain(co, &dummy_drain, NULL);

Which is basically what you want and allow callout_async_drain() to update the callback function if called multiple times.

Instead of:

ret = callout_stop()

In this case. It returns the correct value for the client.

If callout_stop() always return 0 in the MPSAFE cc_exec_curr == c, it breaks the callout API possibly with out-of-the-tree code which we don't control.

--HPS

In D7042#147452, @hselasky wrote:

callout_stop() should return 0 when the callout is currently being serviced and indeed unstoppable.

I think this approach will add extra complications to the code.

I suggest to use:

static void
dummy_drain(void *arg)
{
}

ret = callout_async_drain(co, &dummy_drain, NULL);

Which is basically what you want and allow callout_async_drain() to update the callback function if called multiple times.

Instead of:

ret = callout_stop()

In this case. It returns the correct value for the client.

If callout_stop() always return 0 in the MPSAFE cc_exec_curr == c, it breaks the callout API possibly with out-of-the-tree code which we don't control.

I also agree callout_stop() is an old API, breaking it has a cost...

Limiting D7042 change spirit to only callout_async_drain() will be already enough for me (let me update my previous comment). And as callout_async_drain() is a new API and currently only used in TCP timer, very low cost.

But still ok to modify both callout_async_drain() and callout_stop() behaviour (less confusion).

I think this might re-introduce the unkillable threads issue in the sleepqueue code, since there is seems to be a fine difference between !PENDING and RUNNING, exactly when callout is simultaneously reset and running. Am I right ?

Either way, just limiting the scope of the change to drain would remove the case I worry about from the scope at all.

I would not agree that there is API change for callout_stop(). Let's look into the manual page:

The function callout_stop() cancels a callout c if it is currently
pending.  If the callout is pending and successfully stopped, then
callout_stop() returns a value of one.  If the callout is not set, or has
already been serviced, then negative one is returned.  If the callout is
currently being serviced and cannot be stopped, then zero will be
returned.  If the callout has an associated lock, then that lock must be
held when this function is called.

The problem is that the whole paradigm of callouts is that it has three consequent states: not scheduled -> scheduled -> running -> not scheduled. The API and the manual page assume that, some comments in the code assume that, and looks like some contributors to the code also did. But this paradigm isn't true. A callout can be scheduled and running at the same time. And the mishandling of this state was the source of our TCP panics. Now let's get back to the documentation and see what should callout_stop() return if callout is scheduled and running at the same time. It should return 0 because callout is currently being serviced and can't be stopped. At the same time it shouldreturn 1, since it has successfully unscheduled a callout. So it must be 0 and 1 at the same time! :) Yep, which is impossible. The API is limited by wrong paradigm and we must select the most safe value between these two. Now, I assert that returning 1 is much less safe in this case. Because any properly designed user of the API will be ready to free resources that are now in use by running callout. This is exactly what happened with TCP. Any other user, use it async version or not, is also prone to panic if we return 1. I assert that returning 0 will make behaviour more safe, and won't break any users, since this is what a sane programmer would expect from API. To finalize, I would notice that this state of running+scheduled in practice is extremely rare, if we noticed it only now at above 50 Gbit/s. So, I'm strongly against limiting the behavior only to async version.

P.S. I should have put the above long text into the summary of the review. But the review was started in a hurry. Drew said to me that guys are right now on a that secret phone call, and are willing to see my patch, so I should put it on phabricator ASAP.

Julien, I think your observation is correct and right now I'm already testing that. If you are right, then we actually can deduce running state from cc_exec_curr, as Hans suggests. I will keep you updated on results.

Kostik, I failed to see how can sleepqueue case be affected by the change. Can you please provide more details?

Looks good to me, let me launch our TCP QA on it.

In D7042#147487, @glebius wrote:

I would not agree that there is API change for callout_stop(). Let's look into the manual page:
...

The problem is that the whole paradigm of callouts is that it has three consequent states: not scheduled -> scheduled -> running -> not scheduled. The API and the manual page assume that, some comments in the code assume that, and looks like some contributors to the code also did. But this paradigm isn't true. A callout can be scheduled and running at the same time. And the mishandling of this state was the source of our TCP panics. Now let's get back to the documentation and see what should callout_stop() return if callout is scheduled and running at the same time. It should return 0 because callout is currently being serviced and can't be stopped. At the same time it should return 1, since it has successfully unscheduled a callout. So it must be 0 and 1 at the same time! :) Yep, which is impossible. The API is limited by wrong paradigm and we must select the most safe value between these two. Now, I assert that returning 1 is much less safe in this case. Because any properly designed user of the API will be ready to free resources that are now in use by running callout. This is exactly what happened with TCP. Any other user, use it async version or not, is also prone to panic if we return 1. I assert that returning 0 will make behaviour more safe, and won't break any users, since this is what a sane programmer would expect from API. To finalize, I would notice that this state of running+scheduled in practice is extremely rare, if we noticed it only now at above 50 Gbit/s. So, I'm strongly against limiting the behavior only to async version.

Thanks for taking time to give more context and I do agree with your conclusion.

@glebius

What about the return codes of callout_reset_xxx() . Will they be the same like callout_stop()?

In D7042#147489, @glebius wrote:

Kostik, I failed to see how can sleepqueue case be affected by the change. Can you please provide more details?

Sleepqueue code uses the td_slpcallout to manage time-bound sleeps. The callout is reset after the thread is added to the sleepqueue and the bound sleep was requested. When thread becomes runnable after the voluntary release of CPU, it verifies the conditions and checks whether the scheduled callout was executed or not. If the callout was not executed, it is stopped. If stop failed, then the sleepq_check_timeout() function relinguishes CPU to allow the callout to run.

The last action is done to prevent the situation where callout wakes up possible next sleep, causing too early sleep termination.

Now consider a situation when one (previous) callout is running, and another one (current) callout was successfully canceled. Since new callout was scheduled, this means that previous callout already dropped the thread lock, and is on the path to finish execution. New callout which could make us runnable after the relinquish, is cancelled. In the current KPI, this case, at least with CS_MIGRBLOCK flag set, is returning 1. Your proposal makes the case return 0. The result is that the thread goes into the off-CPU state (it is not runnable, but also not sleeping nor blocked on lock), and the only thing that can make it runnable, the new callout, was already cancelled.

There were several cases where this was already broken, most recent were rss changes to callouts. Although previous breakages were limited to the migrating cases, I fixed them in once in r234952, then fixed after rss once more in 296320. Now you intend to make the situation non-fixable at all.

P.S. To clarify next question, no, this cannot be fixed with the hps' magic bullet, that is the callout_init_mtx. The thread lock, which protects the state shared between sleepq code and sleepq_timeout, is not the lock in usual sense of the word, it is a dynamic set of locks, determined by the current runnability container managing the thread (i.e. runqueue, sleepqueue, turnstile etc lock, changing as thread migrates its running state).

P.S. To clarify next question, no, this cannot be fixed with the hps' magic bullet, that is the callout_init_mtx. The thread lock, which protects the state shared between sleepq code and sleepq_timeout, is not the lock in usual sense of the word, it is a dynamic set of locks, determined by the current runnability container managing the thread (i.e. runqueue, sleepqueue, turnstile etc lock, changing as thread migrates its running state).

If you allow callout_init_mtx() to accept spin-mutexes, like done in hps-head, and add a spin-mutex to "struct thread", which you initialize the callout with - yes it will work just fine w/o needing the return values of callout_reset() and callout_stop() !

--HPS

In D7042#148077, @hselasky wrote:

@glebius

What about the return codes of callout_reset_xxx() . Will they be the same like callout_stop()?

Nothing changes for them, since they don't call _callout_stop_safe internally. AFAIU, the described paradigm problem doesn't apply to scheduling functions.

Kostik, thanks for explanation. What I see is that the case that I now created for sleepqueue is very close to a case when callout was running on a different CPU and wasn't stoppable. And you did r296320 exactly to fix this case. Effectively CS_MIGRBLOCK doesn't block migration. It just flips the return value. So, if I use the same return value flip for my case, that will fix sleepqueue. And I'd suggest to rename CS_MIGRBLOCK to a more meaningful name and document it.

What about the return codes of callout_reset_xxx() . Will they be the same like callout_stop()?

Nothing changes for them, since they don't call _callout_stop_safe internally. AFAIU, the described paradigm problem doesn't apply to scheduling functions.

I think currently callout_stop() returns the same like callout_reset_xxx(), because callout_reset_xxx() can be viewed like:

callout_reset_xxx()
{
int retval;
retval = callout_stop();
restart_callout();
return (retval);
}

If you change the return values of callout_stop() you should also change the return values of callout_reset().

--HPS

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=210884#c11