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

Manfred Spraul
2003-10-03T18:17:15Z

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