[v2,1/3] wiphy: make wiphy work queue reentrant

Commit Message

James Prestwood May 11, 2023, 6:52 p.m. UTC

In some situations its convenient for the same work item to be
inserted (rescheduled) while its in progress. FT for example does
this now if a roam fails. The same ft_work item gets re-inserted
which, currently, is not safe to do since the item is modified
and removed once completed.

Currently FT reschedules the same work item in the 'do_work'
callback but this patch actually allows for rescheduling as long
as the item is currently running, even if do_work has returned and
waiting for wiphy_radio_work_done.

A few new flags were added to the radio work item. One which is
set when the item is running which prevents other items from being
inserted ahead (previously done with priority=INT_MIN). This is
needed since we cannot modify the priority anymore in case the item
is being rescheduled.

The another flag is set when the item was rescheduled. Once the
item is finished the 'rescheduled' flag is checked and the item
will be removed and re-inserted into the queue if needed.
---
 src/wiphy.c | 69 ++++++++++++++++++++++++++++++++++++++++++-----------
 src/wiphy.h |  2 ++
 2 files changed, 57 insertions(+), 14 deletions(-)

Comments

Denis Kenzior May 17, 2023, 1:16 a.m. UTC | #1

Hi James,

On 5/11/23 13:52, James Prestwood wrote:
> In some situations its convenient for the same work item to be
> inserted (rescheduled) while its in progress. FT for example does
> this now if a roam fails. The same ft_work item gets re-inserted
> which, currently, is not safe to do since the item is modified
> and removed once completed.
> 
> Currently FT reschedules the same work item in the 'do_work'
> callback but this patch actually allows for rescheduling as long
> as the item is currently running, even if do_work has returned and
> waiting for wiphy_radio_work_done.
> 
> A few new flags were added to the radio work item. One which is
> set when the item is running which prevents other items from being
> inserted ahead (previously done with priority=INT_MIN). This is
> needed since we cannot modify the priority anymore in case the item
> is being rescheduled.

This seems way too complicated.

> 
> The another flag is set when the item was rescheduled. Once the
> item is finished the 'rescheduled' flag is checked and the item
> will be removed and re-inserted into the queue if needed.
> ---
>   src/wiphy.c | 69 ++++++++++++++++++++++++++++++++++++++++++-----------
>   src/wiphy.h |  2 ++
>   2 files changed, 57 insertions(+), 14 deletions(-)
> 

<snip>

> @@ -2583,7 +2594,7 @@ static void wiphy_radio_work_next(struct wiphy *wiphy)
>   	 * Ensures no other work item will get inserted before this one while
>   	 * the work is being done.
>   	 */
> -	work->priority = INT_MIN;
> +	work->running = true;
>   
>   	l_debug("Starting work item %u", work->id);
>   

Can't you simply take note of the current item id, set the priority, take it off 
the queue here, prior to starting.

> @@ -2592,7 +2603,12 @@ static void wiphy_radio_work_next(struct wiphy *wiphy)
>   	wiphy->work_in_callback = false;
>   

If done and work id changed, there's nothing to do.

If done, then proceed as now.

If not done, reinsert the work to the head.

>   	if (done) {
> -		work->id = 0;
> +		bool rescheduled = work->rescheduled;
> +
> +		work->running = false;
> +
> +		if (!rescheduled)
> +			work->id = 0;
>   
>   		l_queue_remove(wiphy->work, work);
>   
> @@ -2600,26 +2616,49 @@ static void wiphy_radio_work_next(struct wiphy *wiphy)
>   		destroy_work(work);

I'm really not sure this makes sense.  You are calling destroy on an item you 
just potentially reinserted.  I think a reinserted item should not trigger the 
destroy.  The caller has full control over the item anyway, so they can take 
care of this on their end.  In fact, I'd still think that the caller should use 
a different API for this to be absolutely clear, even if it is just 
wiphy_radio_work_insert() with some extra checks.

Regards,
-Denis

Patch

diff --git a/src/wiphy.c b/src/wiphy.c
index 2db2d2cd..e6ca3b76 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -2570,6 +2570,17 @@  static void wiphy_reg_notify(struct l_genl_msg *msg, void *user_data)
 	wiphy_dump_after_regdom(wiphy);
 }
 
