[Patch] Fix up return value from mq_open()

[Patch] Fix up return value from mq_open()

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1236 bytes --]

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

-- 
Doug Ledford <dledford@xsintricity.com>
              GPG KeyID: D30533DF




[-- Attachment #1.2: ipc-mqueue-return-value.patch --]
[-- Type: text/plain, Size: 777 bytes --]

commit b80e4058e2b4234fb33a9fd84b4ce288e8ba65cb
Author: Doug Ledford <dledford@redhat.com>
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.
    
    Signed-off-by: Doug Ledford <dledford@redhat.com>

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 0474ddb..3e53604 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -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;

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Patch] Fix up return value from mq_open()

[Jiri Slaby]

On 09/30/2011 04:15 AM, Doug Ledford wrote:
> 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.

Yes, then it should not definitely return ENOMEM, right? You exceeded a
limit, not ran out of memory. At least for the latter case.

>  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.
...
> commit b80e4058e2b4234fb33a9fd84b4ce288e8ba65cb
> Author: Doug Ledford <dledford@redhat.com>
> 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.

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.

> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 0474ddb..3e53604 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -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,
-- 
js
suse labs

Re: [Patch] Fix up return value from mq_open()

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 4229 bytes --]

On 9/30/2011 2:40 AM, Jiri Slaby wrote:
> On 09/30/2011 04:15 AM, Doug Ledford wrote:
>> 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.
> 
> 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.

>>  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.
> ...
>> commit b80e4058e2b4234fb33a9fd84b4ce288e8ba65cb
>> Author: Doug Ledford <dledford@redhat.com>
>> 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.
> 
> 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.

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

>> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
>> index 0474ddb..3e53604 100644
>> --- a/ipc/mqueue.c
>> +++ b/ipc/mqueue.c
>> @@ -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.

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.


-- 
Doug Ledford <dledford@xsintricity.com>
              GPG KeyID: D30533DF



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]