[RFC][PATCHv2] Fixing POSIX wait queue to insert in task priority order for real-time, including normal tasks

[RFC][PATCHv2] Fixing POSIX wait queue to insert in task priority order for real-time, including normal tasks

[Jonathan Haws]

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][PATCHv2] Fixing POSIX wait queue to insert in task priority order for real-time, including normal tasks

[Jonathan Haws]

Sorry for the initial email - forgot the signed-off-by in my commit.
 Still new to this...

I'd like feedback from the community on this patch.  I've tested this
against my applications that were exhibiting problematic behavior when
tasks would wake up in FIFO order instead of priority order when trying
to write to a POSIX message queue.  Without the patch, multiple RT
tasks waiting to write to a queue wake up in FIFO order (assuming nice
value untouched); with the patch they wake up in priority order (based
on RT priority setting via sched_setparam.

Is this the correct way to remedy this issue?  Using prio here pulls in
the current priority of the task, including boosts from priority
inheritance.  In that case, would normal_prio be more appropriate?

Anyway, if this patch is acceptable then I'd also love some feedback to
know what steps need to be taken from here to push it along.

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;
>  		}

Re: [RFC][PATCHv2] Fixing POSIX wait queue to insert in task priority order for real-time, including normal tasks

[Steven Rostedt]

OK, when I said to Cc the kernel mailing list, I should have said that you
also need to still Cc everyone you want to read it. LKML gets over 600+ emails
a day. Nobody reads it all. Some people filter it, but others (like myself)
stopped reading it because I can barely keep up with just the emails I'm Cc'd
on.

The only reason I found this email is because I was going through my older
email, noticed that I haven't seen another patch from you, and realized that
you may have misunderstood what I meant by Ccing LKML. My fault for not being
clear. Sorry about that.

To know who to Cc, use "scripts/get_maintainer.pl" on your patch. But since
this is a RT issue, it is good to include the RT maintainers as well.

Next, the subject should have a topic in it. If you look at other changes in
the file you changed, you can usually figure it out. For example, looking at
other changes in ipc/mqueue.c, I see "ipc: mqueue:" which you can add to you
subject. That's because we want to know what commits are for what, when doing
git logs, especially one liner log output.

The subject should be a bit shorter. It should try to stay under 76 characters
(subtracting the "[RFC][PATCH*]"). 

Your subject is a little confusing. And you have zero change log. The subject
can be what you are doing, but write a change log to describe why you are
doing it. Don't be afraid to put in how you came about what you discovered. A
year from now, when someone is looking at this code, and does a git blame to
see why things are the way they are, it's good to know what the developer was
thinking for why they made the change. That way, the code can be modified if
circumstances change for why the code is the way it is. But without knowing
why changes were done, new updates may not be made out of fear for breaking
something they don't understand.

-- Steve


On Tue, Dec 05, 2017 at 06:15:32PM -0700, Jonathan Haws wrote:
> 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][PATCHv2] Fixing POSIX wait queue to insert in task priority order for real-time, including normal tasks

[Jonathan Haws]

> OK, when I said to Cc the kernel mailing list, I should have said
> that you
> also need to still Cc everyone you want to read it. LKML gets over
> 600+ emails
> a day. Nobody reads it all. Some people filter it, but others (like
> myself)
> stopped reading it because I can barely keep up with just the emails
> I'm Cc'd
> on.
> 
> The only reason I found this email is because I was going through my
> older
> email, noticed that I haven't seen another patch from you, and
> realized that
> you may have misunderstood what I meant by Ccing LKML. My fault for
> not being
> clear. Sorry about that.

Thanks for the update.  I was wondering if I messed something up when I
submitted this.  I realize this is a high-volume list and I have always
been curious how people stay on top of it.  It just makes sense to
direct specifics to the actual maintainers.

> To know who to Cc, use "scripts/get_maintainer.pl" on your patch. But
> since
> this is a RT issue, it is good to include the RT maintainers as well.

I didn't realize that script was there.  I'll make use of it!  As far
as RT maintainers go, I take it that is Thomas, Sebastian, and
yourself?


> Next, the subject should have a topic in it. If you look at other
> changes in
> the file you changed, you can usually figure it out. For example,
> looking at
> other changes in ipc/mqueue.c, I see "ipc: mqueue:" which you can add
> to you
> subject. That's because we want to know what commits are for what,
> when doing
> git logs, especially one liner log output.
> 
> The subject should be a bit shorter. It should try to stay under 76
> characters
> (subtracting the "[RFC][PATCH*]"). 

Just to make sure I'm following - you're looking for something along
the lines of:

[RFC][PATCH] ipc: mqueue: wq_add priority change to dynamic priority

> 
> Your subject is a little confusing. And you have zero change log. The
> subject
> can be what you are doing, but write a change log to describe why you
> are
> doing it. Don't be afraid to put in how you came about what you
> discovered. A
> year from now, when someone is looking at this code, and does a git
> blame to
> see why things are the way they are, it's good to know what the
> developer was
> thinking for why they made the change. That way, the code can be
> modified if
> circumstances change for why the code is the way it is. But without
> knowing
> why changes were done, new updates may not be made out of fear for
> breaking
> something they don't understand.
> 

Right - I'll shorten the subject as well and add a detailed change log.

Thanks for the tips!  You'll see PATCHv3 soon.



> On Tue, Dec 05, 2017 at 06:15:32PM -0700, Jonathan Haws wrote:
> > 
> > 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;
> >  		}

Re: [RFC][PATCHv2] Fixing POSIX wait queue to insert in task priority order for real-time, including normal tasks

[Steven Rostedt]

On Mon, 11 Dec 2017 15:13:29 +0000
Jonathan Haws <jhaws@sdl.usu.edu> wrote:
> 
> I didn't realize that script was there.  I'll make use of it!  As far
> as RT maintainers go, I take it that is Thomas, Sebastian, and
> yourself?

Correct, and you can also Cc: linux-rt-users as others like Julia
Cartwright may also be interested.

> 
> Just to make sure I'm following - you're looking for something along
> the lines of:
> 
> [RFC][PATCH] ipc: mqueue: wq_add priority change to dynamic priority

Hmm, that subject still doesn't parse. What about something like this:

 ipc: mqueue: Check priority of normal tasks in wq_add()

> 
> Thanks for the tips!  You'll see PATCHv3 soon.

I'm currently traveling, so I wont be able to look at it right away.

-- Steve