Flush xnfree backlog after thread deletion in root context

PATCH In review Jan Kiszka 3 participants 9 messages May 13, 2008

Share & tools

Jan Kiszka

author

2008-05-13 08:26 UTC #0 lore raw

PATCH 1/1 Flush xnfree backlog after thread deletion in root context

The lazy xnfree mechanism turned out to be too lazy: Just run some testsuite program and what /proc/xenomai/heap - it will not reach the level it has on freshly booted box.

The reason is that TCBs of threads that are released over the ROOT thread will only get purged after the next schedule-to-ROOT. And if there is no more Xenomai thread scheduling in the system, this won't happen. It can even leave your system useless if you ran into out-of-heap with allocating TCBs, and the remaining heap is now too small to start at least one thread again (happened to customer). Patch below fixes this by adding an xnfreesync to the related code path.

However, I wonder if we shouldn't better run all those deferred releases in Linux context with only O(1) critical path lengths. Why risking to pile up a large backlog of TCBs when releasing a lot of threads in a row? That may degrade system latency. What about pushing this into some VIRQ over the Linux domain, releasing the nklock once after each iteration?

Jan

ksrc/nucleus/pod.c +3 -0

Index: xenomai-2.4.x/ksrc/nucleus/pod.c
===================================================================
@@ -1236,6 +1236,9 @@ void xnpod_delete_thread(xnthread_t *thr
 		xnthread_cleanup_tcb(thread);
 
 		xnarch_finalize_no_switch(xnthread_archtcb(thread));
+
+		if (xnthread_test_state(sched->runthread, XNROOT))
+			xnfreesync();
 	}
 
       unlock_and_exit:

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Gilles Chanteperdrix

2008-05-13 08:56 UTC #1 lore raw

Quoted 8 lines · Jan Kiszka

@@ -1236,6 +1236,9 @@ void xnpod_delete_thread(xnthread_t *thr

                xnthread_cleanup_tcb(thread);

                xnarch_finalize_no_switch(xnthread_archtcb(thread));
 +
 +               if (xnthread_test_state(sched->runthread, XNROOT))
 +                       xnfreesync();
        }

No, this does not look good. The point of deferring TCB freeing is that the TCB will be accessed shortly after it is freed.

IMHO, the right solution is to add a call to xnpod_schedule or even directly xnfreesyng in the right place (in skins code, after all threads have been freed)

Signature · 1 line

 Gilles

Jan Kiszka

author

2008-05-13 09:10 UTC #2 lore raw

Quoted 4 lines · Gilles Chanteperdrix

Quoted 3 lines · Jan Kiszka
@@ -1236,6 +1236,9 @@ void xnpod_delete_thread(xnthread_t *thr
                xnthread_cleanup_tcb(thread);

Quoted 8 lines

Quoted 5 lines
                xnarch_finalize_no_switch(xnthread_archtcb(thread));
 +
 +               if (xnthread_test_state(sched->runthread, XNROOT))
 +                       xnfreesync();
        }
No, this does not look good. The point of deferring TCB freeing is that the TCB will be accessed shortly after it is freed.

By whom in this case? The thread was not active anymore. IIRC, the use-after-release issue was related to self-deletions.

Quoted 4 lines

IMHO, the right solution is to add a call to xnpod_schedule or even directly xnfreesyng in the right place (in skins code, after all threads have been freed)

Well, as I said, we should rather move all these syncs out of critical sections. So better avoid xnpod_schedule.

I need to think about this a bit more, I'm afraid. It is safe to delete the TCB once the terminating thread is scheduled away and its successor of about to leave (or left) xnpod_schedule, right?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Gilles Chanteperdrix

2008-05-13 09:16 UTC #3 lore raw

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix
Quoted 8 lines · Jan Kiszka
@@ -1236,6 +1236,9 @@ void xnpod_delete_thread(xnthread_t *thr
                xnthread_cleanup_tcb(thread);

                xnarch_finalize_no_switch(xnthread_archtcb(thread));
 +
 +               if (xnthread_test_state(sched->runthread, XNROOT))
 +                       xnfreesync();
        }

Quoted 5 lines

Quoted 2 lines
No, this does not look good. The point of deferring TCB freeing is that the TCB will be accessed shortly after it is freed.
By whom in this case? The thread was not active anymore. IIRC, the use-after-release issue was related to self-deletions.

