On 05/17/2011 09:56 AM, John Baldwin wrote:
On Tuesday, May 17, 2011 12:20:40 pm Max Laier wrote:
On 05/17/2011 05:16 AM, John Baldwin wrote:
...
Index: kern/kern_switch.c
===================================================================
--- kern/kern_switch.c (revision 221536)
+++ kern/kern_switch.c (working copy)
@@ -192,15 +192,22 @@
critical_exit(void)
{
struct thread *td;
- int flags;
+ int flags, owepreempt;
td = curthread;
KASSERT(td->td_critnest != 0,
("critical_exit: td_critnest == 0"));
if (td->td_critnest == 1) {
+ owepreempt = td->td_owepreempt;
+ td->td_owepreempt = 0;
+ /*
+ * XXX: Should move compiler_memory_barrier() from
+ * rmlock to a header.
+ */
XXX: If we get an interrupt at this point and td_owepreempt was zero,
the new interrupt will re-set it, because td_critnest is still non-zero.
So we still end up with a thread that is leaking an owepreempt *and*
lose a preemption.
I don't see how this can still leak owepreempt. The nested interrupt should
do nothing (except for possibly set owepreempt) until td_critnest is 0.
Exactly. The interrupt sets owepreempt and after we return here, we set
td_critnest to 0 and exit without clearing owepreempt. Hence we leak
the owepreempt.
However, we can certainly lose preemptions.
I wonder if we can abuse the high bit of td_critnest for the owepreempt flag
so it is all stored in one cookie. We only set owepreempt while holding
thread_lock() (so interrupts are disabled), so I think we would be ok and not
need atomic ops.
Hmm, actually, the top-half code would have to use atomic ops. Nuts. Let me
think some more.
I think these two really belong into one single variable. Setting the
owepreempt flag can be a normal RMW. Increasing and decreasing critnest
must be atomic (otherwise we could lose the flag) and dropping the final
reference would work like this:
if ((curthread->td_critnest & TD_CRITNEST_MASK) == 1) {
unsigned int owe;
owe = atomic_readandclear(&curthread->td_critnest);
if (owe & TD_OWEPREEMPT_FLAG) {
/* do the switch */
}
That should do it ... I can put that into a patch, if we agree that's
the right thing to do.
Thanks,
Max
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
