This shouldn't be too controversial. I simply looked for where there
was a tiny bit of waste in the message queue code.
Signed-off-by: Steven Stewart-Gallus <[email protected]>
---
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 4fcf39a..aa3f903 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -278,16 +278,29 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
mq_bytes = mq_treesize + (info->attr.mq_maxmsg *
info->attr.mq_msgsize);
- spin_lock(&mq_lock);
- if (u->mq_bytes + mq_bytes < u->mq_bytes ||
- u->mq_bytes + mq_bytes > rlimit(RLIMIT_MSGQUEUE)) {
+ {
+ bool too_many_open_files;
+ long msgqueue_lim;
+ unsigned long u_bytes;
+
+ msgqueue_lim = rlimit(RLIMIT_MSGQUEUE);
+
+ spin_lock(&mq_lock);
+
+ u_bytes = u->mq_bytes;
+ too_many_open_files = u_bytes + mq_bytes < u_bytes ||
+ u_bytes + mq_bytes > msgqueue_lim;
+ if (!too_many_open_files)
+ u->mq_bytes += mq_bytes;
+
spin_unlock(&mq_lock);
+
/* mqueue_evict_inode() releases info->messages */
- ret = -EMFILE;
- goto out_inode;
+ if (too_many_open_files) {
+ ret = -EMFILE;
+ goto out_inode;
+ }
}
- u->mq_bytes += mq_bytes;
- spin_unlock(&mq_lock);
/* all is ok */
info->user = get_uid(u);
@@ -423,44 +436,60 @@ static int mqueue_create(struct inode *dir, struct dentry
*dentry,
umode_t mode, bool excl)
{
struct inode *inode;
- struct mq_attr *attr = dentry->d_fsdata;
- int error;
+ struct mq_attr *attr;
struct ipc_namespace *ipc_ns;
+ int error = 0;
+
+ if (!capable(CAP_SYS_RESOURCE)) {
+ error = -ENOSPC;
+ goto finish;
+ }
+
+ attr = dentry->d_fsdata;
spin_lock(&mq_lock);
ipc_ns = __get_ns_from_inode(dir);
if (!ipc_ns) {
error = -EACCES;
- goto out_unlock;
+ goto unlock_mq;
}
- if (ipc_ns->mq_queues_count >= ipc_ns->mq_queues_max &&
- !capable(CAP_SYS_RESOURCE)) {
+ if (ipc_ns->mq_queues_count >= ipc_ns->mq_queues_max) {
error = -ENOSPC;
- goto out_unlock;
+ goto unlock_mq;
}
ipc_ns->mq_queues_count++;
+unlock_mq:
spin_unlock(&mq_lock);
+ if (error != 0)
+ goto put_ipc_ns;
+
inode = mqueue_get_inode(dir->i_sb, ipc_ns, mode, attr);
if (IS_ERR(inode)) {
error = PTR_ERR(inode);
+
spin_lock(&mq_lock);
ipc_ns->mq_queues_count--;
- goto out_unlock;
+ spin_unlock(&mq_lock);
+
+ goto put_ipc_ns;
}
- put_ipc_ns(ipc_ns);
+put_ipc_ns:
+ if (ipc_ns)
+ put_ipc_ns(ipc_ns);
+
+ if (error != 0)
+ goto finish;
+
dir->i_size += DIRENT_SIZE;
dir->i_ctime = dir->i_mtime = dir->i_atime = CURRENT_TIME;
d_instantiate(dentry, inode);
dget(dentry);
- return 0;
-out_unlock:
- spin_unlock(&mq_lock);
- if (ipc_ns)
- put_ipc_ns(ipc_ns);
+
+finish:
return error;
}
@@ -485,26 +514,39 @@ static int mqueue_unlink(struct inode *dir, struct dentry
*dentry)
static ssize_t mqueue_read_file(struct file *filp, char __user *u_data,
size_t count, loff_t *off)
{
- struct mqueue_inode_info *info = MQUEUE_I(file_inode(filp));
- char buffer[FILENT_SIZE];
ssize_t ret;
+ pid_t notify_owner;
+ unsigned long qsize;
+ struct sigevent notify;
- spin_lock(&info->lock);
- snprintf(buffer, sizeof(buffer),
- "QSIZE:%-10lu NOTIFY:%-5d SIGNO:%-5d NOTIFY_PID:%-6d\n",
- info->qsize,
- info->notify_owner ? info->notify.sigev_notify : 0,
- (info->notify_owner &&
- info->notify.sigev_notify == SIGEV_SIGNAL) ?
- info->notify.sigev_signo : 0,
- pid_vnr(info->notify_owner));
- spin_unlock(&info->lock);
- buffer[sizeof(buffer)-1] = '\0';
+ {
+ struct mqueue_inode_info *info = MQUEUE_I(file_inode(filp));
- ret = simple_read_from_buffer(u_data, count, off, buffer,
- strlen(buffer));
- if (ret <= 0)
- return ret;
+ spin_lock(&info->lock);
+ notify_owner = pid_vnr(info->notify_owner);
+ notify = info->notify;
+ qsize = info->qsize;
+ spin_unlock(&info->lock);
+ }
+
+ {
+ char buffer[FILENT_SIZE];
+
+ snprintf(buffer, sizeof(buffer),
+ "QSIZE:%-10lu NOTIFY:%-5d SIGNO:%-5d NOTIFY_PID:%-6d\n",
+ qsize,
+ notify_owner ? notify.sigev_notify : 0,
+ (notify_owner &&
+ notify.sigev_notify == SIGEV_SIGNAL) ?
+ notify.sigev_signo : 0,
+ notify_owner);
+ buffer[sizeof(buffer)-1] = '\0';
+
+ ret = simple_read_from_buffer(u_data, count, off, buffer,
+ strlen(buffer));
+ if (ret <= 0)
+ return ret;
+ }
file_inode(filp)->i_atime = file_inode(filp)->i_ctime = CURRENT_TIME;
return ret;
@@ -524,18 +566,26 @@ static int mqueue_flush_file(struct file *filp, fl_owner_t id)
static unsigned int mqueue_poll_file(struct file *filp, struct
poll_table_struct *poll_tab)
{
- struct mqueue_inode_info *info = MQUEUE_I(file_inode(filp));
int retval = 0;
+ unsigned long curmsgs;
+ unsigned long maxmsg;
- poll_wait(filp, &info->wait_q, poll_tab);
+ {
+ struct mqueue_inode_info *info = MQUEUE_I(file_inode(filp));
- spin_lock(&info->lock);
- if (info->attr.mq_curmsgs)
+ poll_wait(filp, &info->wait_q, poll_tab);
+
+ spin_lock(&info->lock);
+ curmsgs = info->attr.mq_curmsgs;
+ maxmsg = info->attr.mq_maxmsg;
+ spin_unlock(&info->lock);
+ }
+
+ if (curmsgs)
retval = POLLIN | POLLRDNORM;
- if (info->attr.mq_curmsgs < info->attr.mq_maxmsg)
+ if (curmsgs < maxmsg)
retval |= POLLOUT | POLLWRNORM;
- spin_unlock(&info->lock);
return retval;
}
On Fri, 07 Nov 2014 05:40:37 +0000 (GMT) Steven Stewart-Gallus <[email protected]> wrote:
> This shouldn't be too controversial. I simply looked for where there
> was a tiny bit of waste in the message queue code.
>
It's probably better to do this as three or four separate patches.
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -278,16 +278,29 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
> mq_bytes = mq_treesize + (info->attr.mq_maxmsg *
> info->attr.mq_msgsize);
>
> - spin_lock(&mq_lock);
> - if (u->mq_bytes + mq_bytes < u->mq_bytes ||
> - u->mq_bytes + mq_bytes > rlimit(RLIMIT_MSGQUEUE)) {
> + {
> + bool too_many_open_files;
Well yes, that's what EMFILE means but "too_many_open_files" doesn't
make sense in this context!
> + long msgqueue_lim;
> + unsigned long u_bytes;
> +
> + msgqueue_lim = rlimit(RLIMIT_MSGQUEUE);
> +
> + spin_lock(&mq_lock);
> +
> + u_bytes = u->mq_bytes;
> + too_many_open_files = u_bytes + mq_bytes < u_bytes ||
> + u_bytes + mq_bytes > msgqueue_lim;
> + if (!too_many_open_files)
This test isn't really needed.
> + u->mq_bytes += mq_bytes;
> +
> spin_unlock(&mq_lock);
> +
> /* mqueue_evict_inode() releases info->messages */
> - ret = -EMFILE;
> - goto out_inode;
> + if (too_many_open_files) {
> + ret = -EMFILE;
> + goto out_inode;
> + }
> }
> - u->mq_bytes += mq_bytes;
> - spin_unlock(&mq_lock);
>
> /* all is ok */
> info->user = get_uid(u);
> @@ -423,44 +436,60 @@ static int mqueue_create(struct inode *dir, struct dentry
> *dentry,
> umode_t mode, bool excl)
> {
> struct inode *inode;
> - struct mq_attr *attr = dentry->d_fsdata;
> - int error;
> + struct mq_attr *attr;
> struct ipc_namespace *ipc_ns;
> + int error = 0;
> +
> + if (!capable(CAP_SYS_RESOURCE)) {
> + error = -ENOSPC;
> + goto finish;
> + }
Thatsabug. It only requires CAP_SYS_RESOURCE if we're trying with
queues_count >= queues_max.
On Fri, 2014-11-07 at 05:40 +0000, Steven Stewart-Gallus wrote:
> This shouldn't be too controversial. I simply looked for where there
> was a tiny bit of waste in the message queue code.
What's the benefit here? Seems very risky at very little gain.
The juice ain't worth the squeeze. NAK
Hello, thank you for the criticism.
> It's probably better to do this as three or four separate patches.
Really? Alright if you insist I'll do the next version as multiple
patches.
> Well yes, that's what EMFILE means but "too_many_open_files" doesn't
> make sense in this context!
Fair enough, I'll rename it in the next version.
> Thatsabug. It only requires CAP_SYS_RESOURCE if we're trying with
> queues_count >= queues_max.
Right, that was dumb of me.
> This test isn't really needed.
I don't follow. If the queue creation is not rejected then the
resource user has to be accounted for right? And we can't add the
resource to accounting if it is not created right?
Thank you,
Steven Stewart-Gallus
> What's the benefit here? Seems very risky at very little gain.
>
> The juice ain't worth the squeeze. NAK
Hello,
It is fair to argue that these changes are too tiny to be very
meaningful for performance but the other goal of this patch was also
to make the code look cleaner and easier for me and other people to
understand. I hope that is a reasonable desire.
It is not fair to argue that these changes are risky. If it is risky
for a person to add code then the code is too complicated to
understand and should be rewritten or tests or formal methods should
be used to verify correctness.
Are you suggesting that the mqueue subsystem is too complicated for
one to understand changes made to it and that it needs to be cleaned
up a bit? I am trying to make the code easier to understand with this
patch.
Or that you'd want some more testing of the mqueue subsystem or the
changes I made too it?
Or that you'd want some more formal methods to make the code easier to
verify? I suppose the area of code use a few extra sparse annotations.
Thank you,
Steven Stewart-Gallus
On Sat, 2014-11-15 at 04:44 +0000, Steven Stewart-Gallus wrote:
> > What's the benefit here? Seems very risky at very little gain.
> >
> > The juice ain't worth the squeeze. NAK
>
> Hello,
>
> It is fair to argue that these changes are too tiny to be very
> meaningful for performance but the other goal of this patch was also
> to make the code look cleaner and easier for me and other people to
> understand. I hope that is a reasonable desire.
I don't see how on earth you consider your patch makes things easier to
understand. For instance, adding local variables from structures passed
to a function does *not* make things more clearer:
+ bool too_many_open_files;
+ long msgqueue_lim;
+ unsigned long u_bytes;
+
+ msgqueue_lim = rlimit(RLIMIT_MSGQUEUE);
+
+ spin_lock(&mq_lock);
+
+ u_bytes = u->mq_bytes;
+ too_many_open_files = u_bytes + mq_bytes < u_bytes ||
+ u_bytes + mq_bytes > msgqueue_lim;
+ if (!too_many_open_files)
Plus you add context specific regions within the function (code around
{ }), ugly and something we've been removing!
In fact it makes it *harder* to read: Now you have to keep in mind where
each variable came from, ie: u_bytes.
> It is not fair to argue that these changes are risky.
Oh no? Andrew already found issues with the patch. But you claim there
is no risk. But hey, not getting the patch right the first time is fine,
except that the patch (i) has no tangible effects on performance, (ii)
as a cleanup, it does not make it any easier to read, (iii) can
potentially introduce bugs (yes, extra risk in subtleties when changing
critical regions and goto statements... we have had similar regressions
in ipc in the past).
Thanks,
Davidlohr
Hello,
My intent with my patch was to make things easier to understand
because it reduces the size of critical sections to more
understandable bite sized chunks. My patch would make the purposes of
the critical sections more obvious and understandable. In making this
patch I may have made a few mistakes which we can correct.
> For instance, adding local variables from structures passed
to a function does *not* make things more clearer:
This is not generally indicative of most of the patch. Moreover, the
local variable was introduced into a TIGHTLY restricted scope which
brings me to the next point.
> Plus you add context specific regions within the function (code
> around { }), ugly and something we've been removing!
Small context specific regions are GOOD. This is why we have functions
instead of one big ball of mud. I wouldn't be opposed to moving the {
} regions into smaller sub-functions themselves as the indentation can
get slightly annoying though. Also, this would let me put sparse
locking annotations on them.
> > It is not fair to argue that these changes are risky.
I still hold this position.
If these changes are risky for you then the code needs improvement or
you are incompetent. I am trying to make the code easier to understand
and REDUCE risks. Maybe my patch isn't as obvious and easily
understandable as it should be. In that case, would you agree that
even if my patch isn't the best way to improve the code that it still
needs improvement?
Finally, please don't ignore the rest of my message. Even if my patch
isn't that good there are lots of ways to compromise and improve it
such as adding tests, annotations and making it clearer.
Thank you,
Steven Stewart-Gallus
Hi Steven,
On 11/16/2014 08:40 PM, Steven Stewart-Gallus wrote:
> Finally, please don't ignore the rest of my message. Even if my patch
> isn't that good there are lots of ways to compromise and improve it
> such as adding tests, annotations and making it clearer.
I think you were already given ideas how to improve the patch:
a) split the patch.
b) create test cases so that you are able to check that the code still
behaves as it did before
Did you test the change to mqueue_create()?
c) Give each a good summary of what you want to achieve:
- readability
- coding style
- performance
- avoid a lock entirely, switch to RCU instead of spin_lock(), ...
- reduce the time a lock is held (usually only useful if the reduction
is significant - both relative and absolute).
- ...
Writing that down also helps you:
There were multiple patches that I've dropped myself - simply because I
have noticed that the patch doesn't achieve anything useful.
From your changes: The one to mqueue_read_file might make sense, it
avoids to hold the spinlock over the snprintf.
For the other changes, I don't see that they improve something, but
perhaps I have overlooked something.
Best regards,
Manfred
On Mon, 2014-11-17 at 20:08 +0100, Manfred Spraul wrote:
> Hi Steven,
>
> On 11/16/2014 08:40 PM, Steven Stewart-Gallus wrote:
> > Finally, please don't ignore the rest of my message. Even if my patch
> > isn't that good there are lots of ways to compromise and improve it
> > such as adding tests, annotations and making it clearer.
>
> I think you were already given ideas how to improve the patch:
>
> a) split the patch.
>
> b) create test cases so that you are able to check that the code still
> behaves as it did before
Adding/improving ipc kselftests would also be very welcome.
Thanks,
Davidlohr