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

Peter Wächtler
2003-10-05T12:44:53Z

Am Fre, 2003-10-03 um 20.16 schrieb 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.
>

removed and replaced with fget_light/fput_light

> >+
> >+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?
>

don't disable local_irq , because no irq involved
don't know how expensive a local_irq_save is on SMP

> >+ 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.

msg_bytes is local to posixqueue.c
if I use the SysV queue code, I use its storage. What do you mean by
accounting? Whatever security_msg_msg_alloc() does?
We have no enforcable user limits on queues (in context of ulimits).

> >+ 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.
>

yes, I will try that.

> >+ } 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.
>

Userspace translates SIGEV_THREAD to something that uses SIGEV_SIGNAL.
Ulrich made a suggestion to use a futex, but I think of something even
more lightweight. Just put the requestor right to sleep.
No further syscall involved (and avoids a race inbetween sys_mq_notify
and sigsuspend).

--
Peter Wächtler http://homepage.mac.com/pwaechtler/