dax: use atomic flags to avoid race conditions#10580
dax: use atomic flags to avoid race conditions#10580checkupup wants to merge 1 commit intothesofproject:mainfrom
Conversation
In DP domain, sof_dax_process runs in a DP thread, other sof_dax_* functions run in a different LL thread. Using atomic flags instead of original dax_ctx->update_flags to avoid potential race conditions when updating flags. Since dax_inf.h should not have any dependencies on SOF, we define the new atomic flags in dax.c Signed-off-by: Jun Lai <jun.lai@dolby.com>
checkupup
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
|
Can one of the admins verify this patch?
|
|
Hello @johnylin76 @lgirdwood , let us continue our discussion on race condition issue here. |
| DAX_FLAG_READ_AND_CLEAR, | ||
| }; | ||
|
|
||
| static int32_t flag_process(struct dax_adapter_data *adapter_data, |
There was a problem hiding this comment.
I think zephyr already has an implementation of atomic mask (e.g. atomic_test_and_set_bit). Did you consider it?
| { | ||
| switch (opt_mode) { | ||
| case DAX_FLAG_READ: | ||
| return atomic_read(&adapter_data->proc_flags[ffs(flag) - 1]); |
There was a problem hiding this comment.
You could just define indexes of individual bits instead of masks:
define DAX_DEVICE_MASK 0x4 -> define DAX_DEVICE_IDX 2
This would eliminate ffs
|
|
||
| enum dax_flag_opt_mode { | ||
| DAX_FLAG_READ = 0, | ||
| DAX_FLAG_ADD, |
There was a problem hiding this comment.
DAX_FLAG_ADD -> DAX_FLAG_SET?
|
|
||
| struct dax_adapter_data { | ||
| struct sof_dax dax_ctx; | ||
| atomic_t proc_flags[32]; |
There was a problem hiding this comment.
@checkupup are you able to provide a little more context, as I can now see an array of data instead of a global flag. My understanding was that we would only have 2 instances and had to protect a single shared resource between both DP instances ?
i.e. we can use https://docs.zephyrproject.org/latest/doxygen/html/group__atomic__apis.html#gaef275bd78e093e5b5177ef725d0f570a
bool atomic_cas(atomic_t *target, atomic_val_t old_value, atomic_val_t new_value);
/* is resource mine ? */
owner_id = atomic_get(atomic_var);
if (owner_id == MY_INSTANCE_ID_VALUE) {
// I can freely use the resource
} else {
/* i dont own it, try and get resource */
is_resource_mine = atomic_cas(atomic_var, FREE_VALUE, MY_INSTANCE_ID_VALUE);
if (!is_resource_mine)
yield(); // try again when we are next scheduled
}
// I can use resource at this point
...
/* put resource */
is_resource_mine = atomic_cas(atomic_var, MY_INSTANCE_ID_VALUE, FREE_VALUE);
if (!is_resource_mine)
// should never fail - handle.where each instance would have a unique id for the atomic variable.
4 participants
In DP domain,
sof_dax_processruns in a DP thread, othersof_dax_*functions run in a different LL thread. Using atomic flags instead of originaldax_ctx->update_flagsto avoid potential race conditions when updating flags.Since
dax_inf.hshould not have any dependencies on SOF, we define the new atomic flags indax.c