[NSWI004] A03 grading and notes

[Vojtech Horky]

Hello,

I will be uploading comments for A03 assignment to your grading 
repository shortly.

Generally, your solutions were pretty good - only two teams had some 
failing tests, most of the solutions were simple and elegant and quite 
easy to read (hence there are not that many comments this time). Good 
job and thank you!




Several issues that are worth mentioning are described below as they 
might be interesting to more teams even if not directly relevant to 
their code. They are in no particular order.


Note the difference between having the following in the scheduler:

static list_t* ready_queue_ptr;
static list_t ready_queue;

Both are correct but the first approach requires extra kmalloc and 
panic_if in the initialization to actually allocate the memory. It is 
much simpler to use the second variant as it means less complicated 
code. This is not Java where every object has to be created via "new".

Similarly, it is completely okay (and better as well) to have context_t 
directly embedded inside thread_t (instead of having a pointer) as you 
need only one allocation for the object. It is also a bit more cache 
friendly and it simplifies allocation code.


As already mentioned in some previous e-mails - check for errors where 
possible and also ensure that you are not leaking memory.

void *ptr1 = malloc(..);
if (ptr1 == NULL) { return NULL; }
void *ptr2 = malloc(...);
if (ptr2 == NULL) { return NULL; }

This code leaks memory of ptr1 if the second allocation fails. Correct 
approach is to add free(ptr1) the second "if" or use error gotos as are 
for example used in Linux [1].

[1] 
https://github.com/torvalds/linux/blob/156c05917e0920ef5643eb54c0ea71aae5d60c3d/tools/testing/selftests/cgroup/test_memcontrol.c#L27


I also need to stress once again the usefulness of the list_t/link_t 
data structures. I understand that they are different from what you 
might have been used to and they might look funny but they have one big 
advantage: adding an item to a list does not require any memory 
allocation. Hence, adding link_t sched_link to thread_t means that 
adding or removing a thread from ready queue does not require an 
allocation. An allocation that might fail. In a function that shall not 
fail as it cannot announce its failure.

There were solutions where thread_yield() could actually fail with 
out-of-memory condition. It is rather funny that a thread that 
voluntarily let other threads run would be punished by being killed (or 
by shutting down the whole kernel).

I also discovered a bug in thread/exhaust test that should have actually 
checked against this. The fixed test is now in the upstream repository. 
Please merge and fix your implementation if you are affected by this. 
There should be plenty of time to fix this until next assignment but 
feel free to contact us for an extension for this particular test. Sory 
for the confusion.


For completeness, the scheduler functions can be only thin wrappers 
around list_append() and list_remove() and thread functions will be 
using them:

thread_suspend() - scheduler_remove, scheduler_schedule_next
thread_yield() - scheduler_schedule_next
thread_finish() - scheduler_remove, thread_wakeup(joining_thread_if_set)
etc.

Teams that realized that scheduler_remove_thread() actually makes sense 
has the scheduler implementation about 6 added lines long and thread 
functions (except for thread_create) less than 10 lines each.


On a related note - if you decided to give threads explicit states, it 
it much easier to set them in the threads module rather than in the 
scheduler. Because then the following implementation is sufficient

thread_suspend - state = suspended; remove and yield
thread_finish - state = finished; remove, wake-up joining, yield

while if we set the states in the scheduler, we would need to either add 
an extra method or pass an extra argument or extra "if".


Perhaps this was not clearly stated in the assignment - thread_join 
practically invalidates the thread handle. I.e. any operation on 
thread_t that was already joined is undefined and does not make any sense.

thr = thread_create(...)
while not thread_finished(thr):
     print("still working")

thread_join(thr)
if thread_finished(thr): // ERROR! - thr is no longer valid
     print("finished")

Note that in managed environments (e.g. Java) this may not happen as the 
object will be around as long as you have a reference to it. In C, there 
would be no other place to actually destroy the thread if thread_join 
would not behave in this manner.


While having comments in your code is generally a good idea, please, 
comment things that are not clearly visible in the code. The following 
comments

// Set the thread as ready for execution
thread->state=READY;
// Append it to the ready list
list_append(&thread_queue, &thread->queue_link);

really do not add any new information that would not be provided by the 
actual code. It is not an error but it visually clutters the code and 
such comments are often not updated and thus become confusing over time.


Last note is related to your Git activity (and scoring). We will be 
recomputing the score to actually compute only reasonable lines of code 
as several of you managed to commit the whole target/ subdirectory and 
thus with each commit gained thousands of added lines because of the 
modifications of the log files under target/.

I do not want to prescribe any workflow but any decent Git client should 
be able to show you a diff prior a commit. If you see unrelated changes 
it is perhaps time to split your commit into multiple ones or remove the 
build files from the repository completely.

Note that on command-line, it is possible to use git add -p to split 
changes in one file into multiple commits.


As usual - if you have any comments/questions, do not hesitate to 
contact us (this ML for general questions and teachers-nswi004 for 
complaints/comments/questions about your grading).

When contacting us about grading or asking for a help for a particular 
piece of code, please, add your team mates to CC to speed up the 
communication.

Cheers,
- VH