[NSWI004] a04 no-interrupt best practices

[Martin Zimen]

Hello,



I'm a little bit confused from your answer. If I understood it correctly 
Jakub is asking what would be the best practice. Of course in our assignment
we should know our code perfectly. But in real life that's not the case. And
I thought by having teams you're trying to get at least a little bit closer 
to reality and teach us best practices which are useful when we program 
something bigger. Because when we approach our assignment with attitude "the
code isn't that big", we can scratch most of the principles of writing good 
code which would only pay off in the long term.




We are encouraged to use defensive mechanisms in our code (at least I 
thought so from the feedback) and check things which aren't strictly 
necessary to check but help to avoid making a mistake or at least ease the 
debugging process. We are taught that we should write code as though someone
else will continue development. And if I go really carefully through my code
and reorder a function in a way that the lock (interrupt disable) isn't 
necessary someone else may come to my program do a small tweak and break it 
because he didn't know this is possibly a critical section because it wasn't
locked.




With this in mind, is it still true that we should disable interrupts as 
little as possible? Is it better to leave a comment "This doesn't need a 
lock"? Is it better to put the lock there with comment "This lock isn't 
necessary but better safe than sorry"? There may be cases where analysis 
whether a section does need a lock may be complicated and susceptible to 
errors when edited/refactored/changed. Should I put a lock there? Maybe 
comment "Shouldn't be necessary but just in case".




For example sem_destroy(sem_t* sem) which looks like this.

```

    enter_critical_section();




    assert(sem != NULL);

    sem_check_valid(sem);




    panic_if(!queue_is_empty(&sem->thread_queue), "Cannot destroy semaphore 
with threads waiting");

    sem->magic = MAGIC_INVALID;




    leave_critical_section();

```




 I feel this the most readable form. However not the most effective. I like 
that all validity checks (NULL assert, sem_check_valid) are together because
the feel like one logic block "Check if everything is all right" (Maybe we 
should put NULL assert into sem_check_valid as well? However then we cant 
put enter_critical_section() in the middle). But theoretically assert could 
be outside the critical section. The same goes for sem_check_valid(). It 
could be put outside critical section as long as destroy isn't called twice.
And because in our assignment we know the code well, we can make sure this 
doesn't happen. However if somebody else used our semaphore he could be 
confused that sometimes double destroy prints error and sometimes succeeds 
without problems. Even though it would be explicitly stated in documentation
that double destroy is undefined behavior. However this feels just wrong. 




And let's say I decide that most of the function need to be critical section
from top to bottom and then one function doesn't need from top to bottom but
only part of it. However the code is much more readable when the pattern 
goes on and I put that one function in critical section top to bottom as 
well.  Because then someone else might do stupid mistake when he presumes 
that all functions are critical section top to bottom but didn't notice this
one weird exception.




Therefore I don't feel like instructions "use it sparingly" and "use it for 
minimum duration possible" are complete. I'm trying to avoid the same 
problem we had with a02 when we got minus points for good code practices we 
didn't know about and which weren't talked about in this course. Therefore 
I'm trying to ask in advance here what are the best practices with locking 
because I have no idea what's right. I'd appreciate if you could go through 
our code and say if we use these right or wrong because in our team we don't
know. Or better provide some guidelines so we do it right and don't loose 
point unnecessarily.


And of course I'm not asking only for this case. This happens in higher 
languages. How different is the case here and in a higher-level language? 
Are there some rules which apply to synchronization practices in general?





Thank you for your time and answers

Martin ZImen

---------- Původní e-mail ----------
Od: Petr Tuma <petr.tuma at d3s.mff.cuni.cz>
Komu: Operating Systems Course <nswi004 at d3s.mff.cuni.cz>, Jakub Pelc <jakub.
pelc at email.cz>
Datum: 29. 11. 2020 17:06:41
Předmět: Re: [NSWI004] a04 no-interrupt best practices 
"Hello Jakub,

I think you are in fact answering yourself by the very act of asking - it is
not a good sign if you are not sure what can be going on in your code :-) 
that said, there are certainly systems that are so complex one cannot be 
sure, only we kind of hoped your assignments will not grow into that size :-
)

>From the OS implementation perspective, disabling interrupts is an operation
that should be used sparingly, and for minimum duration possible. In a real 
system it does not avert most problems because it is processor local.

Petr


On 29/11/2020 15:21, Jakub Pelc wrote:
> Hello,
> 
> I've encountered a few (short) functions where I think switching threads 
at any point should not break anything, but I'm not 100% sure, so I disabled
interrupts in this code. Is this the right approach? What are the best 
practices with this kind of "defensive" synchronization?
> 
> Best regards,
> Jakub Pelc
> 
> _______________________________________________
> NSWI004 mailing list
> NSWI004 at d3s.mff.cuni.cz
> https://d3s.mff.cuni.cz/mailman/listinfo/nswi004
> 
_______________________________________________
NSWI004 mailing list
NSWI004 at d3s.mff.cuni.cz
https://d3s.mff.cuni.cz/mailman/listinfo/nswi004
"
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://d3s.mff.cuni.cz/pipermail/nswi004/attachments/20201130/ae9a112a/attachment.htm>