POSIX message queues, 2.5.50

Some notes:
- coding style: linux functions usually have only one return at the end of the function, and goto internally. mqueue_parse_options() does that, mqueue_create contains multiple returns.
- why do you allocate the ext_wait_queue structure dynamically? Put it on the stack, that avoids error handling for failed allocations.
- reusing kernel functions is not a disadvantage - load_msg() and store_msg() automagically split the kmalloc allocations into page sized chunks.
- why do you use __add_wait_queue in wq_sleep_on()? It seems you have copied that code from kernel/sched.c - it's not needed. It was needed for

   cli()
   if(condition_var==0)
       sleep_on(&my_queue);

--
Manfred

Do we have to encourage this abonimation? We do have wait_event().

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Ok, I've fixed it.
I just didn't know that this also belongs to coding style (I don't like goto's much and it isn't in CodingStyle ;-).

Hmh, you remind me about one thing that I'm constantly forgetting. In fact allocation will stay (it is dynamic queue and it must be that way) but I should use there list.h stuff. My fault.

Of course. I just wanted to be fair. There were pointed out main differences (in first place) and some advantages.

And from Russell King:

wait_event is rather not good as I don't have condition to check - in that case I just place process in a queue and wait for wake_up.
But I agree that it is ugly - I used it only as a quick fix for one bug. Now I think I have good solution but I have to test it first.

Thanks for advices - new patch will be placed on
www.mat.uni.torun.pl/~wrona/posix_ipc
on Tuesday along with new library.

Krzysiek Benedyczak

The bad thing is that you use __add_wait_queue() and call the wait queue locking functions yourself. This is not needed. The only function that was permitted to do that is sleep_on(), and that will die soon. I've quoted sleep_on as a reminder to kill that code in kernel/sched.c. Just use add_wait_queue() instead of the internal functions. Or prepare_to_wait/finish_wait, that has a slighly lower locking overhead.

Btw, could you explain how your message priority implementation works?

If I understand it correctly, wq_add maintains a priority sorted linked list. wq_sleep() waits until the process becomes the first entry in the priority queue.
- You use the pid value as the thread identifier - why? Usually the task struct pointer is used within the kernel.
- Is it correct that wq_wakeup wakes up all processes that sleep in wq_send, and then the highest priority process continues? What about waking up just the highest priority process? Look at the wakeup code in ipc/msg.c - it implement message types that way. The sender looks through the list of waiting receivers, and directly sends the message to the right receiver [pipelined_send()]

--
Manfred

Some more explanation. Originally I used sleep_on(). But when it occurred to not work well on SMP I just make my own version of it with queue-wide semaphore protecting whole code. Then it started to work, but it was ugly, again I agree. Now I've made my own function taking wait_event() as a
template: I've removed checking for condition and add process to wait queue with EXCLUSIVE flag. On uniprocessor it works well - but on SMP I just a moment before had the same race - (wq were corrupted). When I added sem IN this function it started to work ok. Why (I mean why it didn't worked previously - in such a case I can imagine that wait_event would also produce some races) ?

Now it looks like this:

+void inline wait_exclusive(wait_queue_head_t *wq) +{
+ wait_queue_t wait;
+ init_waitqueue_entry(&wait, current);
+
+ add_wait_queue_exclusive(wq, &wait);
+ set_current_state(TASK_UNINTERRUPTIBLE);
+
++ up(&queue_sem)
+ schedule();
++ down(&queue_sem);
+
+ current->state = TASK_RUNNING;
+ remove_wait_queue(wq, &wait);
+}

I hope it is acceptable (?)

Ups :-). Yes it will let me remove one extra field from struct.

Yes it was correct, but useless. Now I wakeup only one process. But note that we use different algorithm then in msg.c: we haven't "pipelined" s&r.

I have applied previous suggestions & made some improvements - it works well.

If you have some further notices please send (especially about first case).

Thanks again

Krzysiek Benedyczak