Re: [RFC][PATCH] ipc: mqueue: wq_add priority changed to dynamic priority

Re: [RFC][PATCH] ipc: mqueue: wq_add priority changed to dynamic priority

[Jonathan Haws]

Adding linux-rt-users group to thread.
________________________________________
From: Jonathan Haws <jhaws@sdl.usu.edu>
Sent: Monday, December 11, 2017 08:37
To: mingo@kernel.org; viro@zeniv.linux.org.uk; arnd@arndb.de; akpm@linux-foundation.org; deepa.kernel@gmail.com
Cc: linux-kernel@vger.kernel.org; tglx@linutronix.de; bigeasy@linutronix.de; rostedt@goodmis.org; Jonathan Haws
Subject: [RFC][PATCH] ipc: mqueue: wq_add priority changed to dynamic priority

Previous behavior added tasks to the work queue using the static_prio value
instead of the dynamic priority value in prio. This caused RT tasks to be
added to the work queue in a FIFO manner rather than by priority. Normal
tasks were handled by priority.

This fix utilizes the dynamic priority of the task to ensure that both RT
and normal tasks are added to the work queue in priority order. Utilizing
the dynamic priority (prio) rather than the base priority (normal_prio) was
chosen to ensure that if a task had a boosted priority when it was added to
the work queue, it would be woken sooner to to ensure that it releases any
other locks it may be holding in a more timely manner. It is understood that
the task could have a lower priority when it wakes than when it was added to
the queue in this (unlikely) case.

