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

Address locking order for entity locks

Browse source 
There were inconsistencies in the order in which entity locks were taken
when multiple entities needed to be locked at the same time.  In most
cases, the order was first locking entity X, then locking the parent
entity of X.  However, in some cases the order was reversed, a likely
cause of deadlocks.

This commit sorts these problems, and in particular propagating
operations into children.  The entity refcount is now part of the handle
administration so that it is no longer necessary to lock an entity to
determine whether it is still allowed to be used (previously it had to
check the CLOSED flag afterward).  This allows recursing into the
children while holding handles and the underlying objects alive, but
without violating lock order.

Attendant changes that would warrant there own commits but are too hard
to split off:

* Children are now no longer in a singly linked list, but in an AVL
  tree; this was necessary at some intermediate stage to allow unlocking
  an entity and restarting iteration over all children at the "next"
  child (all thanks to the eternally unique instance handle);

* Waitsets shifted to using arrays of attached entities instead of
  linked lists; this was a consequence of dealing with some locking
  issues in reading triggers and considering which operations on the
  "triggered" and "observed" sets are actually needed.

* Entity status flags and waitset/condition trigger counts are now
  handled using atomic operations.  Entities are now classified as
  having a "status" with a corresponding mask, or as having a "trigger
  count" (conditions).  As there are fewer than 16 status bits, the
  status and its mask can squeeze into the same 32-bits as the trigger
  count.  These atomic updates avoid the need for a separate lock just
  for the trigger/status values and results in a significant speedup
  with waitsets.

* Create topic now has a more rational behaviour when multiple
  participants attempt to create the same topic: each participant now
  gets its own topic definition, but the underlying type representation
  is shared.

Signed-off-by: Erik Boasson <eb@ilities.com>
This commit is contained in:
Erik Boasson 2019-06-21 07:13:57 +02:00 • committed by eboasson
parent f61b2d54da
commit 647f7466d6
26 changed files with 1224 additions and 1004 deletions
Download patch file Download diff file Expand all files Collapse all files

4
src/core/ddsi/include/dds/ddsi/ddsi_sertopic.h
View file

@ -29,7 +29,6 @@ typedef void (*topic_cb_t) (struct dds_topic * topic);
struct ddsi_sertopic_ops;
struct ddsi_sertopic {
ddsrt_avl_node_t avlnode; /* index on name_typename */
const struct ddsi_sertopic_ops *ops;
const struct ddsi_serdata_ops *serdata_ops;
uint32_t serdata_basehash;
@ -38,9 +37,6 @@ struct ddsi_sertopic {
char *type_name;
uint64_t iid;
ddsrt_atomic_uint32_t refc; /* counts refs from entities, not from data */
topic_cb_t status_cb;
struct dds_topic * status_cb_entity;
};
/* Called when the refcount dropped to zero */

35
src/core/ddsi/src/q_entity.c
View file

@ -2063,19 +2063,12 @@ static void writer_qos_mismatch (struct writer * wr, dds_qos_policy_id_t reason)
{
/* When the reason is DDS_INVALID_QOS_POLICY_ID, it means that we compared
* readers/writers from different topics: ignore that. */
if (reason != DDS_INVALID_QOS_POLICY_ID)
if (reason != DDS_INVALID_QOS_POLICY_ID && wr->status_cb)
{
if (wr->topic->status_cb) {
/* Handle INCONSISTENT_TOPIC on topic */
(wr->topic->status_cb) (wr->topic->status_cb_entity);
}
if (wr->status_cb)
{
status_cb_data_t data;
data.raw_status_id = (int) DDS_OFFERED_INCOMPATIBLE_QOS_STATUS_ID;
data.extra = reason;
(wr->status_cb) (wr->status_cb_entity, &data);
}
status_cb_data_t data;
data.raw_status_id = (int) DDS_OFFERED_INCOMPATIBLE_QOS_STATUS_ID;
data.extra = reason;
(wr->status_cb) (wr->status_cb_entity, &data);
}
}
@ -2083,20 +2076,12 @@ static void reader_qos_mismatch (struct reader * rd, dds_qos_policy_id_t reason)
{
/* When the reason is DDS_INVALID_QOS_POLICY_ID, it means that we compared
* readers/writers from different topics: ignore that. */
if (reason != DDS_INVALID_QOS_POLICY_ID)
if (reason != DDS_INVALID_QOS_POLICY_ID && rd->status_cb)
{
if (rd->topic->status_cb)
{
/* Handle INCONSISTENT_TOPIC on topic */
(rd->topic->status_cb) (rd->topic->status_cb_entity);
}
if (rd->status_cb)
{
status_cb_data_t data;
data.raw_status_id = (int) DDS_REQUESTED_INCOMPATIBLE_QOS_STATUS_ID;
data.extra = reason;
(rd->status_cb) (rd->status_cb_entity, &data);
}
status_cb_data_t data;
data.raw_status_id = (int) DDS_REQUESTED_INCOMPATIBLE_QOS_STATUS_ID;
data.extra = reason;
(rd->status_cb) (rd->status_cb_entity, &data);
}
}