I will be away until July, but I would appreciate reviews and collection of feedback in the meantime. Also, for the unlikely event that everyone is absolutely happy about it, I would also not oppose a merge.
Every time I looked at the probe code in the past, my mind ended up twisted and confused. A probe could change the "enabled" state (tracking the temperature) or be removed at any time (unless the mtx is held), yet the code did not seem to reflect this.
We un-twist my mind by implementing probe states:
cold: reflects cold backend temperature
scheduled: probe is on binheap, waiting for its time to come
With this new scheme, we can now have (I think) a clean state diagram (see dot file):
a probe begins in the cold state
from cold, it can either get removed or scheduled via VBP_Control(..., 1)
from scheduled, it can go back to cold (via VBP_Control(..., 0)) or be picked up by vbp_thread() to change to running (aka task started)
once the task finishes, it normally goes back to scheduled, but in the meantime it could have changed to cooling or further on to deleted, so vbp_task_comple() handles these cases and either transitions to cold or deletes the probe
if the task can not be scheduled, the same handling happens
We now also remove running probes from the binheap to remove complexity. I am not entirely sure if there could have been a good reason for keeping running probes on the binheap, so if this is the case, we might want to reconsider this change. But it is not obvious to me how deleting and reinserting just to delete and reinsert later should be better than deleting and reinserting later.
I will be away until July, but I would appreciate reviews and collection of feedback in the meantime. Also, for the unlikely event that everyone is absolutely happy about it, I would also not oppose a merge.
Every time I looked at the probe code in the past, my mind ended up twisted and confused. A probe could change the "enabled" state (tracking the temperature) or be removed at any time (unless the
mtxis held), yet the code did not seem to reflect this.We un-twist my mind by implementing probe states:
cold: reflects cold backend temperaturescheduled: probe is on binheap, waiting for its time to comerunning: a task has been started to run the probecooling: running probe disableddeleted: running probe removed (whilecoolingonly)With this new scheme, we can now have (I think) a clean state diagram (see dot file):
coldstatecold, it can either get removed or scheduled viaVBP_Control(..., 1)scheduled, it can go back tocold(viaVBP_Control(..., 0)) or be picked up byvbp_thread()to change torunning(aka task started)scheduled, but in the meantime it could have changed tocoolingor further on todeleted, sovbp_task_comple()handles these cases and either transitions tocoldor deletes the probeWe now also remove running probes from the binheap to remove complexity. I am not entirely sure if there could have been a good reason for keeping running probes on the binheap, so if this is the case, we might want to reconsider this change. But it is not obvious to me how deleting and reinserting just to delete and reinsert later should be better than deleting and reinserting later.
Written to fix #4108 for good