Re: [PATCH] [2/2] posix message queues

[Manfred Spraul <manfred@colorfullife.com>]

Peter Wächtler wrote:

>+
>+#if 0
>+/* don't use fget() to avoid the fput() for speed reason 
>+ * on create/open the refcount is 1 and decremented on close
>+ * if you have a multithreaded app where one thread closes
>+ * the mqueue while another thread operates on it -> possible crash
>+ * the spec says the behavior is undefined
>+ * separate processes are not affected
>+ */
>
Could you remove that block, instead of just disabling it? Bugs spread 
at an incredible rate...
The right approach to avoid the cost of the fget is fget_light. But 
that's an optimization, it can be added later.

>+
>+static void local_remove_wait_queue(wait_queue_head_t *q, wait_queue_t * wait)
>+{
>+	spin_lock(&q->lock);
>+	__remove_wait_queue(q, wait);
>+	spin_unlock(&q->lock);
>+}
>
What's the difference between remove_wait_queue() and 
local_remove_wait_queue?

>+	queue->q_lspid = current->pid;
>+	queue->q_cbytes += msg_len;
>+	atomic_add(msg_len, &msg_bytes);
>
You are accounting posix messages in the sysv msg variables. Is that 
something we want, or should posix messages have their own accounting 
variables? I don't know what's better, but it should be discussed.

>+	queue->q_qnum++;
>+	inode->i_size = queue->q_qnum;
>+	inode->i_mtime = CURRENT_TIME;
>+
>+	if (waitqueue_active(&q->wait_recv)) {
>+		/* wake up all waiters to serve the highest prio waiter */
>+		wake_up_interruptible_all(&q->wait_recv);
>
Would it be possible to sort the waiters according to their prio? 
wake_all is always bad.

>+	} else {
>+		/* since there was no synchronously waiting process for message
>+		 * we notify it when the state of queue changed from
>+		 * empty to not empty */
>+		if (q->notify_pid != 0 && queue->q_qnum == 1) {
>+			/* TODO: Add support for sigev_notify==SIGEV_THREAD
>+			 *    we should create a thread in userspace
>+			 */
>
Is that comment still correct? You wrote that it's supported in user space.

It looks good.
--
    Manfred