Signed-off-by: Jonathan Haws <jhaws@sdl.usu.edu>
---
 ipc/mqueue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 9649ecd..cb96db9 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -546,7 +546,7 @@ static void wq_add(struct mqueue_inode_info *info, int sr,
        ewp->task = current;

        list_for_each_entry(walk, &info->e_wait_q[sr].list, list) {
-               if (walk->task->static_prio <= current->static_prio) {
+               if (walk->task->prio <= current->prio) {
                        list_add_tail(&ewp->list, &walk->list);
                        return;
                }
--
2.7.4

Re: [RFC][PATCH] ipc: mqueue: wq_add priority changed to dynamic priority

[Steven Rostedt]

On Mon, 11 Dec 2017 17:12:05 +0000
Jonathan Haws <jhaws@sdl.usu.edu> wrote:

> Adding linux-rt-users group to thread.
> ________________________________________
> From: Jonathan Haws <jhaws@sdl.usu.edu>
> Sent: Monday, December 11, 2017 08:37
> To: mingo@kernel.org; viro@zeniv.linux.org.uk; arnd@arndb.de; akpm@linux-foundation.org; deepa.kernel@gmail.com
> Cc: linux-kernel@vger.kernel.org; tglx@linutronix.de; bigeasy@linutronix.de; rostedt@goodmis.org; Jonathan Haws
> Subject: [RFC][PATCH] ipc: mqueue: wq_add priority changed to dynamic priority
> 
> Previous behavior added tasks to the work queue using the static_prio value
> instead of the dynamic priority value in prio. This caused RT tasks to be
> added to the work queue in a FIFO manner rather than by priority. Normal
> tasks were handled by priority.
> 
> This fix utilizes the dynamic priority of the task to ensure that both RT
> and normal tasks are added to the work queue in priority order. Utilizing
> the dynamic priority (prio) rather than the base priority (normal_prio) was
> chosen to ensure that if a task had a boosted priority when it was added to
> the work queue, it would be woken sooner to to ensure that it releases any
> other locks it may be holding in a more timely manner. It is understood that
> the task could have a lower priority when it wakes than when it was added to
> the queue in this (unlikely) case.

Probably a better subject would be:

 ipc: mqueue: Have RT tasks queue in by priority in wq_add()

Other than that (and that this doesn't handle DL tasks),

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve


> 
> Signed-off-by: Jonathan Haws <jhaws@sdl.usu.edu>
> ---
>  ipc/mqueue.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 9649ecd..cb96db9 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -546,7 +546,7 @@ static void wq_add(struct mqueue_inode_info *info, int sr,
>         ewp->task = current;
> 
>         list_for_each_entry(walk, &info->e_wait_q[sr].list, list) {
> -               if (walk->task->static_prio <= current->static_prio) {
> +               if (walk->task->prio <= current->prio) {
>                         list_add_tail(&ewp->list, &walk->list);
>                         return;
>                 }
> --
> 2.7.4
> 

Re: [RFC][PATCH] ipc: mqueue: wq_add priority changed to dynamic priority

[Jonathan Haws]

On Mon, 2017-12-11 at 14:32 -0500, Steven Rostedt wrote:
> On Mon, 11 Dec 2017 17:12:05 +0000
> Jonathan Haws <jhaws@sdl.usu.edu> wrote:
> 
> > 
> > Adding linux-rt-users group to thread.
> > ________________________________________
> > From: Jonathan Haws <jhaws@sdl.usu.edu>
> > Sent: Monday, December 11, 2017 08:37
> > To: mingo@kernel.org; viro@zeniv.linux.org.uk; arnd@arndb.de; akpm@
> > linux-foundation.org; deepa.kernel@gmail.com
> > Cc: linux-kernel@vger.kernel.org; tglx@linutronix.de; bigeasy@linut
> > ronix.de; rostedt@goodmis.org; Jonathan Haws
> > Subject: [RFC][PATCH] ipc: mqueue: wq_add priority changed to
> > dynamic priority
> > 
> > Previous behavior added tasks to the work queue using the
> > static_prio value
> > instead of the dynamic priority value in prio. This caused RT tasks
> > to be
> > added to the work queue in a FIFO manner rather than by priority.
> > Normal
> > tasks were handled by priority.
> > 
> > This fix utilizes the dynamic priority of the task to ensure that
> > both RT
> > and normal tasks are added to the work queue in priority order.
> > Utilizing
> > the dynamic priority (prio) rather than the base priority
> > (normal_prio) was
> > chosen to ensure that if a task had a boosted priority when it was
> > added to
> > the work queue, it would be woken sooner to to ensure that it
> > releases any
> > other locks it may be holding in a more timely manner. It is
> > understood that
> > the task could have a lower priority when it wakes than when it was
> > added to
> > the queue in this (unlikely) case.
> Probably a better subject would be:
> 
>  ipc: mqueue: Have RT tasks queue in by priority in wq_add()

Is the best way to change that just to change it in the email thread or
resubmit the patch as v2?

> 
> Other than that (and that this doesn't handle DL tasks),
> 
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> -- Steve
> 

I'm still new to kernel scheduling details - can you elaborate on what
a DL task is?  I'm happy to update the patch to handle DL tasks as well
if it makes sense to do so.

Thanks!
Jon

> > 
> > 
> > Signed-off-by: Jonathan Haws <jhaws@sdl.usu.edu>
> > ---
> >  ipc/mqueue.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> > index 9649ecd..cb96db9 100644
> > --- a/ipc/mqueue.c
> > +++ b/ipc/mqueue.c
> > @@ -546,7 +546,7 @@ static void wq_add(struct mqueue_inode_info
> > *info, int sr,
> >         ewp->task = current;
> > 
> >         list_for_each_entry(walk, &info->e_wait_q[sr].list, list) {
> > -               if (walk->task->static_prio <= current-
> > >static_prio) {
> > +               if (walk->task->prio <= current->prio) {
> >                         list_add_tail(&ewp->list, &walk->list);
> >                         return;
> >                 }
> > --
> > 2.7.4
> > 

Re: [RFC][PATCH] ipc: mqueue: wq_add priority changed to dynamic priority

[Steven Rostedt]

On Mon, 11 Dec 2017 19:54:33 +0000
Jonathan Haws <jhaws@sdl.usu.edu> wrote:

> > Probably a better subject would be:
> > 
> >  ipc: mqueue: Have RT tasks queue in by priority in wq_add()  
> 
> Is the best way to change that just to change it in the email thread or
> resubmit the patch as v2?

That, or whoever takes it could change it before they pull it into git.

> 
> > 
> > Other than that (and that this doesn't handle DL tasks),
> > 
> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > 
> > -- Steve
> >   
> 
> I'm still new to kernel scheduling details - can you elaborate on what
> a DL task is?  I'm happy to update the patch to handle DL tasks as well
> if it makes sense to do so.

DL is a SCHED_DEADLINE task that has a dynamic priority that changes
over time. It's not a trivial task to compare them in these situations,
and they are currently not used much, and having DL tasks use this
queue is probably even more rare of an occurrence.

-- Steve