ipc/mqueue.c: Drag unneeded code out of locks

This shouldn't be too controversial. I simply looked for where there was a tiny bit of waste in the message queue code.

---

index 4fcf39a..aa3f903 100644
@@ -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;
}

It's probably better to do this as three or four separate patches.

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.

Really? Alright if you insist I'll do the next version as multiple patches.

Fair enough, I'll rename it in the next version.

Right, that was dumb of me.

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

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

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.

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.

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.

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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-newbie" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.linux-learn.org/faqs

Hi Steven,

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

--
To unsubscribe from this list: send the line "unsubscribe linux-newbie" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.linux-learn.org/faqs

Adding/improving ipc kselftests would also be very welcome.

Thanks,
Davidlohr

--
To unsubscribe from this list: send the line "unsubscribe linux-newbie" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.linux-learn.org/faqs