Currently TCP uses a rather round-about way to accomplish a callout-drain-async. This
adds the functionality that does it more direct supported by the callout-system.
Details
Details
First step is to have some review and test in parallel with my callout_test framework. I will
need to build a case where I can force this set of circumstances and verify the callbacks happen
like they should.
Note I will leave this up for quite some time since we need to have some good validation of this.
After its in we then will need to migrate TCP (and possibly other systems) to using it.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| kern/kern_timeout.c | ||
|---|---|---|
| 139 ↗ | (On Diff #9912) | How about typedef'ing this function? |
| 1259 ↗ | (On Diff #9912) | How does the caller know if the drain function is supposed to be called or not? |
| 1377 ↗ | (On Diff #9912) | These three lines should probably be moved under the lock(cc); |
| sys/callout.h | ||
| 125 ↗ | (On Diff #9912) | Minor typo here )) |
Comment Actions
After thinking about it, we need to move the clear of active down and
only call drain in the duplicate stop case if the caller did not
clear active (which they should have when the callout really completed).
| kern/kern_timeout.c | ||
|---|---|---|
| 1250 ↗ | (On Diff #9915) | stupid style nit: blank line. |
Comment Actions
Ok lets make it so that *always* if you call callout_async_drain() on a stopped/completed timer
then kassert and die. You need to always assure the timer is running before you call
async_drain!
Comment Actions
This update is now tested. However I still need to an update to
the manual. Kib has a proposal to change where I KASSERT(drain == NULL) and
I think we should do that i.e. make that case return -1.
That way when you call callout_stop (in any form)
1 -- it was stopped
-1 -- it was not running, i.e. it was stopped already or has already expired and was processed
0 -- can't stop it.
For async_drain then 0 means the drain will be called, the others no.
Comment Actions
Ok this adds the small section into the manual page. With
that we are about ready :-)
Comment Actions
Ok this will update it so we get three return codes out
of callout_stop and friends
- 0 -- we could not stop it (and maybe a drain will come if you do the async style)
- -1 -- It was not running or has already expired (you were not tracking it silly)
- 1 -- It was stopped
This makes it so the callout_stop and friends return codes are reliable to tell us something. In
particular it removes the KASSERT in the -1 return path so that a caller of the async_drain
can count how many 0 returns come back and based on that know how many drain() calls
will happen before it can say purge memory of the reference...
| share/man/man9/timeout.9 | ||
|---|---|---|
| 266 ↗ | (On Diff #10067) | I think this sentence is incorrect? callout_async_drain() is a non-blocking function and can be called locked. |
| sys/sys/callout.h | ||
| 125 ↗ | (On Diff #10067) | Should callout_async_drain() take an additional "void *" argument, which should be passed to the callback function? |
| kern/kern_timeout.c | ||
|---|---|---|
| 1259 ↗ | (On Diff #9919) | Possibly we should take this to just always asserting that drain is NULL. |
| sys/sys/callout.h | ||
| 125 ↗ | (On Diff #10067) | Why? What purpose does it serve over the original argument? For the use case most likely to happen, TCP, its the argument which What other use are you thinking you will need it for? |
| kern/kern_timeout.c | ||
|---|---|---|
| 1259 ↗ | (On Diff #9919) | I think this issue was fixed by the new negative return value. |
| sys/sys/callout.h | ||
| 125 ↗ | (On Diff #10067) | Hi, Say you have a child parent structure relationship. The refcount is on the parent, and callout argument is in the child. Then when destroying such an object, you don't want to have the c_arg passed to the drain destructor, but rather a pointer to the parent. --HPS |
Comment Actions
"RETURN VALUES" man page section still states that "The callout_stop() and callout_drain() functions return non-zero if the callout was still pending when it was called or zero otherwise.". This should also be updated.
Also, I wonder if it would be better to make api consistent in terms of return values.
callout_stop()/callout_drain() now returns -1 for non-scheduled callout, but callout_reset() still returns zero for that case.