Skip to contentSkip to conversation

relax context check for rt_task_suspend

PATCH In review Jan Kiszka 3 participants 10 messages Jul 12, 2006

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/1 relax context check for rt_task_suspend

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

skins/native/task.c +4 -6
Index: ksrc/skins/native/task.c
===================================================================
@@ -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;
 	}
#1 lore raw

Hi,

given the existing doc of rt_task_suspend about allowed environments and

Quoted 2 lines

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) {
Quoted 12 lines
                err = xeno_handle_error(task, XENO_TASK_MAGIC, RT_TASK);
                goto unlock_and_exit;
skins/native/task.c +1 -3
-       }
-
-       if (xnpod_unblockable_p()) {
+       } else if (xnpod_unblockable_p()) {
                err = -EPERM;
                goto unlock_and_exit;
        }
Signature · 2 lines
Best regards,
Dmitry Adamushko
author
#2 lore raw
Quoted 12 lines · Dmitry Adamushko

Hi,

given the existing doc of rt_task_suspend about allowed environments and

Quoted 2 lines

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

Quoted 20 lines

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) {
Quoted 12 lines
                err = xeno_handle_error(task, XENO_TASK_MAGIC, RT_TASK);
                goto unlock_and_exit;
(quoted diff) +1 -3
-       }
-
-       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

skins/native/task.c +4 -4
Index: ksrc/skins/native/task.c
===================================================================
@@ -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;
 	}
#3 lore raw

skins/native/task.c int rt_task_suspend(RT_TASK *task) · line 435

Quoted 3 lines · Jan Kiszka
skins/native/task.c +1 -1
-	if (xnpod_unblockable_p()) {
+	if (xnpod_locked_p()) {

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

Signature · 1 line
					    Gilles Chanteperdrix.
author
#4 lore raw
Quoted 7 lines · Gilles Chanteperdrix

skins/native/task.c int rt_task_suspend(RT_TASK *task) · line 435

Quoted 3 lines · Jan Kiszka
skins/native/task.c +1 -1
-	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

#7 lore raw
In reply to Jan Kiszka
task-my.c 2006-07-12 15:40:16.549170000 +0200 renamed from task.c 2006-05-19 18:25:19.000000000 +0200 +2 -1
@@ -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.

Quoted 2 lines

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>

Signature · 2 lines
Best regards,
Dmitry Adamushko
author
#8 lore raw
Quoted 16 lines · Dmitry Adamushko
task-my.c 2006-07-12 15:40:16.549170000 +0200 renamed from task.c 2006-05-19 18:25:19.000000000 +0200 +2 -1
@@ -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;

    }
Quoted 4 lines

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.

Quoted 11 lines

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.

Quoted 2 lines

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

skins/native/task.c +4 -4
Index: ksrc/skins/native/task.c
===================================================================
@@ -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;
 	}
#5 lore raw
In reply to Jan Kiszka
Quoted 4 lines · Jan Kiszka
Quoted 2 lines · Gilles Chanteperdrix
Quoted 1 line · Jan Kiszka
Quoted 8 lines
Quoted 5 lines

skins/native/task.c int rt_task_suspend(RT_TASK *task) · line 435

Quoted 2 lines
skins/native/task.c +1 -1
-  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

Quoted 4 lines
Signature · 2 lines
Best regards,
Dmitry Adamushko
#6 lore raw

Sure I don't know all the implementation details and can be just driven by

Quoted 2 lines

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

Quoted 3 lines
Quoted 3 lines
Signature · 2 lines
Best regards,
Dmitry Adamushko
#9 lore raw
In reply to Jan Kiszka
Quoted 25 lines · Jan Kiszka
Quoted 16 lines · Dmitry Adamushko
task-my.c 2006-07-12 15:40:16.549170000 +0200 renamed from task.c 2006-05-19 18:25:19.000000000 +0200 +2 -1
@@ -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;

    }
Quoted 4 lines

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! :)

Quoted 2 lines

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.

Signature · 1 line
					    Gilles Chanteperdrix.