I do not remember the issue precisely, I know that it was related to thread deletion hooks. The XNARCH_WANT_UNLOCKED_CTXSW complicated the issue further.

Signature · 1 line

 Gilles

Philippe Gerum

2008-05-13 09:29 UTC #4 lore raw

Quoted 12 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka
Quoted 10 lines · Gilles Chanteperdrix
Quoted 8 lines · Jan Kiszka
@@ -1236,6 +1236,9 @@ void xnpod_delete_thread(xnthread_t *thr
                xnthread_cleanup_tcb(thread);

                xnarch_finalize_no_switch(xnthread_archtcb(thread));
 +
 +               if (xnthread_test_state(sched->runthread, XNROOT))
 +                       xnfreesync();
        }

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
No, this does not look good. The point of deferring TCB freeing is that the TCB will be accessed shortly after it is freed.
By whom in this case? The thread was not active anymore. IIRC, the use-after-release issue was related to self-deletions.
I do not remember the issue precisely, I know that it was related to thread deletion hooks.

The point is that we have to run thread deletion hooks on behalf of the outgoing thread context; this is a strong requirement.

The XNARCH_WANT_UNLOCKED_CTXSW complicated the

Quoted 2 lines

issue further.

Signature · 1 line

Philippe.

Jan Kiszka

author

2008-05-13 09:32 UTC #5 lore raw

Quoted 16 lines · Philippe Gerum

Quoted 15 lines · Gilles Chanteperdrix
Quoted 14 lines · Jan Kiszka
Quoted 12 lines · Gilles Chanteperdrix
Quoted 8 lines · Jan Kiszka
@@ -1236,6 +1236,9 @@ void xnpod_delete_thread(xnthread_t *thr
                xnthread_cleanup_tcb(thread);

                xnarch_finalize_no_switch(xnthread_archtcb(thread));
 +
 +               if (xnthread_test_state(sched->runthread, XNROOT))
 +                       xnfreesync();
        }
No, this does not look good. The point of deferring TCB freeing is that the TCB will be accessed shortly after it is freed.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
By whom in this case? The thread was not active anymore. IIRC, the use-after-release issue was related to self-deletions.
I do not remember the issue precisely, I know that it was related to thread deletion hooks.
The point is that we have to run thread deletion hooks on behalf of the outgoing thread context; this is a strong requirement.

Yes, I remember.

Quoted 3 lines

The XNARCH_WANT_UNLOCKED_CTXSW complicated the
Quoted 1 line
issue further.

I now wonder when in this unlocked case do we schedule the tcb for deletion - should be _before_ the switch - and what then prevents that the tcb is flushed by some other CPU that happen to run xnfreesync while we are unlocked during that switch. Or does XNARCH_WANT_UNLOCKED_CTXSW imply !CONFIG_CMP?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Gilles Chanteperdrix

2008-05-13 09:38 UTC #6 lore raw

Quoted 19 lines · Jan Kiszka

Quoted 18 lines · Philippe Gerum
Quoted 17 lines · Gilles Chanteperdrix
Quoted 16 lines · Jan Kiszka
Quoted 4 lines · Gilles Chanteperdrix
Quoted 3 lines · Jan Kiszka
@@ -1236,6 +1236,9 @@ void xnpod_delete_thread(xnthread_t *thr
                xnthread_cleanup_tcb(thread);
Quoted 8 lines
Quoted 5 lines
                xnarch_finalize_no_switch(xnthread_archtcb(thread));
 +
 +               if (xnthread_test_state(sched->runthread, XNROOT))
 +                       xnfreesync();
        }
No, this does not look good. The point of deferring TCB freeing is that the TCB will be accessed shortly after it is freed.
By whom in this case? The thread was not active anymore. IIRC, the use-after-release issue was related to self-deletions.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
I do not remember the issue precisely, I know that it was related to thread deletion hooks.
The point is that we have to run thread deletion hooks on behalf of the outgoing thread context; this is a strong requirement.
Yes, I remember.

Ok. Now I remember. The point is that the tcb is scheduled for deletion by thread deletion hooks, and accessed shortly after by xnthread_cleanup_tcb and xnarch_finalize_no_switch. So basically, your initial patch looks Ok. However, you should add the same call to __xnpod_finalize_zombie.

Quoted 10 lines

Quoted 5 lines
The XNARCH_WANT_UNLOCKED_CTXSW complicated the
Quoted 1 line
issue further.
I now wonder when in this unlocked case do we schedule the tcb for deletion - should be _before_ the switch - and what then prevents that the tcb is flushed by some other CPU that happen to run xnfreesync while we are unlocked during that switch.

