Fix up return value from mq_open()
PATCH 1/1 Fix up return value from mq_open()
Commit d40dcdb ipc/mqueue.c: fix mq_open() return value changed the return value for mq_open() to return
EMFILE - The process already has the maximum number of files and
message queues open.
instead of
ENOMEM - Insufficient memory.
While the portion of d40dcdb that introduced using ERR_PTR() to propagate the condition were helpful, the change to the type of error was not. The specific test that was failing was a memory space test. It can fail if there are too many message queues open and this one would cause us to exceed the tasks counter for memory bytes capacity (aka, int wrap) or it can fail if the total number of bytes already allocated plus this allocation will exceed the user's RLIMIT for POSIX message queues.
However, depending on the values passed in via an attribute struct,
this could happen on the 1st file or the 100th file. There is no direct relationship to file count in this test. There is only a direct correlation to memory usage. Therefore return the code to giving ENOMEM instead of EMFILE.
(If the attachment is a problem I'll find another mailer to use and resend).
Signature · 2 lines
Doug Ledford <[email protected]> GPG KeyID: D30533DF
commit b80e4058e2b4234fb33a9fd84b4ce288e8ba65cb
Author: Doug Ledford <[email protected]> Date: Thu Sep 29 19:02:47 2011 -0400
ipc/mqueue: make the error returns match the conditions
We should not return -EMFILE when the problem is memory usage.
ipc/mqueue.c +1 -1
@@ -165,7 +165,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb, u->mq_bytes + mq_bytes > task_rlimit(p, RLIMIT_MSGQUEUE)) {spin_unlock (&mq_lock);/* mqueue_evict_inode() releases info->messages */ - ret = -EMFILE ; + ret = -ENOMEM ; goto out_inode; } u->mq_bytes += mq_bytes;
Quoted 10 lines · Doug Ledford
Commit d40dcdb
ipc/mqueue.c: fixmq_open()return value changed the return value formq_open()to returnEMFILE - The process already has the maximum number of files and
message queues open.instead of
ENOMEM - Insufficient memory.
Quoted 7 lines
While the portion of d40dcdb that introduced using
ERR_PTR()to propagate the condition were helpful, the change to the type of error was not. The specific test that was failing was a memory space test. It can fail if there are too many message queues open and this one would cause us to exceed the tasks counter for memory bytes capacity (aka, int wrap) or it can fail if the total number of bytes already allocated plus this allocation will exceed the user's RLIMIT for POSIX message queues.
Yes, then it should not definitely return ENOMEM, right? You exceeded a limit, not ran out of memory. At least for the latter case.
Quoted 5 lines
However, depending on the values passed in via an attribute struct,
this could happen on the 1st file or the 100th file. There is no direct relationship to file count in this test. There is only a direct correlation to memory usage. Therefore return the code to giving ENOMEM instead of EMFILE.
...
Quoted 7 lines
commit
b80e4058e2b4234fb33a9fd84b4ce288e8ba65cbAuthor: Doug Ledford <[email protected]> Date: Thu Sep 29 19:02:47 2011 -0400ipc/mqueue: make the error returns match the conditions
We should not return
-EMFILEwhen the problem is memory usage.
Sorry, I still don't udnerstand, how this test:
u->mq_bytes + mq_bytes > task_rlimit(p, RLIMIT_MSGQUEUE)
is about running out of memory. I see only a test for exceeding some quota. Should this be changed, change it to something like ENOSPC.
ipc/mqueue.c static struct inode *mqueue_get_inode(struct super_block *sb, · line 168
Quoted 10 lines
ipc/mqueue.c +1
-1@@ -165,7 +165,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb, u->mq_bytes + mq_bytes > task_rlimit(p, RLIMIT_MSGQUEUE)) {spin_unlock (&mq_lock);/* mqueue_evict_inode() releases info->messages */ - ret = -EMFILE ; + ret = -ENOMEM ;
The code now looks like:
int ret = -ENOMEM;
...
if (sth)
ret = -ENOMEM;
...
return ret;
which is not good.
thanks,
Signature · 2 lines
js suse labs
Quoted 19 lines · Jiri Slaby
Quoted 10 lines · Doug Ledford
Commit d40dcdb
ipc/mqueue.c: fixmq_open()return value changed the return value formq_open()to returnEMFILE - The process already has the maximum number of files and
message queues open.instead of
ENOMEM - Insufficient memory.
Quoted 7 lines
While the portion of d40dcdb that introduced using
ERR_PTR()to propagate the condition were helpful, the change to the type of error was not. The specific test that was failing was a memory space test. It can fail if there are too many message queues open and this one would cause us to exceed the tasks counter for memory bytes capacity (aka, int wrap) or it can fail if the total number of bytes already allocated plus this allocation will exceed the user's RLIMIT for POSIX message queues.
Quoted 2 lines
Yes, then it should not definitely return ENOMEM, right? You exceeded a limit, not ran out of memory. At least for the latter case.
I would prefer another return myself, but there doesn't seem to be one.
I had originally toyed with the idea of returning EPERM because you
don't have permissions to exceed the limit, but that's usually for file permissions so I decided that would be confusing. In the end, I stuck with ENOMEM because that's what you would get if the ulimit for memory was set low and you tried to exceed it. In theory, ENOMEM means out of memory in the system, in practice it's also used for memory consumption in excess of limits.
Quoted 10 lines
Quoted 5 lines
However, depending on the values passed in via an attribute struct,
this could happen on the 1st file or the 100th file. There is no direct relationship to file count in this test. There is only a direct correlation to memory usage. Therefore return the code to giving ENOMEM instead of EMFILE....
Quoted 4 lines
commit
b80e4058e2b4234fb33a9fd84b4ce288e8ba65cbAuthor: Doug Ledford <[email protected]> Date: Thu Sep 29 19:02:47 2011 -0400
Quoted 7 lines
Quoted 3 lines
ipc/mqueue: make the error returns match the conditions
We should not return
-EMFILEwhen the problem is memory usage.Sorry, I still don't udnerstand, how this test:
u->mq_bytes+mq_bytes>task_rlimit(p, RLIMIT_MSGQUEUE)
is about running out of memory.
Type ulimit -a, you'll see that RLIMIT_MSGQUEUE is specifically for the number of bytes of memory consumed by POSIX message queues as reported by bash anyway. That's memory. And it's being compared against the total amount of memory consumed in bytes after the allocation, not against the number of queues created.
Quoted 2 lines
I see only a test for exceeding some
quota. Should this be changed, change it to something like ENOSPC.
Or fix the man page...
Your original quote about ENOMEM was from the man page, just update it to read something like:
ENOMEM - Insufficient memory. The application has tried to open a POSIX message queue and there is either A) insufficient memory in the system to create the queue or B) the application has exceeded the user specific RLIMIT for the number of message queue bytes allocation to the application or C) the application has run into the linux implementation specific limit of 2GB of POSIX message queue bytes per application across all open message queues.
Quoted 19 lines
ipc/mqueue.c static struct inode *mqueue_get_inode(struct super_block *sb, · line 168
Quoted 10 lines
ipc/mqueue.c +1
-1@@ -165,7 +165,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb, u->mq_bytes + mq_bytes > task_rlimit(p, RLIMIT_MSGQUEUE)) {spin_unlock (&mq_lock);/* mqueue_evict_inode() releases info->messages */ - ret = -EMFILE ; + ret = -ENOMEM ;The code now looks like:
int ret =-ENOMEM;
...
if (sth)
ret =-ENOMEM;
...
return ret;
Quoted 1 line
which is not good.
I agree, but I left it because we hadn't heard from you yet and if it needed to be set to something other than ENOMEM it would be easy to do if the infrastructure for supporting it had not been ripped out already.
Signature · 2 lines
Doug Ledford <[email protected]> GPG KeyID: D30533DF