Hi,

On Thu, Aug 16, 2007 at 12:52:58AM +0300, Mindaugas R. wrote:

> Hello,
> 
> here is the implementation of POSIX message queues:

Nice work!
 
> http://www.netbsd.org/~rmind/mqueue.diff
> http://www.netbsd.org/~rmind/sys_mqueue.c
> http://www.netbsd.org/~rmind/sys_mqueue.h (src/sys/sys/mqueue.h)
> http://www.netbsd.org/~rmind/include_mqueue.h (src/include/mqueue.h)
> 
> Most of the POSIX Test-Suite tests passes.
> Please review.

 * Lock order:
 * 	mqueue_lock
 * 	  -> mqueue::mq_mtx

A brief description in the header file or here about which locks protect
what would be useful, if you're willing to do that.

	pool_init(&mqueue_poll, sizeof(struct mqueue), 0, 0, 0,
	    "mqueue_poll", &pool_allocator_nointr, IPL_NONE);
	pool_init(&mqdes_poll, sizeof(struct mq_desc), 0, 0, 0,
	    "mqdes_poll", &pool_allocator_nointr, IPL_NONE);

I don't think it's worth having a pool for these since they are "low volume"
items. On the other hand, it may be worthwhile having a pool for struct
mq_msg, and then having a limited amount of storage within mq_msg itself for
small messages. See ktealloc() in kern_trace.c.

	 * This is done without lock held, so while we are
	 * allocating - the count of descriptors might change, etc.
	 * Also, the file descriptors table must be consistent.

Really needs to be fixed before it goes in. The usual "check, allocate,
lock, check again, unlock+free+retry" dance applies I think :-).

	/* TODO: Hashing */

hash32_str()?

		/* Lock the message queue, and return .. */
		mutex_enter(&mq->mq_mtx);
		/* Check the access mode and permission */
		if (acc_mode != VNOVAL) {
			if (((acc_mode & mqdes->mq_acc_mode) == 0) ||
			    mqueue_access(l, mq, acc_mode)) {
				mutex_exit(&mq->mq_mtx);
				mq = NULL;
			}
		}
		break;

Should we not later be returning EPERM to the caller instead of EBADF?

	*timo = mstohz((ts.tv_sec * 1000) + (ts.tv_nsec / 1000000));

tstohz() does the same thing and itimespecfix() is probably of use here.

		/* Check the limit */
		if (p->p_mqueue_cnt == mq_open_max) {

That check's OK without any locks, but we need another one later while
mqueue_lock is held.

	/*
	 * According to POSIX - no message should be removed on
	 * failing case, thus we will check the pointers here.
	 */
	if (subyte(msg_ptr, 0) == -1)
		return EFAULT;
	if (msg_prio && (subyte(msg_prio, 0) == -1))
		return EFAULT;

There's no real point to this and it's expensive to do. Does it make the
system call pass some conformance test?

		/*
		 * Block until someone sends the message.
		 * While doing this, notification should not be sent.
		 */
		mq->mq_attrib.mq_flags |= MQ_RECEIVE;
		error = cv_timedwait_sig(&mq->mq_send_cv, &mq->mq_mtx, t);
		mq->mq_attrib.mq_flags &= ~MQ_RECEIVE;
		if (error || (mq->mq_attrib.mq_flags & MQ_UNLINK)) {
			mutex_exit(&mq->mq_mtx);
			return (error == EWOULDBLOCK) ? ETIMEDOUT : EINTR;
		}

error should be returned here instead of EINTR, unless the system call is
deemed to be non-restartable in which case it should read:

			switch (error) {
			case EWOULDBLOCK:
				return ETIMEDOUT;
			case ERESTART:
				return EINTR;
			default:
				return error;
			}

	(void)copyout(msg->msg_ptr, msg_ptr, msg->msg_len);

The system call shouldn't fail silently, even if it means throwing the
message away. The value from copyout should be returned.

	/* XXXlock: If it fails - the new attributes should not be set */

Does the spec say so? I can't imagine it's a big deal..

	/* Wake up all waiters, if there are such */
	if (mq->mq_attrib.mq_flags & MQ_RECEIVE)
		cv_broadcast(&mq->mq_send_cv);
	if (mq->mq_attrib.mq_flags & MQ_SEND)
		cv_broadcast(&mq->mq_recv_cv);

These kinds of tests are't needed, since cv_broadcast/cv_signal already do
it for you.

+257	STD MPSAFE	{ mqd_t sys_mq_open(const char * name, int oflag, \
+			    mode_t mode, struct mq_attr *attr); }

...

Any functions that allocate or free memory aren't MP safe, and the same goes
for file descriptor operations. Once the vmlocking stuff gets merged they can
be marked MP safe.

> - For message queue descriptors, I would like to use a file descriptor
> allocator - fdalloc()/fdremove() (follow the #ifdefs in the code), but there
> is a problem - there is no need to allocate the file structure or allow
> open(), read(), close(), etc calls, and fdalloc/fdremove is not working
> with NULL fdp->fd_ofiles[fd].

Whyso? Is this for fork/clone? Are select/poll valid on a message queue?

> - Currently, proc::p_mqd list of descriptors is protected by global
> mqueue_lock RW-lock. I am not sure if this worth inventing a separate
> per-process lock, because only mq_open(), mq_close() and mq_unlink() would
> compete (they acquires write lock). In normal case, these should not be an
> often calls in the system. Anyway - any thoughts on this?

If you find a non-contrived usage and lockstat shows it's a problem then
maybe we should look at it. I agree with Jason, it should be fine for now.

Thanks,
Andrew