The XNSWLOCK bit.

Or does XNARCH_WANT_UNLOCKED_CTXSW

Quoted 1 line

imply !CONFIG_CMP?

No, it is supposed to work. The sched->zombie points to this zombie, which is finalized in xnpod_finish_unlocked_switch

Signature · 1 line

 Gilles

Jan Kiszka

author

2008-05-13 10:46 UTC #7 lore raw

Quoted 26 lines · Gilles Chanteperdrix

Quoted 25 lines · Jan Kiszka
Quoted 23 lines · Philippe Gerum
Quoted 12 lines · Gilles Chanteperdrix
Quoted 11 lines · Jan Kiszka
Quoted 10 lines · Gilles Chanteperdrix
Quoted 8 lines · Jan Kiszka
@@ -1236,6 +1236,9 @@ void xnpod_delete_thread(xnthread_t *thr
                xnthread_cleanup_tcb(thread);

                xnarch_finalize_no_switch(xnthread_archtcb(thread));
 +
 +               if (xnthread_test_state(sched->runthread, XNROOT))
 +                       xnfreesync();
        }
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
No, this does not look good. The point of deferring TCB freeing is that the TCB will be accessed shortly after it is freed.
By whom in this case? The thread was not active anymore. IIRC, the use-after-release issue was related to self-deletions.
I do not remember the issue precisely, I know that it was related to thread deletion hooks.
The point is that we have to run thread deletion hooks on behalf of the outgoing thread context; this is a strong requirement.

Quoted 7 lines

Quoted 1 line
Yes, I remember.
Ok. Now I remember. The point is that the tcb is scheduled for deletion by thread deletion hooks, and accessed shortly after by xnthread_cleanup_tcb and xnarch_finalize_no_switch. So basically, your initial patch looks Ok. However, you should add the same call to __xnpod_finalize_zombie.

That's for trunk only:

ksrc/nucleus/pod.c +6 -0

Index: xenomai/ksrc/nucleus/pod.c
===================================================================
@@ -583,6 +583,9 @@ void __xnpod_finalize_zombie(xnsched_t *
 
 	xnarch_finalize_no_switch(xnthread_archtcb(thread));
 
+	if (xnthread_test_state(sched->runthread, XNROOT))
+		xnfreesync();
+
 	sched->zombie = NULL;
 }

@@ -1231,6 +1234,9 @@ void xnpod_delete_thread(xnthread_t *thr
 		xnthread_cleanup_tcb(thread);
 
 		xnarch_finalize_no_switch(xnthread_archtcb(thread));
+
+		if (xnthread_test_state(sched->runthread, XNROOT))
+			xnfreesync();
 	}
 
       unlock_and_exit:

Everyone fine when I apply both patches?

Quoted 11 lines

Quoted 10 lines
Quoted 5 lines
The XNARCH_WANT_UNLOCKED_CTXSW complicated the
Quoted 1 line
issue further.
I now wonder when in this unlocked case do we schedule the tcb for deletion - should be _before_ the switch - and what then prevents that the tcb is flushed by some other CPU that happen to run xnfreesync while we are unlocked during that switch.

Quoted 7 lines

The XNSWLOCK bit.
Or does XNARCH_WANT_UNLOCKED_CTXSW
Quoted 1 line
imply !CONFIG_CMP?
No, it is supposed to work. The sched->zombie points to this zombie, which is finalized in xnpod_finish_unlocked_switch

OK. However, I would still prefer to get xnfreesync out of the critical paths. Is there anything which speaks against my VIRQ idea (except that it still has to be written :) )?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Gilles Chanteperdrix

2008-05-13 12:33 UTC #8 lore raw

Quoted 8 lines · Jan Kiszka

Quoted 2 lines · Gilles Chanteperdrix
No, it is supposed to work. The sched->zombie points to this zombie, which is finalized in xnpod_finish_unlocked_switch
OK. However, I would still prefer to get xnfreesync out of the critical paths. Is there anything which speaks against my VIRQ idea (except that it still has to be written :) )?

Ok. Even though I am not sure how it works now, getting thread deletion to work with XNARCH_WANT_UNLOCKED_CTXSW was by far the hardest part of the work. I am not sure you can have the same control over what happens with a VIRQ.

Signature · 1 line

 Gilles