Separate the union of timeslice and result in struct mrb_task#6869
Conversation
Bug scenario: * VM returns an Exception `t->state.result = mrb_vm_exec(...);` * Despite task is still MRB_TASK_STATUS_RUNNING, IRQ triggered by chance and `mrb_tick()` executes `t->state.timeslice--;` * But the same memory area already holds the `result`, `timeslice--` reduces `result.value.p`'s top byte The fix is to separate `timeslice` and `result` into different fields. I have considered improving critical sections, but I ended up with this patch because I believe it is widely effective and less error-prone.
There was a problem hiding this comment.
Code Review
This pull request removes the state union from the mrb_task struct, making timeslice and result independent fields. The reviewer provided valuable feedback on optimizing struct packing to avoid padding, simplifying garbage collection marking for the now-independent result field, and reordering assignments when starting a task to prevent potential race conditions with the interrupt handler.
| volatile uint8_t timeslice; /* Remaining ticks while RUNNING */ | ||
| mrb_value result; /* Task return value */ |
There was a problem hiding this comment.
Grouping all uint8_t fields together at the top of the struct (e.g., right after uint8_t reason;) allows the compiler to pack them tightly without padding. Leaving volatile uint8_t timeslice; here introduces 3 bytes of padding on 32-bit systems and 7 bytes of padding on 64-bit systems before the mrb_value result field.
To apply this optimization, please accept this suggestion to remove timeslice from its current position, and manually add volatile uint8_t timeslice; right after uint8_t reason; at the top of the struct.
mrb_value result; /* Task return value */| if (t->status == MRB_TASK_STATUS_DORMANT) { | ||
| mrb_gc_mark_value(mrb, t->state.result); | ||
| mrb_gc_mark_value(mrb, t->result); | ||
| } |
There was a problem hiding this comment.
Since timeslice and result are no longer unioned, t->result is now a dedicated field that is always a valid mrb_value (initialized to a safe immediate value via memset in task_alloc). We can safely mark t->result unconditionally, which simplifies the code and prevents any potential GC misses during state transitions or synchronous execution.
mrb_gc_mark_value(mrb, t->result);| t->status = MRB_TASK_STATUS_RUNNING; | ||
| t->state.timeslice = MRB_TIMESLICE_TICK_COUNT; | ||
| t->timeslice = MRB_TIMESLICE_TICK_COUNT; |
There was a problem hiding this comment.
In interrupt-driven systems, it is a best practice to initialize data fields (like timeslice) before setting the state flag (like status) that enables the interrupt handler (mrb_tick) to process them. If an interrupt occurs between setting status to RUNNING and setting timeslice, the interrupt handler might read a stale timeslice value. Reversing the order of these two assignments eliminates this transient state.
t->timeslice = MRB_TIMESLICE_TICK_COUNT;
t->status = MRB_TASK_STATUS_RUNNING;`mrb_tick()` observes `status == RUNNING` before touching `timeslice`, so initializing `timeslice` first avoids exposing a partially initialized running state
Bug scenario:
t->state.result = mrb_vm_exec(...);mrb_tick()executest->state.timeslice--;result,timeslice--reducesresult.value.p's top byteFix
Separate
timesliceandresultinto different fields.I considered improving critical sections, but I ended up with this patch because I believe it is widely effective and less error-prone.