[Xenomai-core] [PATCH] Flush xnfree backlog after thread deletion in root context

[Xenomai-core] [PATCH] Flush xnfree backlog after thread deletion in root context

[Jan Kiszka]

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

Index: xenomai-2.4.x/ksrc/nucleus/pod.c
===================================================================
--- xenomai-2.4.x/ksrc/nucleus/pod.c	(Revision 3764)
+++ xenomai-2.4.x/ksrc/nucleus/pod.c	(Arbeitskopie)
@@ -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:

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

Re: [Xenomai-core] [PATCH] Flush xnfree backlog after thread deletion in root context

[Gilles Chanteperdrix]

On Tue, May 13, 2008 at 10:26 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>  @@ -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)

-- 
 Gilles

Re: [Xenomai-core] [PATCH] Flush xnfree backlog after thread deletion in root context

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> On Tue, May 13, 2008 at 10:26 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>  @@ -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.

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

> 
> 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

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

Re: [Xenomai-core] [PATCH] Flush xnfree backlog after thread deletion in root context

[Gilles Chanteperdrix]

On Tue, May 13, 2008 at 11:10 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> Gilles Chanteperdrix wrote:
>  > On Tue, May 13, 2008 at 10:26 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>  >>  @@ -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.
>
>  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.

-- 
 Gilles

Re: [Xenomai-core] [PATCH] Flush xnfree backlog after thread deletion in root context

[Philippe Gerum]

Gilles Chanteperdrix wrote:
> On Tue, May 13, 2008 at 11:10 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> Gilles Chanteperdrix wrote:
>>  > On Tue, May 13, 2008 at 10:26 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>  >>  @@ -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.
>>
>>  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
> issue further.
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH] Flush xnfree backlog after thread deletion in root context

[Jan Kiszka]

Philippe Gerum wrote:
> Gilles Chanteperdrix wrote:
>> On Tue, May 13, 2008 at 11:10 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>> Gilles Chanteperdrix wrote:
>>>  > On Tue, May 13, 2008 at 10:26 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>>  >>  @@ -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.
>>>
>>>  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.

> 
>  The XNARCH_WANT_UNLOCKED_CTXSW complicated the
>> 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

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

Re: [Xenomai-core] [PATCH] Flush xnfree backlog after thread deletion in root context

[Gilles Chanteperdrix]

On Tue, May 13, 2008 at 11:32 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> Philippe Gerum wrote:
>  > Gilles Chanteperdrix wrote:
>  >> On Tue, May 13, 2008 at 11:10 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>  >>> Gilles Chanteperdrix wrote:
>  >>>  > On Tue, May 13, 2008 at 10:26 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>  >>>  >>  @@ -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.
>  >>>
>  >>>  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.

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.

>
>
>  >
>  >  The XNARCH_WANT_UNLOCKED_CTXSW complicated the
>  >> 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
>  imply !CONFIG_CMP?

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

-- 
 Gilles

Re: [Xenomai-core] [PATCH] Flush xnfree backlog after thread deletion in root context

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> On Tue, May 13, 2008 at 11:32 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> Philippe Gerum wrote:
>>  > Gilles Chanteperdrix wrote:
>>  >> On Tue, May 13, 2008 at 11:10 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>  >>> Gilles Chanteperdrix wrote:
>>  >>>  > On Tue, May 13, 2008 at 10:26 AM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>  >>>  >>  @@ -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.
>>  >>>
>>  >>>  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.
> 
> 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:

Index: xenomai/ksrc/nucleus/pod.c
===================================================================
--- xenomai/ksrc/nucleus/pod.c	(Revision 3770)
+++ xenomai/ksrc/nucleus/pod.c	(Arbeitskopie)
@@ -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?

> 
>>
>>  >
>>  >  The XNARCH_WANT_UNLOCKED_CTXSW complicated the
>>  >> 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
>>  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

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

Re: [Xenomai-core] [PATCH] Flush xnfree backlog after thread deletion in root context

[Gilles Chanteperdrix]

On Tue, May 13, 2008 at 12:46 PM, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> Gilles Chanteperdrix wrote:
>  > 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.

-- 
 Gilles