[net-next] tipc: clarify meaning of 'inactive' field in struct tipc_subscription

[net-next] tipc: clarify meaning of 'inactive' field in struct tipc_subscription

[jmaloy]

From: Jon Maloy <jmaloy@redhat.com>

struct tipc_subscription has a boolean field 'inactive' which purpose
is not immediately obvious. When the subscription timer expires we are
still in interrupt context, and cannot easily just delete the
subscription. We therefore delay that action until the expiration
event has reached the work queue context where it is being sent to the
user. However, in the meantime other events may occur, which must be
suppressed to avoid any unexpected behavior.

We now clarify this with renaming the field and adding a comment.

Signed-off-by: Jon Maloy <jmaloy@redhat.com>
---
 net/tipc/subscr.c | 10 ++++++----
 net/tipc/subscr.h |  4 ++--
 net/tipc/topsrv.c |  7 ++++---
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index 05d49ad81290..094a5bf5145c 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2000-2017, Ericsson AB
  * Copyright (c) 2005-2007, 2010-2013, Wind River Systems
- * Copyright (c) 2020-2021, Red Hat Inc
+ * Copyright (c) 2020-2022, Red Hat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -46,7 +46,7 @@ static void tipc_sub_send_event(struct tipc_subscription *sub,
 	struct tipc_subscr *s = &sub->evt.s;
 	struct tipc_event *evt = &sub->evt;
 
-	if (sub->inactive)
+	if (sub->expired)
 		return;
 	tipc_evt_write(evt, event, event);
 	if (p) {
@@ -109,7 +109,9 @@ static void tipc_sub_timeout(struct timer_list *t)
 
 	spin_lock(&sub->lock);
 	tipc_sub_send_event(sub, NULL, TIPC_SUBSCR_TIMEOUT);
-	sub->inactive = true;
+
+	/* Block for more events until sub can be deleted from work context */
+	sub->expired = true;
 	spin_unlock(&sub->lock);
 }
 
@@ -152,7 +154,7 @@ struct tipc_subscription *tipc_sub_subscribe(struct net *net,
 	INIT_LIST_HEAD(&sub->sub_list);
 	sub->net = net;
 	sub->conid = conid;
-	sub->inactive = false;
+	sub->expired = false;
 	memcpy(&sub->evt.s, s, sizeof(*s));
 	sub->s.seq.type = tipc_sub_read(s, seq.type);
 	sub->s.seq.lower = lower;
diff --git a/net/tipc/subscr.h b/net/tipc/subscr.h
index 60b877531b66..1af00c69cd6c 100644
--- a/net/tipc/subscr.h
+++ b/net/tipc/subscr.h
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2003-2017, Ericsson AB
  * Copyright (c) 2005-2007, 2012-2013, Wind River Systems
- * Copyright (c) 2020-2021, Red Hat Inc
+ * Copyright (c) 2020-2022, Red Hat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -69,7 +69,7 @@ struct tipc_subscription {
 	struct list_head service_list;
 	struct list_head sub_list;
 	int conid;
-	bool inactive;
+	bool expired;
 	spinlock_t lock;
 };
 
diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index 5522865deae9..2d0e044a2524 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2012-2013, Wind River Systems
  * Copyright (c) 2017-2018, Ericsson AB
+ * Copyright (c) 2020-2022, Redhat Inc
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -105,7 +106,7 @@ struct tipc_conn {
 
 /* An entry waiting to be sent */
 struct outqueue_entry {
-	bool inactive;
+	bool expired;
 	struct tipc_event evt;
 	struct list_head list;
 };
@@ -261,7 +262,7 @@ static void tipc_conn_send_to_sock(struct tipc_conn *con)
 		evt = &e->evt;
 		spin_unlock_bh(&con->outqueue_lock);
 
-		if (e->inactive)
+		if (e->expired)
 			tipc_conn_delete_sub(con, &evt->s);
 
 		memset(&msg, 0, sizeof(msg));
@@ -325,7 +326,7 @@ void tipc_topsrv_queue_evt(struct net *net, int conid,
 	e = kmalloc(sizeof(*e), GFP_ATOMIC);
 	if (!e)
 		goto err;
-	e->inactive = (event == TIPC_SUBSCR_TIMEOUT);
+	e->expired = (event == TIPC_SUBSCR_TIMEOUT);
 	memcpy(&e->evt, evt, sizeof(*evt));
 	spin_lock_bh(&con->outqueue_lock);
 	list_add_tail(&e->list, &con->outqueue);
-- 
2.31.1

Re: [net-next] tipc: clarify meaning of 'inactive' field in struct tipc_subscription

[Jakub Kicinski]

On Tue, 29 Mar 2022 13:32:18 -0400 jmaloy@redhat.com wrote:
> From: Jon Maloy <jmaloy@redhat.com>
> 
> struct tipc_subscription has a boolean field 'inactive' which purpose
> is not immediately obvious. When the subscription timer expires we are
> still in interrupt context, and cannot easily just delete the
> subscription. We therefore delay that action until the expiration
> event has reached the work queue context where it is being sent to the
> user. However, in the meantime other events may occur, which must be
> suppressed to avoid any unexpected behavior.
> 
> We now clarify this with renaming the field and adding a comment.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>

# Form letter - net-next is closed

We have already sent the networking pull request for 5.18
and therefore net-next is closed for new drivers, features,
code refactoring and optimizations. We are currently accepting
bug fixes only.

Please repost when net-next reopens after 5.18-rc1 is cut.

RFC patches sent for review only are obviously welcome at any time.