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

[Manfred Spraul]

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/