[Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 315 bytes --]

Hi,

given the existing doc of rt_task_suspend about allowed environments and
also when thinking about the logic behind it, I see no reason why
xnpod_unblockable_p() is also applied to task != NULL. This patch
confines the check to task == NULL and also tries to clarify the error
code documentation.

Jan

[-- Attachment #1.2: suspend-foreign-task-from-any-context.patch --]
[-- Type: text/plain, Size: 1066 bytes --]

Index: ksrc/skins/native/task.c
===================================================================
--- ksrc/skins/native/task.c	(revision 1323)
+++ ksrc/skins/native/task.c	(working copy)
@@ -391,9 +391,9 @@ int rt_task_start(RT_TASK *task, void (*
  *
  * - -EINVAL is returned if @a task is not a task descriptor.
  *
- * - -EPERM is returned if @a task is NULL but not called from a task
- * context, or this service was called from a context which cannot
- * sleep (e.g. interrupt, non-realtime or scheduler locked).
+ * - -EPERM is returned if @a task is NULL and this service was called
+ * from a context which cannot sleep (e.g. interrupt, non-realtime or
+ * scheduler locked).
  *
  * - -EIDRM is returned if @a task is a deleted task descriptor.
  *
@@ -430,9 +430,7 @@ int rt_task_suspend(RT_TASK *task)
 	if (!task) {
 		err = xeno_handle_error(task, XENO_TASK_MAGIC, RT_TASK);
 		goto unlock_and_exit;
-	}
-
-	if (xnpod_unblockable_p()) {
+	} else if (xnpod_unblockable_p()) {
 		err = -EPERM;
 		goto unlock_and_exit;
 	}

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

Re: [Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Dmitry Adamushko]

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

Hi,


given the existing doc of rt_task_suspend about allowed environments and
> also when thinking about the logic behind it, I see no reason why
> xnpod_unblockable_p() is also applied to task != NULL.



XNLOCK is set? anyway, I don't see how the patch makes current logic
different...

in both cases xnpod_unblockable_p() has a chance to be executed only when
task != NULL (thanks to goto). and -EPERM is returned when task != NULL &&
xnpod_unblockable_p() == true.


        if (!task) {
>                 err = xeno_handle_error(task, XENO_TASK_MAGIC, RT_TASK);
>                 goto unlock_and_exit;
> -       }
> -
> -       if (xnpod_unblockable_p()) {
> +       } else if (xnpod_unblockable_p()) {
>                 err = -EPERM;
>                 goto unlock_and_exit;
>         }
>
>
>


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 1752 bytes --]

Re: [Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 1021 bytes --]

Dmitry Adamushko wrote:
> Hi,
> 
> 
> given the existing doc of rt_task_suspend about allowed environments and
>> also when thinking about the logic behind it, I see no reason why
>> xnpod_unblockable_p() is also applied to task != NULL.
> 
> 
> 
> XNLOCK is set? anyway, I don't see how the patch makes current logic
> different...
> 
> in both cases xnpod_unblockable_p() has a chance to be executed only when
> task != NULL (thanks to goto). and -EPERM is returned when task != NULL &&
> xnpod_unblockable_p() == true.
> 
> 
>        if (!task) {
>>                 err = xeno_handle_error(task, XENO_TASK_MAGIC, RT_TASK);
>>                 goto unlock_and_exit;
>> -       }
>> -
>> -       if (xnpod_unblockable_p()) {
>> +       } else if (xnpod_unblockable_p()) {
>>                 err = -EPERM;
>>                 goto unlock_and_exit;
>>         }
>>
>>
>>
> 
> 

Oh, my dear. I should stop hacking patches while talking to colleagues.
Does this one make more sense?

Jan

[-- Attachment #1.2: suspend-foreign-task-from-any-context-v2.patch --]
[-- Type: text/plain, Size: 977 bytes --]

Index: ksrc/skins/native/task.c
===================================================================
--- ksrc/skins/native/task.c	(revision 1323)
+++ ksrc/skins/native/task.c	(working copy)
@@ -391,9 +391,9 @@ int rt_task_start(RT_TASK *task, void (*
  *
  * - -EINVAL is returned if @a task is not a task descriptor.
  *
- * - -EPERM is returned if @a task is NULL but not called from a task
- * context, or this service was called from a context which cannot
- * sleep (e.g. interrupt, non-realtime or scheduler locked).
+ * - -EPERM is returned if @a task is NULL and this service was called
+ * from a context which cannot sleep (e.g. interrupt, non-realtime or
+ * scheduler locked).
  *
  * - -EIDRM is returned if @a task is a deleted task descriptor.
  *
@@ -432,7 +432,7 @@ int rt_task_suspend(RT_TASK *task)
 		goto unlock_and_exit;
 	}
 
-	if (xnpod_unblockable_p()) {
+	if (xnpod_locked_p()) {
 		err = -EPERM;
 		goto unlock_and_exit;
 	}

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

Re: [Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 >  
 > -	if (xnpod_unblockable_p()) {
 > +	if (xnpod_locked_p()) {

Why not if (task == xeno_current_task() && xnpod_unblockable_p()) ?

-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Jan Kiszka]

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

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>  >  
>  > -	if (xnpod_unblockable_p()) {
>  > +	if (xnpod_locked_p()) {
> 
> Why not if (task == xeno_current_task() && xnpod_unblockable_p()) ?
> 

Is it legal to suspend a foreign task when XNLOCK is set? If yes, I
would vote for your version as well.

Jan


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

Re: [Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Dmitry Adamushko]

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

On 12/07/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> Gilles Chanteperdrix wrote:
> > Jan Kiszka wrote:
> >  >
> >  > -  if (xnpod_unblockable_p()) {
> >  > +  if (xnpod_locked_p()) {
> >
> > Why not if (task == xeno_current_task() && xnpod_unblockable_p()) ?
> >
>
> Is it legal to suspend a foreign task when XNLOCK is set? If yes, I
> would vote for your version as well.



You mean if the current task holds XNLOCK and wanna suspend another task ->
which, in turn, seemingly implies some "scheduling" action?

The only (probably) idea behind XNLOCK is to avoid rescheduling for as long
as the current task (on this scheduler) locks the scheduler.

So it disallows _rescheduling_ but any actions like manipulating e.g. the
list of active tasks should still remain allowed. One may recall
preempt_enable/disable() in Linux.

(ummm... actually, xnpod_thread_mode() doesn't seem to cause re-scheduling
in case of removing the lock as it's done by xnpod_unlock_sched())

Moreover, we have per-cpu schedulers so that a task that locks the scheduler
on cpu0 may still suspend (and cause re-scheduling) of any really running
task on another cpu (or it likely should be so :)

So I don't see any problem here. Sure I don't know all the implementation
details and can be just driven by my own misunderstanding.



Jan
>
>
>
>

-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 2001 bytes --]

Re: [Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Dmitry Adamushko]

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

  Sure I don't know all the implementation details and can be just driven by
> my own misunderstanding.
>

Yep,  just a common excuse to keep my face out of the soup in case I'm
completely wrong.
Normally means (by Freud?) a person doesn't want to take responsibility
err... for his own words. IOW, if I'm right let's my carma to be updated, if
no - weeell you know I'm still leeeaarning but see I can solve this quadr.
math equotion !

err.. hope the majority of people subscribed to this mailing list have got
good spam filters :)


Jan
> >
> >
> >
-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 1160 bytes --]

Re: [Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Dmitry Adamushko]

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

--- task.c      2006-05-19 18:25:19.000000000 +0200
+++ task-my.c   2006-07-12 15:40:16.549170000 +0200
@@ -426,7 +426,8 @@ int rt_task_suspend(RT_TASK *task)
         goto unlock_and_exit;
     }


-    if (xnpod_unblockable_p()) {
+    /* we are about to suspend a task, let's check wehther it may sleep */
+    if (xnthread_test_flags(task, XNLOCK|XNROOT)) {
         err = -EPERM;
         goto unlock_and_exit;
     }



See the difference? :) it was slightly broken indeed, meaning it just bailed
out all the calls from the interrupt context which is wrong according to
spec.

maybe something like xnthread_task_unblockable/blockable(task)  would be
also of avail.


Oh, my dear. I should stop hacking patches while talking to colleagues.
> Does this one make more sense?
>

maybe it's better that the other way around? I mean, writting good code and
saying nonsense to your collegues... :o>


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 3152 bytes --]

Re: [Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 1286 bytes --]

Dmitry Adamushko wrote:
> --- task.c      2006-05-19 18:25:19.000000000 +0200
> +++ task-my.c   2006-07-12 15:40:16.549170000 +0200
> @@ -426,7 +426,8 @@ int rt_task_suspend(RT_TASK *task)
>         goto unlock_and_exit;
>     }
> 
> 
> -    if (xnpod_unblockable_p()) {
> +    /* we are about to suspend a task, let's check wehther it may sleep */
> +    if (xnthread_test_flags(task, XNLOCK|XNROOT)) {
>         err = -EPERM;
>         goto unlock_and_exit;
>     }
> 
> 
> 
> See the difference? :) it was slightly broken indeed, meaning it just
> bailed
> out all the calls from the interrupt context which is wrong according to
> spec.

Fine, now I achieved what I always wanted: someone really thought about
it! :)

I tried to bundle your approach with appropriate documentation update
and a minor fix in the attached patch. Credits go to you then.

> 
> maybe something like xnthread_task_unblockable/blockable(task)  would be
> also of avail.
> 
> 
> Oh, my dear. I should stop hacking patches while talking to colleagues.
>> Does this one make more sense?
>>
> 
> maybe it's better that the other way around? I mean, writting good code and
> saying nonsense to your collegues... :o>

Who said I was talking witted stuff to them?

Jan

[-- Attachment #1.2: suspend-foreign-task-from-any-context-v3.patch --]
[-- Type: text/plain, Size: 1069 bytes --]

Index: ksrc/skins/native/task.c
===================================================================
--- ksrc/skins/native/task.c	(revision 1323)
+++ ksrc/skins/native/task.c	(working copy)
@@ -391,9 +391,8 @@ int rt_task_start(RT_TASK *task, void (*
  *
  * - -EINVAL is returned if @a task is not a task descriptor.
  *
- * - -EPERM is returned if @a task is NULL but not called from a task
- * context, or this service was called from a context which cannot
- * sleep (e.g. interrupt, non-realtime or scheduler locked).
+ * - -EPERM is returned if the addressed @a task is not allowed to sleep
+ * (e.g. in interrupt context, non-realtime task, or scheduler locked).
  *
  * - -EIDRM is returned if @a task is a deleted task descriptor.
  *
@@ -432,7 +431,8 @@ int rt_task_suspend(RT_TASK *task)
 		goto unlock_and_exit;
 	}
 
-	if (xnpod_unblockable_p()) {
+	/* We are about to suspend a task, let's check whether it may sleep */
+	if (xnthread_test_flags(&task->thread_base, XNLOCK|XNROOT)) {
 		err = -EPERM;
 		goto unlock_and_exit;
 	}

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

Re: [Xenomai-core] [PATCH] relax context check for rt_task_suspend

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 > Dmitry Adamushko wrote:
 > > --- task.c      2006-05-19 18:25:19.000000000 +0200
 > > +++ task-my.c   2006-07-12 15:40:16.549170000 +0200
 > > @@ -426,7 +426,8 @@ int rt_task_suspend(RT_TASK *task)
 > >         goto unlock_and_exit;
 > >     }
 > > 
 > > 
 > > -    if (xnpod_unblockable_p()) {
 > > +    /* we are about to suspend a task, let's check wehther it may sleep */
 > > +    if (xnthread_test_flags(task, XNLOCK|XNROOT)) {
 > >         err = -EPERM;
 > >         goto unlock_and_exit;
 > >     }
 > > 
 > > 
 > > 
 > > See the difference? :) it was slightly broken indeed, meaning it just
 > > bailed
 > > out all the calls from the interrupt context which is wrong according to
 > > spec.
 > 
 > Fine, now I achieved what I always wanted: someone really thought about
 > it! :)
 > 
 > I tried to bundle your approach with appropriate documentation update
 > and a minor fix in the attached patch. Credits go to you then.

Patch applied.

-- 


					    Gilles Chanteperdrix.