niklas/cyclonedds
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix race conditions in instance id-to-key lookup

Browse source 
this is just an intermediate step: firstly, iid-to-key lookup is a linear scan; secondly, the lookup should happen on the RHC or the WHC and not on some internal map to associates key values with instance handles and is independent of entities

Signed-off-by: Erik Boasson <eb@ilities.com>
This commit is contained in:
Erik Boasson 2018-12-19 13:29:53 +01:00
parent 0f7ce1ac43
commit 179a35d192
3 changed files with 45 additions and 74 deletions
Download patch file Download diff file Expand all files Collapse all files

44
src/core/ddsc/src/dds_instance.c
View file

@ -250,9 +250,7 @@ dds_unregister_instance_ih_ts(
bool autodispose = true; bool autodispose = true;
dds_write_action action = DDS_WR_ACTION_UNREGISTER; dds_write_action action = DDS_WR_ACTION_UNREGISTER;
dds_entity *wr; dds_entity *wr;
struct ddsi_tkmap *map; struct ddsi_tkmap_instance *tk;
const dds_topic *topic;
void *sample;
rc = dds_entity_lock(writer, DDS_KIND_WRITER, &wr); rc = dds_entity_lock(writer, DDS_KIND_WRITER, &wr);
if (rc != DDS_RETCODE_OK) { if (rc != DDS_RETCODE_OK) {
@ -269,19 +267,21 @@ dds_unregister_instance_ih_ts(
action |= DDS_WR_DISPOSE_BIT; action |= DDS_WR_DISPOSE_BIT;
} }
map = gv.m_tkmap;
if (asleep) { if (asleep) {
thread_state_awake(thr); thread_state_awake(thr);
} }
topic = dds_instance_info((dds_entity*)wr); tk = ddsi_tkmap_find_by_id (gv.m_tkmap, handle);
sample = ddsi_sertopic_alloc_sample (topic->m_stopic); if (tk) {
if (ddsi_tkmap_get_key (map, topic->m_stopic, handle, sample)) { struct ddsi_sertopic *tp = ((dds_writer*) wr)->m_topic->m_stopic;
void *sample = ddsi_sertopic_alloc_sample (tp);
ddsi_serdata_topicless_to_sample (tp, tk->m_sample, sample, NULL, NULL);
ddsi_tkmap_instance_unref (tk);
ret = dds_write_impl ((dds_writer*)wr, sample, timestamp, action); ret = dds_write_impl ((dds_writer*)wr, sample, timestamp, action);
} else{ ddsi_sertopic_free_sample (tp, sample, DDS_FREE_ALL);
} else {
DDS_ERROR("No instance related with the provided handle is found\n"); DDS_ERROR("No instance related with the provided handle is found\n");
ret = DDS_ERRNO(DDS_RETCODE_PRECONDITION_NOT_MET); ret = DDS_ERRNO(DDS_RETCODE_PRECONDITION_NOT_MET);
} }
ddsi_sertopic_free_sample (topic->m_stopic, sample, DDS_FREE_ALL);
if (asleep) { if (asleep) {
thread_state_asleep(thr); thread_state_asleep(thr);
} }
@ -387,14 +387,17 @@ dds_dispose_ih_ts(
if (rc == DDS_RETCODE_OK) { if (rc == DDS_RETCODE_OK) {
struct thread_state1 * const thr = lookup_thread_state(); struct thread_state1 * const thr = lookup_thread_state();
const bool asleep = !vtime_awake_p(thr->vtime); const bool asleep = !vtime_awake_p(thr->vtime);
struct ddsi_tkmap *map = gv.m_tkmap; struct ddsi_tkmap_instance *tk;
const dds_topic *topic = dds_instance_info((dds_entity*)wr);
void *sample = ddsi_sertopic_alloc_sample (topic->m_stopic);
if (asleep) { if (asleep) {
thread_state_awake(thr); thread_state_awake(thr);
} }
if (ddsi_tkmap_get_key (map, topic->m_stopic, handle, sample)) { if ((tk = ddsi_tkmap_find_by_id (gv.m_tkmap, handle)) != NULL) {
ret = dds_dispose_impl(wr, sample, handle, timestamp); struct ddsi_sertopic *tp = ((dds_writer*) wr)->m_topic->m_stopic;
void *sample = ddsi_sertopic_alloc_sample (tp);
ddsi_serdata_topicless_to_sample (tp, tk->m_sample, sample, NULL, NULL);
ddsi_tkmap_instance_unref (tk);
ret = dds_dispose_impl (wr, sample, handle, timestamp);
ddsi_sertopic_free_sample (tp, sample, DDS_FREE_ALL);
} else { } else {
DDS_ERROR("No instance related with the provided handle is found\n"); DDS_ERROR("No instance related with the provided handle is found\n");
ret = DDS_ERRNO(DDS_RETCODE_PRECONDITION_NOT_MET); ret = DDS_ERRNO(DDS_RETCODE_PRECONDITION_NOT_MET);
@ -402,7 +405,6 @@ dds_dispose_ih_ts(
if (asleep) { if (asleep) {
thread_state_asleep(thr); thread_state_asleep(thr);
} }
ddsi_sertopic_free_sample (topic->m_stopic, sample, DDS_FREE_ALL);
dds_writer_unlock(wr); dds_writer_unlock(wr);
} else { } else {
DDS_ERROR("Error occurred on locking writer\n"); DDS_ERROR("Error occurred on locking writer\n");
@ -461,14 +463,14 @@ _Pre_satisfies_(entity & DDS_ENTITY_KIND_MASK)
dds_return_t dds_return_t
dds_instance_get_key( dds_instance_get_key(
dds_entity_t entity, dds_entity_t entity,
dds_instance_handle_t inst, dds_instance_handle_t ih,
void *data) void *data)
{ {
struct thread_state1 * const thr = lookup_thread_state(); struct thread_state1 * const thr = lookup_thread_state();
const bool asleep = !vtime_awake_p(thr->vtime); const bool asleep = !vtime_awake_p(thr->vtime);
dds_return_t ret; dds_return_t ret;
const dds_topic * topic; const dds_topic * topic;
struct ddsi_tkmap * map = gv.m_tkmap; struct ddsi_tkmap_instance * tk;
if(data == NULL){ if(data == NULL){
DDS_ERROR("Argument data is NULL\n"); DDS_ERROR("Argument data is NULL\n");
@ -482,13 +484,15 @@ dds_instance_get_key(
ret = DDS_ERRNO(DDS_RETCODE_BAD_PARAMETER); ret = DDS_ERRNO(DDS_RETCODE_BAD_PARAMETER);
goto err; goto err;
} }
ddsi_sertopic_zero_sample (topic->m_stopic, data);
if (asleep) { if (asleep) {
thread_state_awake(thr); thread_state_awake(thr);
} }
if (ddsi_tkmap_get_key (map, topic->m_stopic, inst, data)) { if ((tk = ddsi_tkmap_find_by_id(gv.m_tkmap, ih)) != NULL) {
ddsi_sertopic_zero_sample (topic->m_stopic, data);
ddsi_serdata_topicless_to_sample (topic->m_stopic, tk->m_sample, data, NULL, NULL);
ddsi_tkmap_instance_unref (tk);
ret = DDS_RETCODE_OK; ret = DDS_RETCODE_OK;
} else{ } else {
DDS_ERROR("No instance related with the provided entity is found\n"); DDS_ERROR("No instance related with the provided entity is found\n");
ret = DDS_ERRNO(DDS_RETCODE_BAD_PARAMETER); ret = DDS_ERRNO(DDS_RETCODE_BAD_PARAMETER);
} }

1
src/core/ddsi/include/ddsi/ddsi_tkmap.h
View file

@ -36,7 +36,6 @@ struct ddsi_tkmap * ddsi_tkmap_new (void);
void ddsi_tkmap_free (_Inout_ _Post_invalid_ struct ddsi_tkmap *tkmap); void ddsi_tkmap_free (_Inout_ _Post_invalid_ struct ddsi_tkmap *tkmap);
void ddsi_tkmap_instance_ref (_In_ struct ddsi_tkmap_instance *tk); void ddsi_tkmap_instance_ref (_In_ struct ddsi_tkmap_instance *tk);
uint64_t ddsi_tkmap_lookup (_In_ struct ddsi_tkmap *tkmap, _In_ const struct ddsi_serdata *serdata); uint64_t ddsi_tkmap_lookup (_In_ struct ddsi_tkmap *tkmap, _In_ const struct ddsi_serdata *serdata);
_Check_return_ bool ddsi_tkmap_get_key (_In_ struct ddsi_tkmap * map, const struct ddsi_sertopic *topic, _In_ uint64_t iid, _Out_ void * sample);
_Check_return_ struct ddsi_tkmap_instance * ddsi_tkmap_find( _Check_return_ struct ddsi_tkmap_instance * ddsi_tkmap_find(
_In_ struct ddsi_serdata * sd, _In_ struct ddsi_serdata * sd,
_In_ const bool rd, _In_ const bool rd,

74
src/core/ddsi/src/ddsi_tkmap.c
View file

@ -82,7 +82,7 @@ static int dds_tk_equals_void (const void *a, const void *b)
return dds_tk_equals (a, b); return dds_tk_equals (a, b);
} }
struct ddsi_tkmap * ddsi_tkmap_new (void) struct ddsi_tkmap *ddsi_tkmap_new (void)
{ {
struct ddsi_tkmap *tkmap = dds_alloc (sizeof (*tkmap)); struct ddsi_tkmap *tkmap = dds_alloc (sizeof (*tkmap));
tkmap->m_hh = ut_chhNew (1, dds_tk_hash_void, dds_tk_equals_void, gc_buckets); tkmap->m_hh = ut_chhNew (1, dds_tk_hash_void, dds_tk_equals_void, gc_buckets);
@ -111,64 +111,32 @@ uint64_t ddsi_tkmap_lookup (_In_ struct ddsi_tkmap * map, _In_ const struct ddsi
{ {
struct ddsi_tkmap_instance dummy; struct ddsi_tkmap_instance dummy;
struct ddsi_tkmap_instance * tk; struct ddsi_tkmap_instance * tk;
assert (vtime_awake_p(lookup_thread_state()->vtime));
dummy.m_sample = (struct ddsi_serdata *) sd; dummy.m_sample = (struct ddsi_serdata *) sd;
tk = ut_chhLookup (map->m_hh, &dummy); tk = ut_chhLookup (map->m_hh, &dummy);
return (tk) ? tk->m_iid : DDS_HANDLE_NIL; return (tk) ? tk->m_iid : DDS_HANDLE_NIL;
} }
typedef struct
{
const struct ddsi_sertopic *topic;
uint64_t m_iid;
void * m_sample;
bool m_ret;
}
tkmap_get_key_arg;
static void dds_tkmap_get_key_fn (void * vtk, void * varg)
{
struct ddsi_tkmap_instance * tk = vtk;
tkmap_get_key_arg * arg = (tkmap_get_key_arg*) varg;
if (tk->m_iid == arg->m_iid)
{
ddsi_serdata_topicless_to_sample (arg->topic, tk->m_sample, arg->m_sample, 0, 0);
arg->m_ret = true;
}
}
_Check_return_ _Check_return_
bool ddsi_tkmap_get_key (_In_ struct ddsi_tkmap * map, const struct ddsi_sertopic *topic, _In_ uint64_t iid, _Out_ void * sample) struct ddsi_tkmap_instance *ddsi_tkmap_find_by_id (_In_ struct ddsi_tkmap *map, _In_ uint64_t iid)
{ {
tkmap_get_key_arg arg = { topic, iid, sample, false }; /* This is not a function that should be used liberally, as it linearly scans the key-to-iid map. */
os_mutexLock (&map->m_lock); struct ut_chhIter it;
ut_chhEnumUnsafe (map->m_hh, dds_tkmap_get_key_fn, &arg); struct ddsi_tkmap_instance *tk;
os_mutexUnlock (&map->m_lock); uint32_t refc;
return arg.m_ret; assert (vtime_awake_p(lookup_thread_state()->vtime));
} for (tk = ut_chhIterFirst (map->m_hh, &it); tk; tk = ut_chhIterNext (&it))
if (tk->m_iid == iid)
typedef struct break;
{ if (tk == NULL)
uint64_t m_iid; /* Common case of it not existing at all */
struct ddsi_tkmap_instance * m_inst; return NULL;
} else if (!((refc = os_atomic_ld32 (&tk->m_refc)) & REFC_DELETE) && os_atomic_cas32 (&tk->m_refc, refc, refc+1))
tkmap_get_inst_arg; /* Common case of it existing, not in the process of being deleted and no one else concurrently modifying the refcount */
return tk;
static void dds_tkmap_get_inst_fn (void * vtk, void * varg) else
{ /* Let key value lookup handle the possible CAS loop and the complicated cases */
struct ddsi_tkmap_instance * tk = vtk; return ddsi_tkmap_find (tk->m_sample, false, false);
tkmap_get_inst_arg * arg = (tkmap_get_inst_arg*) varg;
if (tk->m_iid == arg->m_iid)
{
arg->m_inst = tk;
}
}
_Check_return_
struct ddsi_tkmap_instance * ddsi_tkmap_find_by_id (_In_ struct ddsi_tkmap * map, _In_ uint64_t iid)
{
tkmap_get_inst_arg arg = { iid, NULL };
ut_chhEnumUnsafe (map->m_hh, dds_tkmap_get_inst_fn, &arg);
return arg.m_inst;
} }
/* Debug keyhash generation for debug and coverage builds */ /* Debug keyhash generation for debug and coverage builds */
@ -193,6 +161,7 @@ struct ddsi_tkmap_instance * ddsi_tkmap_find(
struct ddsi_tkmap_instance * tk; struct ddsi_tkmap_instance * tk;
struct ddsi_tkmap * map = gv.m_tkmap; struct ddsi_tkmap * map = gv.m_tkmap;
assert (vtime_awake_p(lookup_thread_state()->vtime));
dummy.m_sample = sd; dummy.m_sample = sd;
retry: retry:
if ((tk = ut_chhLookup(map->m_hh, &dummy)) != NULL) if ((tk = ut_chhLookup(map->m_hh, &dummy)) != NULL)
@ -242,7 +211,6 @@ retry:
_Check_return_ _Check_return_
struct ddsi_tkmap_instance * ddsi_tkmap_lookup_instance_ref (_In_ struct ddsi_serdata * sd) struct ddsi_tkmap_instance * ddsi_tkmap_lookup_instance_ref (_In_ struct ddsi_serdata * sd)
{ {
assert (vtime_awake_p (lookup_thread_state ()->vtime));
return ddsi_tkmap_find (sd, true, true); return ddsi_tkmap_find (sd, true, true);
} }