Re: [PATCH] sched: Fix rt_task to work properly

Re: [PATCH] sched: Fix rt_task to work properly

[Yin-goo Yim]

Thank you for your immediate response.

> AFAICS this change is a larger layer violation (dependency issue)
> since AFAIK prio.h is a generic, common base header
> which is to provide priority definitions
> common to *all* of the
> more specific scheduler sub handling
> (deadline, rt, ...),
> which thus should not have to aggregate
> any specific knowledge whatsoever about
> various scheduler sub type specific handling.

We agree that the prio.h file has to be generic, but we thought that adding 
the priority information of deadline scheduler (i.e., #define MAX_DL_PRIO 0) 
to this file should be fine, because prio.h already has the priority 
information for cfs and rt schedulers (i.e., MAX_PRIO and MAX_RT_PRIO).

> rt_prio() quite likely is to be seen as an *rt-specific* API
> since it is defined in the *rt-specific* rt.h header.
> IOW, rt_prio() is *not to be used* for any areas where we
> do not have an RT case
> (quite certainly header docs should be added to rt_prio()
> to definitely emphasize this fact,
> maybe something like
> "Note that since this is an RT API it is meaningful for RT tasks only").

The rt_prio() function returns 1 or 0 according to whether or not the 
corresponding process is an rt task. Thus, we believe that rt_prio() is a 
suitable function to determine if a task is rt one, and can be called even 
for non-rt tasks. Indeed we can easily find examples where 
rt_prio()/rt_task() is called for an arbitrary process without limiting to 
rt tasks (e.g., sched_fork()).

Though our initial intention was to fix the origin of the problem, if you/we 
are not convinced of our patch due to dependency issues, we can reinforce 
the conditional statement in tg_has_rt_tasks() by adding !dl_task(p) as 
follows:

if (!dl_task(p) && rt_task(p) && task_group(p) == tg)

In this manner, we can avoid modifying the header files in 
include/linux/sched/, while having the same effect with respect to cgroup.

Thanks,
Yin-goo Yim

Re: [PATCH] sched: Fix rt_task to work properly

[Andreas Mohr]

Hi,

[no properly binding reference via In-Reply-To: available thus manually re-creating, sorry]

> +++ b/include/linux/sched/deadline.h
> @@ -1,13 +1,7 @@
> #ifndef _SCHED_DEADLINE_H
> #define _SCHED_DEADLINE_H
> 
> -/*
> - * SCHED_DEADLINE tasks has negative priorities, reflecting
> - * the fact that any of them has higher prio than RT and
> - * NORMAL/BATCH tasks.
> - */
> -
> -#define MAX_DL_PRIO 0
> +#include <linux/sched/prio.h>
> 
> 
> 
> 
> +++ b/include/linux/sched/deadline.h
> @@ -1,13 +1,7 @@
> #ifndef _SCHED_DEADLINE_H
> #define _SCHED_DEADLINE_H
> 
> -/*
> - * SCHED_DEADLINE tasks has negative priorities, reflecting
> - * the fact that any of them has higher prio than RT and
> - * NORMAL/BATCH tasks.
> - */
> -
> -#define MAX_DL_PRIO 0
> +#include <linux/sched/prio.h>



AFAICS this change is a larger layer violation (dependency issue)
since AFAIK prio.h is a generic, common base header
which is to provide priority definitions
common to *all* of the
more specific scheduler sub handling
(deadline, rt, ...),
which thus should not have to aggregate
any specific knowledge whatsoever about
various scheduler sub type specific handling.


While this "complaint" won't help your case
since it will cause the fix to be semi-incorrect,
some things come to mind:

rt_prio() quite likely is to be seen as an *rt-specific* API
since it is defined in the *rt-specific* rt.h header.

IOW, rt_prio() is *not to be used* for any areas where we
do not have an RT case
(quite certainly header docs should be added to rt_prio()
to definitely emphasize this fact,
maybe something like
"Note that since this is an RT API it is meaningful for RT tasks only").



IOW, it quite likely is some API *user* which is
making a mistake since it uses rt_prio() for cases
where it should be using APIs provided by other non-RT cases.
To be specific, according to your comments
this API user seems to be tg_has_rt_tasks()
which is wrongly calling rt_prio() [not "rt_task", right??]
for tasks where it has NOT yet properly determined
that in fact they *are* RT-type tasks
(the wording of "tg_has_rt_tasks" clearly suggests
that this API does *not* know yet
which task type it currently deals with,
so it does need to figure the type out, reliably,
by *not* directly calling into a fully RT-specific API).

That's at least what I'm inferring from the situation context,
without having looked into details.


BTW, Good Catch!


HTH,

Andreas Mohr

[PATCH] sched: Fix rt_task to work properly

[Yin-goo Yim]

Hi,
I am a graduate student of System Software Lab. at Konkuk University

We found that the rt_task() function returns true even if the task is a
deadline task scheduled by SCHED_DEADLINE, because the function simply
checks whether the priority of the task is smaller than 100. As the
priority of deadline tasks is smaller than 0, the rt_task() function
returns true for deadline tasks. However, there exists a separate function
(i.e., dl_task()) to check if a task is a deadline task. That is, we
believe that rt_task() has to return true only if the task is an rt task
scheduled by SCHED_FIFO or SCHED_RR, while giving false for deadline tasks.

This ambiguity of rt_task() is causing a strange problem in cgroup. More
precisely, we found that -EBUSY was always returned when we tried to run a
task group that consisted of rt tasks together with a deadline task. This
happened regardless of how much CPU resources were reserved by cgroups and

deadline tasks. It turns out that tg_rt_schedulable() returns ?EBUSY by the
following conditional statement for deadline tasks:
if (rt_bandwidth_enabled() && !runtime && tg_has_rt_tasks(tg))
return -EBUSY;
For deadline tasks, tg_has_rt_tasks() in the above statement has to return
false. However, the current implementation of rt_task() called by
tg_has_rt_tasks() only checks whether the priority of tasks are smaller
than 100; thus, it returns true even for deadline tasks that have a minus
number.

Our patch is to fix this problem by checking the priority of tasks more
precisely (i.e., 0 < priority < 100) in rt_task().

Signed-off-by: Yin-goo Yim <ygyim@konkuk.ac.kr>
---
include/linux/sched/deadline.h | 8 +-------
include/linux/sched/prio.h     | 8 ++++++++
include/linux/sched/rt.h       | 2 +-
3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
index 9089a2a..3e38564 100644
--- a/include/linux/sched/deadline.h
+++ b/include/linux/sched/deadline.h
@@ -1,13 +1,7 @@
#ifndef _SCHED_DEADLINE_H
#define _SCHED_DEADLINE_H

-/*
- * SCHED_DEADLINE tasks has negative priorities, reflecting
- * the fact that any of them has higher prio than RT and
- * NORMAL/BATCH tasks.
- */
-
-#define MAX_DL_PRIO 0
+#include <linux/sched/prio.h>

static inline int dl_prio(int prio)
{
diff --git a/include/linux/sched/prio.h b/include/linux/sched/prio.h
index d9cf5a5..3faddc6 100644
--- a/include/linux/sched/prio.h
+++ b/include/linux/sched/prio.h
@@ -25,6 +25,14 @@
#define DEFAULT_PRIO (MAX_RT_PRIO + NICE_WIDTH / 2)

/*
+ * SCHED_DEADLINE tasks has negative priorities, reflecting
+ * the fact that any of them has higher prio than RT and
+ * NORMAL/BATCH tasks.
+ */
+
+#define MAX_DL_PRIO 0
+
+/*
  * Convert user-nice values [ -20 ... 0 ... 19 ]
  * to static priority [ MAX_RT_PRIO..MAX_PRIO-1 ],
  * and back.
diff --git a/include/linux/sched/rt.h b/include/linux/sched/rt.h
index a30b172..062cddb 100644
--- a/include/linux/sched/rt.h
+++ b/include/linux/sched/rt.h
@@ -5,7 +5,7 @@

static inline int rt_prio(int prio)
{
- if (unlikely(prio < MAX_RT_PRIO))
+ if (unlikely(prio < MAX_RT_PRIO && prio > MAX_DL_PRIO))
  return 1;
  return 0;
}