[NSWI004] Thread queue hurts

[Vojtech Horky]

Hello.

Dne 10. 12. 20 v 12:18 Jan Kytka napsal(a):
> Hello,
> 
> our team got a comment about ugly thread queue implementation, which was
> implemented using a generic list_t macro that takes: type of list item, 
> the link member and name
> of the new specialised struct, and expands to a new specialised struct 
> containing list_t
> and all the methods used to work with the new struct.

Ooops, sorry about an unclear comment. It was supposed to refer to the 
mixing of camelCase and snake_case names and should have been merged 
with the previous comment about formatting ;-).

> 
> I understand that this is not the most beautiful thing under the sun :-)
> 
> But anyhow, I couldn't figure out how to make the thread queue a 
> first-class citizen
> while at the same time make it reusable by both the scheduler and the 
> mutex, since
> they each use different link_t in the thread_t structure.

My point was rather that the name suggests it is a queue implementation 
but regarding types, it is still just list_t and also the user has to 
use struct_container_of macro. I guess it comes from the fact that you 
do not want to expose the whole list_generic in the mutex.h header but 
it just seems confusing to me.

As a side-note: it would actually be possible to use the same link for 
both scheduler and mutexes (either the thread is ready or is blocked).


To me, the following is easier to use, provides pretty good type-safety 
and offers little space for misuse. And it can be used by both mutexes 
and semaphores, and eventually by also the disk driver, for example.

typedef struct {
     list_t queue;
} thread_queue_t;

struct thread {
     ...
     link_t queue_link;
     ...
}

typedef struct {
     thread_queue_t waiting_threads;
     ...
} mutex_t;


void thread_queue_enqeueu(thread_queue_t *tq, thread_t *t) {
    list_append(&tq->list, &t->link);
}
thread_t *thread_queue_dequeue(thread_queue_t *tq) { ... }

I would even go one step further and add functions that block the 
current thread after adding it to the queue and waking-up the first 
queue as top-level operations as that is the typical use.


But this is also not perfect. Your list_generic avoid some code 
duplication that is needed here, on the other hand here we do not add an 
extra member just to keep the type information etc. etc.


As for many other comments to your solutions - if we believe that 
another approach balances the ease of use, readability, maintainability 
etc. better, we try to mention it. It is up to you whether you want to 
change your code or not. You know it better than we do and can decide 
whether the change is worth it.


I hope this explains things a bit better, I am sorry if the intent of 
the comment was not clear from the grading file.

Cheers,
- VH