Re: [Xenomai-core] [BUG] module_put called over non-root domain

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 6344 bytes --]

Philippe Gerum wrote:
> On Mon, 2007-05-21 at 18:21 +0200, Jan Kiszka wrote:
>>> This code has to work with an awful lot of runtime situations, including
>>> those raised by ptracing/GDB, and thread signaling in general, so test
>>> harnessing is key here.
>> Well, what about listing those scenarios and their requirements in a
>> comment if this is so critical?
> 
> Eh, because I'm likely lazy as you are? :o>
> 

Sigh, that's not a fair argument... ;->

>>  That may make /me feel better as I could
>> more easily go through them and check changes like those of today
>> against them.
>>
> 
> Those situations have been encountered when debugging, there is no
> limited set of golden rules to document, I'm afraid, this would have
> been far too easy. The entire signal handling wrt ptracing is a terrible
> mess - somebody must have endured a very bad Karma to implement this the
> way it is (long life utrace). The ChangeLog might be a good start for
> comments related to fixes in shadow.c/unmap and various head banging
> sessions I went through regarding this (2.4/2.6).
> 
>>>>  and I'm furthermore
>>>> not sure of the old code was handling SMP scenarios safely (What if the
>>>> thread to be unmapped was running on different CPU than xnshadow_unmap?
>>>> How to ensure test-atomicity then?).
>>>>
>>> This wakeup call is there to release emerging shadows waiting on their
>>> start barrier that get killed before start. Other regular cases imply
>>> that either the current thread is exiting - in which case this is a no
>>> brainer, or it has been sent a group kill signal, in which case it has
>>> to wake up anyway.
>> I didn't _removed_ cases where the wakeup takes place, I _added_ some
>> more my removing the test.
>>
> 
> In the light of what I have just written, you would only cause useless
> wakeups doing so.

My question was if the test for "useless" was really race-safe on SMP.
Then we could introduce it again in the APC handler. Otherwise, it might
be safer to live with some degree of uselessness.

> 
>>>> Well, this thing seems to work,
>>> It does, actually.
>>>
>>>>  but it doesn't leave me with a good
>>>> feeling. IMHO, there are far too many cleanup cases with all the
>>>> combinations of non-shadow termination, shadow self-termination,
>>>> termination on task exit, various SMP scenarios, etc.
>>>>  Moreover, quite a
>>>> lot of stuff is executed in atomic scheduling hooks (namely on cleanup).
>>>> At least for shadow threads, I don't think all of this is necessary.
>>>> Actually, deferring most of the uncritical cleanup work into Linux
>>>> context would make things more regular, easier to review, and likely
>>>> less disturbing for overall system latencies. I know that atomic
>>>> scheduling hooks are required to implement certain skins, but I don't
>>>> see why janitor work should be done there as well. And we also still
>>>> have that tiny uncleanness on shadow threads termination around TCB
>>>> release vs. hook execution.
>>> Talking about xnshadow_unmap, and as your patch illustrates, what you
>>> could postpone is basically the first part of the routine, which updates
>>> the reference counts. The rest _must_ run over the deletion hook in
>>> atomic context (already runs fine all Linux debug switches on). The
>> rpi_pop()? No chance to call it before entering the scheduler and then
>> the hooks?
> 
> What do you have in mind precisely?

Preemptibility whenever having all things in a single atomic region
doesn't buy us much. Even if rpi_pop per se might not be heavy, adding
it to an already complex path doesn't improve things.

So my spontaneous idea was, as you said the rest _must_ be atomic, if
that piece couldn't be actually moved into the task deletion service and
the task_exit Linux hook, ie. before taking the nklock again and do the
final reschedule. Just an example for what might be improvable - once we
dig here again.

> 
>>> latency hit is certainly not seen there; the real issues are left in the
>>> native and POSIX skin thread deletion routines (heap management and
>>> other housekeeping calls under nklock and so on).
>> That's surely true. Besides RPI manipulation, there is really no
>> problematic code left here latency-wise. Other hooks are trickier. But
>> my point is that we first need some infrastructure to provide an
>> alternative cleanup context before we can start moving things. My patch
>> today is a hack...err...workaround from this perspective.
>>
> 
> I don't see it this way. This code is very specific, in the sense it
> lays on the inter-domain boundary between Linux and Xenomai's primary
> mode for a critical operation like thread deletion, this is what make
> things a bit confusing. What's below and above this layer has to be as
> seamless as possible, this code in particular can't.

Well, we could move the memory release of the shadow thread in Linux
context e.g. (release on idle, a bit like RCU - uuuh). Then the
reference counter can find a new home as well. Maybe some more things,
dunno yet.

> 
>>>> As we are already at it: LO_MAX_REQUESTS is a hidden limitation that
>>>> may (theoretically) bite large setups when e.g. too many threads gets
>>>> killed from the "wrong" context. At least some overflow detection is
>>>> required here to warn the unfortunate user that he reached hard-coded
>>>> scalability limits.
>>> Yes.
>> CONFIG_XENO_OPT_DEBUG[_NUCLEUS] or unconditionally (error return code)?
>>
> 
> Making it depend on the debug switch looks good. People who are serious
> about making production software do want to activate this switch once in
> a while while developing it, really.
> 

--- xenomai.orig/ksrc/nucleus/shadow.c
+++ xenomai/ksrc/nucleus/shadow.c
@@ -890,11 +890,19 @@ static void schedule_linux_call(int type
 		);

 	splhigh(s);
+
 	reqnum = rq->in;
+
+	if (XENO_DEBUG(NUCLEUS) &&
+	    ((reqnum + 1) & (LO_MAX_REQUESTS - 1)) == rq->out)
+		xnpod_fatal("lostage queue overflow on CPU %d! "
+			    "Increase LO_MAX_REQUESTS", cpuid);
+
 	rq->req[reqnum].type = type;
 	rq->req[reqnum].task = p;
 	rq->req[reqnum].arg = arg;
 	rq->in = (reqnum + 1) & (LO_MAX_REQUESTS - 1);
+
 	splexit(s);

 	rthal_apc_schedule(lostage_apc);



Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]