[PATCH 1/2] genl: rework l_genl_family to avoid extra lookups

[PATCH 1/2] genl: rework l_genl_family to avoid extra lookups

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 19939 bytes --]

Instead of looking up and re-allocating an entirely new l_genl_family
with each call to l_genl_family_new, we can use a previous lookup that
matches our family name.

A new internal structure was introduced, genl_parent, which holds all
the lookup information for a given family name. Now, when
l_genl_famil_new is called, we allocate a smaller child structure
(l_genl_family) which holds a pointer back to the parent object in order
to send and receive messages. This child structure only holds callback
information and the (new) group ID (gid).

Messages are now tagged with child group ID which allows for cancelling
all requets on a given child object without affecting other children of
the same parent. Cancelling all requets on a child object is as simple
as destroying the l_genl_family object (l_genl_family_unref).
---
 ell/genl.c | 369 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 245 insertions(+), 124 deletions(-)

diff --git a/ell/genl.c b/ell/genl.c
index 3a0281a..1dfdf1f 100644
--- a/ell/genl.c
+++ b/ell/genl.c
@@ -63,8 +63,9 @@ struct l_genl {
 	struct l_queue *notify_list;
 	unsigned int next_request_id;
 	unsigned int next_notify_id;
+	unsigned int next_gid;
 	struct l_queue *family_list;
-	struct l_genl_family *nlctrl;
+	struct genl_parent *nlctrl;
 	struct genl_unicast_notify *unicast_notify;
 	l_genl_debug_func_t debug_callback;
 	l_genl_destroy_func_t debug_destroy;
@@ -85,6 +86,7 @@ struct l_genl_msg {
 
 struct genl_request {
 	unsigned int id;
+	unsigned int gid;
 	uint16_t type;
 	uint16_t flags;
 	uint32_t seq;
@@ -114,8 +116,7 @@ struct genl_mcast {
 	unsigned int users;
 };
 
-struct l_genl_family {
-	int ref_count;
+struct genl_parent {
 	struct l_genl *genl;
 	char name[GENL_NAMSIZ];
 	uint16_t id;
@@ -124,11 +125,19 @@ struct l_genl_family {
 	uint32_t maxattr;
 	struct l_queue *op_list;
 	struct l_queue *mcast_list;
+	unsigned int nlctrl_cmd;
+	struct l_queue *children;
+	bool live;
+};
+
+struct l_genl_family {
+	struct genl_parent *parent;
+	int ref_count;
+	unsigned int gid;
 	l_genl_watch_func_t watch_appeared;
 	l_genl_watch_func_t watch_vanished;
 	l_genl_destroy_func_t watch_destroy;
 	void *watch_data;
-	unsigned int nlctrl_cmd;
 };
 
 static void destroy_request(void *data)
@@ -153,20 +162,31 @@ static void destroy_notify(void *data)
 	l_free(notify);
 }
 
-static struct l_genl_family *family_alloc(struct l_genl *genl,
-							const char *name)
+static struct l_genl_family *family_alloc_child(struct genl_parent *parent)
 {
-	struct l_genl_family *family;
+	struct l_genl_family *family = l_new(struct l_genl_family, 1);
 
-	family = l_new(struct l_genl_family, 1);
+	family->parent = parent;
+	family->gid = parent->genl->next_gid++;
 
-	family->genl = genl;
-	strncpy(family->name, name, GENL_NAMSIZ);
+	l_queue_push_tail(parent->children, family);
 
-	family->op_list = l_queue_new();
-	family->mcast_list = l_queue_new();
+	return family;
+}
 
-	return l_genl_family_ref(family);
+static struct genl_parent *family_alloc_parent(struct l_genl *genl,
+							const char *name)
+{
+	struct genl_parent *parent = l_new(struct genl_parent, 1);
+
+	parent->genl = genl;
+	strncpy(parent->name, name, GENL_NAMSIZ);
+
+	parent->op_list = l_queue_new();
+	parent->mcast_list = l_queue_new();
+	parent->children = l_queue_new();
+
+	return parent;
 }
 
 static void op_free(void *data)
@@ -191,16 +211,63 @@ static void mcast_free(void *data, void *user_data)
 	l_free(mcast);
 }
 
-static void family_free(void *data)
+static bool request_match_gid(const void *a, const void *b)
 {
-	struct l_genl_family *family = data;
+	const struct genl_request *request = a;
+	unsigned int gid = L_PTR_TO_UINT(b);
+
+	if (request->gid == gid)
+		return true;
+
+	return false;
+}
+
+static void cancel_all_requests(struct l_genl_family *child)
+{
+	struct l_genl *genl = child->parent->genl;
+	struct genl_request *request;
+
+	/* genl has already been cleaned up */
+	if (!genl->request_queue)
+		return;
+
+	while ((request = l_queue_find(genl->request_queue, request_match_gid,
+					L_UINT_TO_PTR(child->gid))))
+		l_genl_family_cancel(child, request->id);
+}
+
+static void child_free(void *data)
+{
+	struct l_genl_family *child = data;
+
+	cancel_all_requests(child);
+
+	if (child->parent->id > 0 && child->watch_vanished)
+		child->watch_vanished(child->watch_data);
+
+	if (child->watch_destroy)
+		child->watch_destroy(child->watch_data);
+
+
+	l_free(child);
+}
+
+static void parent_free(void *data)
+{
+	struct genl_parent *parent = data;
+
+	l_queue_destroy(parent->children, child_free);
+
+	l_queue_destroy(parent->op_list, op_free);
 
-	family->genl = NULL;
+	l_queue_foreach(parent->mcast_list, mcast_free, parent->genl);
+	l_queue_destroy(parent->mcast_list, NULL);
+	parent->mcast_list = NULL;
 
-	l_genl_family_unref(family);
+	l_free(parent);
 }
 
-static void family_add_op(struct l_genl_family *family, uint32_t id,
+static void family_add_op(struct genl_parent *parent, uint32_t id,
 							uint32_t flags)
 {
 	struct genl_op *op;
@@ -210,7 +277,7 @@ static void family_add_op(struct l_genl_family *family, uint32_t id,
 	op->id = id;
 	op->flags = flags;
 
-	l_queue_push_tail(family->op_list, op);
+	l_queue_push_tail(parent->op_list, op);
 }
 
 static bool match_mcast_name(const void *a, const void *b)
@@ -221,16 +288,16 @@ static bool match_mcast_name(const void *a, const void *b)
 	return !strcmp(mcast->name, name);
 }
 
-static void family_add_mcast(struct l_genl_family *family, const char *name,
+static void family_add_mcast(struct genl_parent *parent, const char *name,
 								uint32_t id)
 {
-	struct l_genl *genl = family->genl;
+	struct l_genl *genl = parent->genl;
 	struct genl_mcast *mcast;
 
 	if (!genl)
 		return;
 
-	mcast = l_queue_find(family->mcast_list, match_mcast_name,
+	mcast = l_queue_find(parent->mcast_list, match_mcast_name,
 							(char *) name);
 	if (mcast)
 		return;
@@ -241,7 +308,26 @@ static void family_add_mcast(struct l_genl_family *family, const char *name,
 	mcast->id = id;
 	mcast->users = 0;
 
-	l_queue_push_tail(family->mcast_list, mcast);
+	l_queue_push_tail(parent->mcast_list, mcast);
+}
+
+/* special initializer for the nlctrl object */
+static struct genl_parent *family_alloc_ctrl(struct l_genl *genl)
+{
+	struct genl_parent *nlctrl;
+
+	nlctrl = l_new(struct genl_parent, 1);
+
+	nlctrl->genl = genl;
+	strncpy(nlctrl->name, "nlctrl", GENL_NAMSIZ);
+
+	nlctrl->op_list = l_queue_new();
+	nlctrl->mcast_list = l_queue_new();
+	nlctrl->id = GENL_ID_CTRL;
+
+	family_add_mcast(nlctrl, "notify", GENL_ID_CTRL);
+
+	return nlctrl;
 }
 
 static struct l_genl_msg *msg_alloc(uint8_t cmd, uint8_t version, uint32_t size)
@@ -549,13 +635,7 @@ LIB_EXPORT struct l_genl *l_genl_new(int fd)
 	genl->fd = fd;
 	genl->close_on_unref = false;
 
-	genl->nlctrl = family_alloc(genl, "nlctrl");
-
-	genl->nlctrl->id = GENL_ID_CTRL;
-
-	family_add_mcast(genl->nlctrl, "notify", GENL_ID_CTRL);
-
-	l_queue_push_tail(genl->family_list, genl->nlctrl);
+	genl->nlctrl = family_alloc_ctrl(genl);
 
 	genl->io = l_io_new(genl->fd);
 
@@ -632,6 +712,7 @@ LIB_EXPORT void l_genl_unref(struct l_genl *genl)
 	l_queue_destroy(genl->notify_list, destroy_notify);
 	l_queue_destroy(genl->pending_list, destroy_request);
 	l_queue_destroy(genl->request_queue, destroy_request);
+	genl->request_queue = NULL;
 
 	l_io_set_write_handler(genl->io, NULL, NULL, NULL);
 	l_io_set_read_handler(genl->io, NULL, NULL, NULL);
@@ -639,9 +720,9 @@ LIB_EXPORT void l_genl_unref(struct l_genl *genl)
 	l_io_destroy(genl->io);
 	genl->io = NULL;
 
-	l_genl_family_unref(genl->nlctrl);
+	parent_free(genl->nlctrl);
 
-	l_queue_destroy(genl->family_list, family_free);
+	l_queue_destroy(genl->family_list, parent_free);
 
 	if (genl->close_on_unref)
 		close(genl->fd);
@@ -984,7 +1065,7 @@ LIB_EXPORT bool l_genl_attr_recurse(struct l_genl_attr *attr,
 	return true;
 }
 
-static void family_ops(struct l_genl_family *family, struct l_genl_attr *attr)
+static void family_ops(struct genl_parent *parent, struct l_genl_attr *attr)
 {
 	uint16_t type, len;
 	const void *data;
@@ -1007,11 +1088,11 @@ static void family_ops(struct l_genl_family *family, struct l_genl_attr *attr)
 		}
 
 		if (id > 0)
-			family_add_op(family, id, flags);
+			family_add_op(parent, id, flags);
 	}
 }
 
-static void family_mcast_groups(struct l_genl_family *family,
+static void family_mcast_groups(struct genl_parent *parent,
 						struct l_genl_attr *attr)
 {
 	uint16_t type, len;
@@ -1036,21 +1117,30 @@ static void family_mcast_groups(struct l_genl_family *family,
 		}
 
 		if (name && id > 0)
-			family_add_mcast(family, name, id);
+			family_add_mcast(parent, name, id);
 	}
 }
 
+static void family_notify_appeared(void *data, void *user_data)
+{
+	struct l_genl_family *family = data;
+
+	if (family->watch_appeared)
+		family->watch_appeared(family->watch_data);
+}
+
 static void get_family_callback(struct l_genl_msg *msg, void *user_data)
 {
 	struct l_genl_family *family = user_data;
+	struct genl_parent *parent = family->parent;
 	struct l_genl_attr attr, nested;
 	uint16_t type, len;
 	const void *data;
 	int error;
 
-	family->nlctrl_cmd = 0;
+	parent->nlctrl_cmd = 0;
 
-	if (family->id > 0)
+	if (parent->id > 0)
 		return;
 
 	error = l_genl_msg_get_error(msg);
@@ -1066,38 +1156,86 @@ static void get_family_callback(struct l_genl_msg *msg, void *user_data)
 	while (l_genl_attr_next(&attr, &type, &len, &data)) {
 		switch (type) {
 		case CTRL_ATTR_FAMILY_ID:
-			family->id = *((uint16_t *) data);
+			parent->id = *((uint16_t *) data);
 			break;
 		case CTRL_ATTR_FAMILY_NAME:
-			strncpy(family->name, data, GENL_NAMSIZ);
+			strncpy(parent->name, data, GENL_NAMSIZ);
 			break;
 		case CTRL_ATTR_VERSION:
-			family->version = *((uint32_t *) data);
+			parent->version = *((uint32_t *) data);
 			break;
 		case CTRL_ATTR_HDRSIZE:
-			family->hdrsize = *((uint32_t *) data);
+			parent->hdrsize = *((uint32_t *) data);
 			break;
 		case CTRL_ATTR_MAXATTR:
-			family->maxattr = *((uint32_t *) data);
+			parent->maxattr = *((uint32_t *) data);
 			break;
 		case CTRL_ATTR_OPS:
 			if (l_genl_attr_recurse(&attr, &nested))
-				family_ops(family, &nested);
+				family_ops(parent, &nested);
 			break;
 		case CTRL_ATTR_MCAST_GROUPS:
 			if (l_genl_attr_recurse(&attr, &nested))
-				family_mcast_groups(family, &nested);
+				family_mcast_groups(parent, &nested);
 			break;
 		}
 	}
 
-	if (family->watch_appeared)
-		family->watch_appeared(family->watch_data);
+	parent->live = true;
+
+	l_queue_foreach(parent->children, family_notify_appeared, NULL);
+}
+
+static bool match_family_name(const void *a, const void *b)
+{
+	const struct genl_parent *parent = a;
+
+	return !strncmp(parent->name, (const char *)b, GENL_NAMSIZ);
+}
+
+static unsigned int send_common(struct genl_parent *parent, uint16_t flags,
+				struct l_genl_msg *msg, unsigned int gid,
+				l_genl_msg_func_t callback, void *user_data,
+				l_genl_destroy_func_t destroy)
+{
+	struct l_genl *genl;
+	struct genl_request *request;
+
+	if (!parent || !msg)
+		return 0;
+
+	genl = parent->genl;
+	if (!genl)
+		return 0;
+
+	request = l_new(struct genl_request, 1);
+
+	request->type = parent->id;
+	request->flags = NLM_F_REQUEST | flags;
+
+	request->msg = msg;
+	request->gid = gid;
+
+	request->callback = callback;
+	request->destroy = destroy;
+	request->user_data = user_data;
+
+	if (genl->next_request_id < 1)
+		genl->next_request_id = 1;
+
+	request->id = genl->next_request_id++;
+
+	l_queue_push_tail(genl->request_queue, request);
+
+	wakeup_writer(genl);
+
+	return request->id;
 }
 
 LIB_EXPORT struct l_genl_family *l_genl_family_new(struct l_genl *genl,
 							const char *name)
 {
+	struct genl_parent *parent;
 	struct l_genl_family *family;
 	struct l_genl_msg *msg;
 
@@ -1105,26 +1243,38 @@ LIB_EXPORT struct l_genl_family *l_genl_family_new(struct l_genl *genl,
 			unlikely(strlen(name) >= GENL_NAMSIZ))
 		return NULL;
 
-	family = family_alloc(genl, name);
-	if (!family)
+	parent = l_queue_find(genl->family_list, match_family_name, name);
+	if (parent) {
+		family = family_alloc_child(parent);
+		goto done;
+	}
+
+	parent = family_alloc_parent(genl, name);
+
+	if (!parent)
 		return NULL;
 
+	family = family_alloc_child(parent);
+
+	/* family has not yet been looked up */
 	msg = l_genl_msg_new_sized(CTRL_CMD_GETFAMILY, NLA_HDRLEN + GENL_NAMSIZ);
 
 	l_genl_msg_append_attr(msg, CTRL_ATTR_FAMILY_NAME,
-						GENL_NAMSIZ, family->name);
+						GENL_NAMSIZ, parent->name);
 
-	family->nlctrl_cmd = l_genl_family_send(genl->nlctrl, msg,
-					get_family_callback, family, NULL);
+	parent->nlctrl_cmd = send_common(genl->nlctrl, NLM_F_ACK, msg, 0,
+						get_family_callback, family,
+						NULL);
 
-	if (!family->nlctrl_cmd) {
-		family_free(family);
+	if (!parent->nlctrl_cmd) {
+		parent_free(parent);
 		return NULL;
 	}
 
-	l_queue_push_tail(genl->family_list, family);
+	l_queue_push_tail(genl->family_list, parent);
 
-	return family;
+done:
+	return l_genl_family_ref(family);
 }
 
 LIB_EXPORT struct l_genl_family *l_genl_family_ref(
@@ -1141,34 +1291,35 @@ LIB_EXPORT struct l_genl_family *l_genl_family_ref(
 LIB_EXPORT void l_genl_family_unref(struct l_genl_family *family)
 {
 	struct l_genl *genl;
+	struct genl_parent *parent;
 
 	if (unlikely(!family))
 		return;
 
+	parent = family->parent;
+
 	if (__sync_sub_and_fetch(&family->ref_count, 1))
 		return;
 
-	if (family->nlctrl_cmd > 0)
-		l_genl_family_cancel(family, family->nlctrl_cmd);
-
-	genl = family->genl;
-	if (genl)
-		l_queue_remove(genl->family_list, family);
-
-	l_queue_destroy(family->op_list, op_free);
-
-	l_queue_foreach(family->mcast_list, mcast_free, genl);
+	if (!l_queue_remove(parent->children, family))
+		return;
 
-	l_queue_destroy(family->mcast_list, NULL);
-	family->mcast_list = NULL;
+	/* parent still has children, just free this child instance */
+	if (!l_queue_isempty(parent->children)) {
+		child_free(family);
+		return;
+	}
 
-	if (family->id > 0 && family->watch_vanished)
-		family->watch_vanished(family->watch_data);
+	if (parent->nlctrl_cmd > 0)
+		l_genl_family_cancel(family, parent->nlctrl_cmd);
 
-	if (family->watch_destroy)
-		family->watch_destroy(family->watch_data);
+	genl = parent->genl;
+	if (genl)
+		l_queue_remove(genl->family_list, parent);
 
-	l_free(family);
+	/* child is no longer in 'children' list, free manually */
+	child_free(family);
+	parent_free(parent);
 }
 
 LIB_EXPORT bool l_genl_family_set_watches(struct l_genl_family *family,
@@ -1188,6 +1339,10 @@ LIB_EXPORT bool l_genl_family_set_watches(struct l_genl_family *family,
 	family->watch_destroy = destroy;
 	family->watch_data = user_data;
 
+	/* if the parent has already been looked up, we notify immediately */
+	if (family->parent->live)
+		family->watch_appeared(family->watch_data);
+
 	return true;
 }
 
@@ -1196,7 +1351,7 @@ LIB_EXPORT uint32_t l_genl_family_get_version(struct l_genl_family *family)
 	if (unlikely(!family))
 		return 0;
 
-	return family->version;
+	return family->parent->version;
 }
 
 LIB_EXPORT struct l_genl *l_genl_family_get_genl(struct l_genl_family *family)
@@ -1204,7 +1359,7 @@ LIB_EXPORT struct l_genl *l_genl_family_get_genl(struct l_genl_family *family)
 	if (unlikely(!family))
 		return 0;
 
-	return family->genl;
+	return family->parent->genl;
 }
 
 static bool match_op_id(const void *a, const void *b)
@@ -1223,7 +1378,8 @@ LIB_EXPORT bool l_genl_family_can_send(struct l_genl_family *family,
 	if (unlikely(!family))
 		return false;
 
-	op = l_queue_find(family->op_list, match_op_id, L_UINT_TO_PTR(cmd));
+	op = l_queue_find(family->parent->op_list, match_op_id,
+				L_UINT_TO_PTR(cmd));
 	if (!op)
 		return false;
 
@@ -1241,7 +1397,8 @@ LIB_EXPORT bool l_genl_family_can_dump(struct l_genl_family *family,
 	if (!family)
 		return false;
 
-	op = l_queue_find(family->op_list, match_op_id, L_UINT_TO_PTR(cmd));
+	op = l_queue_find(family->parent->op_list, match_op_id,
+				L_UINT_TO_PTR(cmd));
 	if (!op)
 		return false;
 
@@ -1251,51 +1408,14 @@ LIB_EXPORT bool l_genl_family_can_dump(struct l_genl_family *family,
 	return false;
 }
 
-static unsigned int send_common(struct l_genl_family *family, uint16_t flags,
-				struct l_genl_msg *msg, l_genl_msg_func_t callback,
-				void *user_data, l_genl_destroy_func_t destroy)
-{
-	struct l_genl *genl;
-	struct genl_request *request;
-
-	if (!family || !msg)
-		return 0;
-
-	genl = family->genl;
-	if (!genl)
-		return 0;
-
-	request = l_new(struct genl_request, 1);
-
-	request->type = family->id;
-	request->flags = NLM_F_REQUEST | flags;
-
-	request->msg = msg;
-
-	request->callback = callback;
-	request->destroy = destroy;
-	request->user_data = user_data;
-
-	if (genl->next_request_id < 1)
-		genl->next_request_id = 1;
-
-	request->id = genl->next_request_id++;
-
-	l_queue_push_tail(genl->request_queue, request);
-
-	wakeup_writer(genl);
-
-	return request->id;
-}
-
 LIB_EXPORT unsigned int l_genl_family_send(struct l_genl_family *family,
 						struct l_genl_msg *msg,
 						l_genl_msg_func_t callback,
 						void *user_data,
 						l_genl_destroy_func_t destroy)
 {
-	return send_common(family, NLM_F_ACK, msg, callback,
-						user_data, destroy);
+	return send_common(family->parent, NLM_F_ACK, msg, family->gid,
+						callback, user_data, destroy);
 }
 
 LIB_EXPORT unsigned int l_genl_family_dump(struct l_genl_family *family,
@@ -1304,8 +1424,9 @@ LIB_EXPORT unsigned int l_genl_family_dump(struct l_genl_family *family,
 						void *user_data,
 						l_genl_destroy_func_t destroy)
 {
-	return send_common(family, NLM_F_ACK | NLM_F_DUMP, msg, callback,
-							user_data, destroy);
+	return send_common(family->parent, NLM_F_ACK | NLM_F_DUMP, msg,
+						family->gid, callback,
+						user_data, destroy);
 }
 
 static bool match_request_id(const void *a, const void *b)
@@ -1325,7 +1446,7 @@ LIB_EXPORT bool l_genl_family_cancel(struct l_genl_family *family,
 	if (unlikely(!family) || unlikely(!id))
 		return false;
 
-	genl = family->genl;
+	genl = family->parent->genl;
 	if (!genl)
 		return false;
 
@@ -1367,7 +1488,7 @@ LIB_EXPORT bool l_genl_family_has_group(struct l_genl_family *family,
 	if (unlikely(!family))
 		return false;
 
-	mcast = l_queue_find(family->mcast_list, match_mcast_name,
+	mcast = l_queue_find(family->parent->mcast_list, match_mcast_name,
 							(char *) group);
 	if (!mcast)
 		return false;
@@ -1388,18 +1509,18 @@ LIB_EXPORT unsigned int l_genl_family_register(struct l_genl_family *family,
 	if (unlikely(!family) || unlikely(!group))
 		return 0;
 
-	genl = family->genl;
+	genl = family->parent->genl;
 	if (!genl)
 		return 0;
 
-	mcast = l_queue_find(family->mcast_list, match_mcast_name,
+	mcast = l_queue_find(family->parent->mcast_list, match_mcast_name,
 							(char *) group);
 	if (!mcast)
 		return 0;
 
 	notify = l_new(struct genl_mcast_notify, 1);
 
-	notify->type = family->id;
+	notify->type = family->parent->id;
 	notify->group = mcast->id;
 
 	notify->callback = callback;
@@ -1435,7 +1556,7 @@ LIB_EXPORT bool l_genl_family_unregister(struct l_genl_family *family,
 	if (!family || !id)
 		return false;
 
-	genl = family->genl;
+	genl = family->parent->genl;
 	if (!genl)
 		return false;
 
-- 
2.17.1

[PATCH 2/2] unit: test-genl:

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 7754 bytes --]

Added more unit tests to exercise new parent/child structure in genl
---
 unit/test-genl.c | 247 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 240 insertions(+), 7 deletions(-)

diff --git a/unit/test-genl.c b/unit/test-genl.c
index a381e25..5b53ba8 100644
--- a/unit/test-genl.c
+++ b/unit/test-genl.c
@@ -36,6 +36,11 @@ struct test_data
 
 	unsigned int group_id;
 	bool vanished_called;
+	bool destroyed_called;
+	bool appeared_called;
+	int message_destroy;
+
+	int idle_count;
 };
 
 static void do_debug(const char *str, void *user_data)
@@ -117,7 +122,6 @@ static bool check_test_data(struct test_data *data)
 static void idle_callback(struct l_idle *idle, void *user_data)
 {
 	struct test_data *data = user_data;
-	static int count = 0;
 
 	/*
 	 * Exit the event loop if the desired results have been
@@ -129,7 +133,7 @@ static void idle_callback(struct l_idle *idle, void *user_data)
 	 * the generic netlink watches and family registration to be
 	 * called and completed, respectively.
 	 */
-	if (check_test_data(data) || ++count > 3)
+	if (check_test_data(data) || ++data->idle_count > 3)
 		l_main_quit();
 }
 
@@ -138,21 +142,22 @@ static bool destroy_test_data(struct test_data *data)
 	bool unregistered =
 			l_genl_family_unregister(data->appeared_family,
 							data->group_id);
-
 	l_genl_family_unref(data->vanished_family);
 	l_genl_family_unref(data->appeared_family);
 
 	return unregistered;
 }
 
-int main(int argc, char *argv[])
+static void test_basic(const void *user_data)
 {
 	struct l_genl *genl;
 	struct l_idle *idle;
 	struct test_data data = { .group_id = 0 };
 
-	if (!l_main_init())
-		return -1;
+	if (!l_main_init()) {
+		assert(false);
+		return;
+	}
 
 	l_log_set_stderr();
 
@@ -177,6 +182,234 @@ int main(int argc, char *argv[])
 	l_genl_unref(genl);
 
 	l_main_exit();
+}
+
+static void test_vanished_called(void *user_data)
+{
+	struct test_data *td = user_data;
+
+	td->vanished_called = true;
+}
+
+static void test_destroyed_called(void *user_data)
+{
+	struct test_data *td = user_data;
+
+	td->destroyed_called = true;
+}
+
+static void test_appeared_called(void *user_data)
+{
+	struct test_data *td = user_data;
+
+	td->appeared_called = true;
+}
+
+static void test_ref_unref(const void *user_data)
+{
+	struct l_genl *genl;
+	struct l_genl_family *family;
+	struct l_idle *idle;
+	struct test_data td = { 0 };
+
+	if (!l_main_init()) {
+		assert(false);
+		return;
+	}
+
+	genl = l_genl_new_default();
+
+	l_genl_set_debug(genl, do_debug, "[GENL] ", NULL);
+
+	family = l_genl_family_new(genl, "nlctrl");
+
+	l_genl_family_set_watches(family, NULL, test_vanished_called,
+					&td, test_destroyed_called);
+
+	idle = l_idle_create(idle_callback, &td, NULL);
+
+	l_main_run();
+
+	l_idle_remove(idle);
+
+	l_genl_family_ref(family);
+	l_genl_family_ref(family);
+	/* family is returned ref'ed, so we need an extra unref */
+	l_genl_family_unref(family);
+	l_genl_family_unref(family);
+	l_genl_family_unref(family);
+
+	assert(td.vanished_called);
+	assert(td.destroyed_called);
+
+	l_genl_unref(genl);
+
+	l_main_exit();
+}
+
+static void test_cleanup_before_lookup(const void *user_data)
+{
+	struct l_genl *genl;
+	struct l_genl_family *family;
+	struct test_data td = { 0 };
+
+	genl = l_genl_new_default();
+
+	l_genl_set_debug(genl, do_debug, "[GENL] ", NULL);
+
+	family = l_genl_family_new(genl, "nlctrl");
+
+	l_genl_family_set_watches(family, NULL, test_vanished_called,
+					&td, test_destroyed_called);
+	/* family is returned immediately but not yet looked up */
+	l_genl_family_unref(family);
+
+	/* vanished only called if 'appeared' (i.e. lookup completed) */
+	assert(!td.vanished_called);
+	assert(td.destroyed_called);
+
+	l_genl_unref(genl);
+}
+
+static void test_family_after_lookup(const void *user_data)
+{
+	struct l_genl *genl;
+	struct l_genl_family *family1;
+	struct l_genl_family *family2;
+	struct l_idle *idle;
+	struct test_data td1 = { 0 };
+	struct test_data td2 = { 0 };
+
+	if (!l_main_init()) {
+		assert(false);
+		return;
+	}
+
+	genl = l_genl_new_default();
+
+	l_genl_set_debug(genl, do_debug, "[GENL] ", NULL);
+
+	family1 = l_genl_family_new(genl, "nlctrl");
+
+	l_genl_family_set_watches(family1, test_appeared_called,
+					test_vanished_called,
+					&td1, test_destroyed_called);
+
+	idle = l_idle_create(idle_callback, &td1, NULL);
+
+	l_main_run();
+
+	l_idle_remove(idle);
+
+	assert(td1.appeared_called);
+
+	/* now that family has been looked up, it should appear immediately */
+	family2 = l_genl_family_new(genl, "nlctrl");
+	l_genl_family_set_watches(family2, test_appeared_called,
+					test_vanished_called,
+					&td2, test_destroyed_called);
+	assert(td2.appeared_called);
+
+	l_genl_unref(genl);
+
+	/* unref'ing genl should trigger removal of both families */
+	assert(td1.vanished_called && td1.destroyed_called);
+	assert(td2.vanished_called && td2.destroyed_called);
+
+	l_main_exit();
+}
+
+static void message_destroy(void *user_data)
+{
+	struct test_data *td = user_data;
+
+	td->message_destroy++;
+}
+
+static void test_cancel_all_request(const void *user_data)
+{
+	struct l_genl *genl;
+	struct l_genl_family *family1;
+	struct l_genl_family *family2;
+	struct l_genl_msg *msg1, *msg2, *msg3, *msg4;
+	unsigned int msg3_id;
+	struct l_idle *idle;
+
+	struct test_data td1 = { 0 };
+	struct test_data td2 = { 0 };
+
+	if (!l_main_init()) {
+		assert(false);
+		return;
+	}
+
+	genl = l_genl_new_default();
+
+	l_genl_set_debug(genl, do_debug, "[GENL] ", NULL);
+
+	family1 = l_genl_family_new(genl, "nlctrl");
+	family2 = l_genl_family_new(genl, "nlctrl");
+
+	l_genl_family_set_watches(family1, test_appeared_called,
+					test_vanished_called,
+					&td1, test_destroyed_called);
+	l_genl_family_set_watches(family2, test_appeared_called,
+					test_vanished_called,
+					&td2, test_destroyed_called);
+
+	idle = l_idle_create(idle_callback, &td1, NULL);
+
+	l_main_run();
+
+	l_idle_remove(idle);
+
+	assert(td1.appeared_called);
+	assert(td2.appeared_called);
+
+	/* create some dummy messages to send on both families */
+	msg1 = l_genl_msg_new(1);
+	msg2 = l_genl_msg_new(1);
+	msg3 = l_genl_msg_new(1);
+	msg4 = l_genl_msg_new(1);
+
+	l_genl_family_send(family1, msg1, NULL, &td1, message_destroy);
+	l_genl_family_send(family1, msg2, NULL, &td1, message_destroy);
+	msg3_id = l_genl_family_send(family2, msg3, NULL, &td2,
+					message_destroy);
+	l_genl_family_send(family2, msg4, NULL, &td2, message_destroy);
+
+	/* this should cancel msg1 and msg2 */
+	l_genl_family_unref(family1);
+
+	assert(td1.vanished_called);
+	assert(td1.destroyed_called);
+	assert(td1.message_destroy == 2);
+	assert(td2.message_destroy == 0);
+
+	/* cancel just message 3 */
+	l_genl_family_cancel(family2, msg3_id);
+	assert(td2.message_destroy == 1);
+
+	/* cancel all remaining (just message 4) */
+	l_genl_family_unref(family2);
+	assert(td2.vanished_called);
+	assert(td2.destroyed_called);
+	assert(td2.message_destroy == 2);
+
+	l_genl_unref(genl);
+
+	l_main_exit();
+}
+
+int main(int argc, char *argv[])
+{
+	l_test_init(&argc, &argv);
+
+	l_test_add("Basic", test_basic, NULL);
+	l_test_add("Ref/Unref", test_ref_unref, NULL);
+	l_test_add("Cleaup before lookup", test_cleanup_before_lookup, NULL);
+	l_test_add("New family after lookup", test_family_after_lookup, NULL);
+	l_test_add("Cancel all family requests", test_cancel_all_request, NULL);
 
-	return 0;
+	return l_test_run();
 }
-- 
2.17.1

Re: [PATCH 1/2] genl: rework l_genl_family to avoid extra lookups

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 8215 bytes --]

Hi James,

On 10/24/2018 03:52 PM, James Prestwood wrote:
> Instead of looking up and re-allocating an entirely new l_genl_family
> with each call to l_genl_family_new, we can use a previous lookup that
> matches our family name.
> 
> A new internal structure was introduced, genl_parent, which holds all
> the lookup information for a given family name. Now, when
> l_genl_famil_new is called, we allocate a smaller child structure
> (l_genl_family) which holds a pointer back to the parent object in order
> to send and receive messages. This child structure only holds callback
> information and the (new) group ID (gid).

So while I kind of understand why you picked 'genl_parent', I really 
wonder if that naming is confusing.  Netlink Family is an actual term 
used by the Kernel.  E.g. NETLINK_ROUTE is a netlink 'family'.  Think 
socket families.  NETLINK_GENERIC (what l_genl is) is also a netlink 
family, and it itself has families (l_genl_family).

So perhaps we should stick with some internal naming for this.  E.g. 
just family or genl_family or __genl_family or genl_family_internal, etc.

> 
> Messages are now tagged with child group ID which allows for cancelling
> all requets on a given child object without affecting other children of
> the same parent. Cancelling all requets on a child object is as simple
> as destroying the l_genl_family object (l_genl_family_unref).
> ---
>   ell/genl.c | 369 +++++++++++++++++++++++++++++++++++------------------
>   1 file changed, 245 insertions(+), 124 deletions(-)
> 
> diff --git a/ell/genl.c b/ell/genl.c
> index 3a0281a..1dfdf1f 100644
> --- a/ell/genl.c
> +++ b/ell/genl.c
> @@ -63,8 +63,9 @@ struct l_genl {
>   	struct l_queue *notify_list;
>   	unsigned int next_request_id;
>   	unsigned int next_notify_id;
> +	unsigned int next_gid;
>   	struct l_queue *family_list;
> -	struct l_genl_family *nlctrl;
> +	struct genl_parent *nlctrl;
>   	struct genl_unicast_notify *unicast_notify;
>   	l_genl_debug_func_t debug_callback;
>   	l_genl_destroy_func_t debug_destroy;
> @@ -85,6 +86,7 @@ struct l_genl_msg {
>   
>   struct genl_request {
>   	unsigned int id;
> +	unsigned int gid;
>   	uint16_t type;
>   	uint16_t flags;
>   	uint32_t seq;
> @@ -114,8 +116,7 @@ struct genl_mcast {
>   	unsigned int users;
>   };
>   
> -struct l_genl_family {
> -	int ref_count;
> +struct genl_parent {
>   	struct l_genl *genl;
>   	char name[GENL_NAMSIZ];
>   	uint16_t id;
> @@ -124,11 +125,19 @@ struct l_genl_family {
>   	uint32_t maxattr;
>   	struct l_queue *op_list;
>   	struct l_queue *mcast_list;
> +	unsigned int nlctrl_cmd;
> +	struct l_queue *children;

So I wonder why you need this?

> +	bool live;
> +};
> +
> +struct l_genl_family {
> +	struct genl_parent *parent;
> +	int ref_count;
> +	unsigned int gid;
>   	l_genl_watch_func_t watch_appeared;
>   	l_genl_watch_func_t watch_vanished;
>   	l_genl_destroy_func_t watch_destroy;
>   	void *watch_data;

I wonder if these should just be kept on the parent object ?  Perhaps we 
should move watchlist from iwd to ell?

> -	unsigned int nlctrl_cmd;
>   };
>   
>   static void destroy_request(void *data)
> @@ -153,20 +162,31 @@ static void destroy_notify(void *data)
>   	l_free(notify);
>   }
>   
> -static struct l_genl_family *family_alloc(struct l_genl *genl,
> -							const char *name)
> +static struct l_genl_family *family_alloc_child(struct genl_parent *parent)
>   {
> -	struct l_genl_family *family;
> +	struct l_genl_family *family = l_new(struct l_genl_family, 1);
>   
> -	family = l_new(struct l_genl_family, 1);
> +	family->parent = parent;
> +	family->gid = parent->genl->next_gid++;
>   
> -	family->genl = genl;
> -	strncpy(family->name, name, GENL_NAMSIZ);
> +	l_queue_push_tail(parent->children, family);
>   
> -	family->op_list = l_queue_new();
> -	family->mcast_list = l_queue_new();
> +	return family;
> +}
>   
> -	return l_genl_family_ref(family);
> +static struct genl_parent *family_alloc_parent(struct l_genl *genl,
> +							const char *name)
> +{
> +	struct genl_parent *parent = l_new(struct genl_parent, 1);
> +
> +	parent->genl = genl;
> +	strncpy(parent->name, name, GENL_NAMSIZ);
> +
> +	parent->op_list = l_queue_new();
> +	parent->mcast_list = l_queue_new();
> +	parent->children = l_queue_new();
> +
> +	return parent;
>   }
>   
>   static void op_free(void *data)
> @@ -191,16 +211,63 @@ static void mcast_free(void *data, void *user_data)
>   	l_free(mcast);
>   }
>   
> -static void family_free(void *data)
> +static bool request_match_gid(const void *a, const void *b)
>   {
> -	struct l_genl_family *family = data;
> +	const struct genl_request *request = a;
> +	unsigned int gid = L_PTR_TO_UINT(b);
> +
> +	if (request->gid == gid)
> +		return true;
> +
> +	return false;
> +}
> +
> +static void cancel_all_requests(struct l_genl_family *child)
> +{
> +	struct l_genl *genl = child->parent->genl;
> +	struct genl_request *request;
> +
> +	/* genl has already been cleaned up */
> +	if (!genl->request_queue)
> +		return;
> +
> +	while ((request = l_queue_find(genl->request_queue, request_match_gid,
> +					L_UINT_TO_PTR(child->gid))))
> +		l_genl_family_cancel(child, request->id);

This seems like an N^2 operation.  Can't we use l_queue_remove_all or 
something?

> +}
> +
> +static void child_free(void *data)
> +{
> +	struct l_genl_family *child = data;
> +
> +	cancel_all_requests(child);
> +
> +	if (child->parent->id > 0 && child->watch_vanished)
> +		child->watch_vanished(child->watch_data);
> +
> +	if (child->watch_destroy)
> +		child->watch_destroy(child->watch_data);
> +

Okay, now I understand why you need the list of children.  So when you 
destroy the parent, you forcefully destroy all the children.  Yikes :)

Personally I think this was always a weird area.  The whole 
watch_vanished and watch_appeared stuff was badly implemented I think. 
family_appeared is really 'family was queried successfully and it 
exists' callback.  And 'family_vanished' was meant as 'this family 
really doesn't exist'.

The other issue is that l_genl_family_ref/unref had weird semantics.  We 
have a weird situation where family_free uses l_genl_family_unref.  But 
if someone actually took a ref of l_genl_family and the underlying genl 
object gets destroyed, the family is still around.  But can't send 
anything...

So now you're going from one extreme to the other, where every user of 
l_genl_family has to set a watch to make sure that the object isn't 
destroyed from under them.  And there's not much they can do about it. 
Not sure this is better or useful?

Perhaps we should have each parent take a ref of genl?

Then we can simplify the whole l_genl_family_new to two functions:

void l_genl_get_family_async(genl, name, callback, user, destroy);

Where family_vanished / family_appeared / set_watches is replaced by the 
callback.

And maybe a shorthand for when we know the family exists / was already 
queried:
struct l_genl_family *l_genl_get_family(genl, name);

And I would simply drop l_genl_family_ref/unref as they make no sense now.

> +
> +	l_free(child);
> +}
> +
> +static void parent_free(void *data)
> +{
> +	struct genl_parent *parent = data;
> +
> +	l_queue_destroy(parent->children, child_free);
> +
> +	l_queue_destroy(parent->op_list, op_free);
>   
> -	family->genl = NULL;
> +	l_queue_foreach(parent->mcast_list, mcast_free, parent->genl);
> +	l_queue_destroy(parent->mcast_list, NULL);
> +	parent->mcast_list = NULL;
>   
> -	l_genl_family_unref(family);
> +	l_free(parent);

Can we just keep the 'parent' object always around once queried?

Or alternatively, why don't we destroy it when all instances of 
l_genl_family with this parent have been destroyed.  E.g. have each 
l_genl_family take a ref of 'parent'.

>   }
>   
> -static void family_add_op(struct l_genl_family *family, uint32_t id,
> +static void family_add_op(struct genl_parent *parent, uint32_t id,
>   							uint32_t flags)
>   {
>   	struct genl_op *op;

Regards,
-Denis