[Core] rewrite locking in split transaction handlers #18417
Conversation
The access to the split shared memory shall be as short as possible in order to not block any pending transaction initiated by the main half of the keyboard. Most transaction handlers are deterministic and short in runtime and therefore can simply take the lock for the whole duration of the handler. Other handlers execute code that might end up calling user code or invoke interactions with external peripherals. Those handlers need a specialized implementation that takes a lock for the shortest possible duration. This is done in the following commit.
KarlK90
force-pushed
the
fix/transaction-handlers-short-critical-sections
branch
from
e8fc119
to
ea4b76d
Compare
| @@ -535,7 +576,11 @@ static bool oled_handlers_master(matrix_row_t master_matrix[], matrix_row_t slav | |||
| } | |||
|
|
|||
| static void oled_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) { | |||
| if (split_shmem->current_oled_state) { | |||
| split_shared_memory_lock(); | |||
There was a problem hiding this comment.
The value of having a lock around a single read of a byte; either you read the value before or after the concurrent update, but there is no possibility of getting an inconsistent view of a single byte.
There was a problem hiding this comment.
You are right, I didn't think of that. I can surely change that, on the other hand I'm thinking: if someone changes the datatype in the future that doesn't have this atomic guarantees will they remember to introduce a lock here? I would bet that they don't do that, so while redundant at this point it could stay as a precaution? But I'm not commited to keeping it by all means. I totally forgot that the next step will be adding checksums to all transactions, so the atomic guarantees won't be there in the near future.
There was a problem hiding this comment.
Appreciate the concern for defensive measures, though.
There was a problem hiding this comment.
Doing a checksum on a single byte isn't going to speed things up either -- may as well just transfer the actual byte unconditionally.
There was a problem hiding this comment.
You're right, but the checksum is actually meant to prevent/detect data corruption as we usually don't use any resilient to means of communication (except for odd/even parity checks for some USART peripherals).
There was a problem hiding this comment.
Understood; but in that circumstance the request/response nature of the protocol should probably be modified such that at a certain amount of data transfer, you transmit payload+checksum in a single operation. That's a longer term discussion, at least.
There was a problem hiding this comment.
Sure the refactoring should be done after some discussion, does this prevent merging the changes in this PR?
| @@ -566,15 +611,19 @@ static bool st7565_handlers_master(matrix_row_t master_matrix[], matrix_row_t sl | |||
| } | |||
|
|
|||
| static void st7565_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) { | |||
| if (split_shmem->current_st7565_state) { | |||
| split_shared_memory_lock(); | |||
| @@ -384,7 +408,11 @@ static bool backlight_handlers_master(matrix_row_t master_matrix[], matrix_row_t | |||
| } | |||
|
|
|||
| static void backlight_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) { | |||
| backlight_set(split_shmem->backlight_level); | |||
There was a problem hiding this comment.
Keep this line - locking is not necessary for accessing a single byte.
Description
The access to the split shared memory shall be as short as possible in order to not block any pending transaction initiated by the main half of the keyboard. Most transaction handlers are deterministic and short in runtime and therefore can simply take the lock for the whole duration of the handler. Other handlers execute code that might end up calling user code or invoke interactions with external peripherals. Those handlers need a specialized implementation that takes a lock for the shortest possible duration. This is done in the following commit.
Types of Changes
Issues Fixed or Closed by This PR
Checklist