+static int insert_by_priority(const void *a, const void *b, void *user_data)
+{
+	const struct wiphy_radio_work_item *new = a;
+	const struct wiphy_radio_work_item *work = b;
+
+	if (work->running || work->priority <= new->priority)
+		return 1;
+
+	return -1;
+}
+
 static void wiphy_radio_work_next(struct wiphy *wiphy)
 {
 	struct wiphy_radio_work_item *work;
@@ -2583,7 +2594,7 @@  static void wiphy_radio_work_next(struct wiphy *wiphy)
 	 * Ensures no other work item will get inserted before this one while
 	 * the work is being done.
 	 */
-	work->priority = INT_MIN;
+	work->running = true;
 
 	l_debug("Starting work item %u", work->id);
 
@@ -2592,7 +2603,12 @@  static void wiphy_radio_work_next(struct wiphy *wiphy)
 	wiphy->work_in_callback = false;
 
 	if (done) {
-		work->id = 0;
+		bool rescheduled = work->rescheduled;
+
+		work->running = false;
+
+		if (!rescheduled)
+			work->id = 0;
 
 		l_queue_remove(wiphy->work, work);
 
@@ -2600,26 +2616,49 @@  static void wiphy_radio_work_next(struct wiphy *wiphy)
 		destroy_work(work);
 		wiphy->work_in_callback = false;
 
+		/*
+		 * If the item was rescheduled inside do_work() we can safely
+		 * insert it here, otherwise destroy_work() could have freed it.
+		 * The item could have been re-inserted inside destroy_work()
+		 * but this is safe since the item was removed from the queue.
+		 */
+		if (rescheduled) {
+			work->rescheduled = false;
+			l_queue_insert(wiphy->work, work,
+					insert_by_priority, NULL);
+		}
+
 		wiphy_radio_work_next(wiphy);
 	}
 }
 
-static int insert_by_priority(const void *a, const void *b, void *user_data)
-{
-	const struct wiphy_radio_work_item *new = a;
-	const struct wiphy_radio_work_item *work = b;
-
-	if (work->priority <= new->priority)
-		return 1;
-
-	return -1;
-}
-
 uint32_t wiphy_radio_work_insert(struct wiphy *wiphy,
 				struct wiphy_radio_work_item *item,
 				int priority,
 				const struct wiphy_radio_work_item_ops *ops)
 {
+	/*
+	 * Handling the case of re-inserting the same work item that is in
+	 * progress. A non-started work item should never be re-inserted
+	 * into the queue. Keep the same ID, priority, and ops. If these somehow
+	 * are different the caller should really be using a separate work item.
+	 * Once the item is finished it will be removed and re-inserted based
+	 * on the rescheduled flag.
+	 */
+	if (item == l_queue_peek_head(wiphy->work)) {
+		/*
+		 * Shouldn't cause problems, but at least warn the caller they
+		 * should really be using a separate item
+		 */
+		L_WARN_ON(item->rescheduled || item->priority != priority ||
+				ops != item->ops);
+
+		item->rescheduled = true;
+		l_debug("Rescheduled work item %u", item->id);
+
+		return item->id;
+	}
+
 	item->priority = priority;
 	item->ops = ops;
 	item->id = ++work_ids;
@@ -2656,6 +2695,7 @@  void wiphy_radio_work_done(struct wiphy *wiphy, uint32_t id)
 	if (item->id == id) {
 		next = true;
 		l_queue_pop_head(wiphy->work);
+		item->running = false;
 	} else
 		item = l_queue_remove_if(wiphy->work, match_id,
 						L_UINT_TO_PTR(id));
@@ -2664,7 +2704,8 @@  void wiphy_radio_work_done(struct wiphy *wiphy, uint32_t id)
 
 	l_debug("Work item %u done", id);
 
-	item->id = 0;
+	if (!item->rescheduled)
+		item->id = 0;
 
 	wiphy->work_in_callback = true;
 	destroy_work(item);
diff --git a/src/wiphy.h b/src/wiphy.h
index d5d1cc8f..b6861f5f 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -45,6 +45,8 @@  struct wiphy_radio_work_item {
 	uint32_t id;
 	int priority;
 	const struct wiphy_radio_work_item_ops *ops;
+	bool rescheduled : 1;
+	bool running : 1;
 };
 
 enum {