1. Patchew
  2. linux
  3. View series

[PATCH wireless-next] wifi: mac80211: fold tid_ampdu_rx allocations into a flexible array

Rosen Penev posted 1 patch 4 days, 8 hours ago
There is a newer version of this series
net/mac80211/agg-rx.c   | 23 ++++++-----------------
net/mac80211/sta_info.h |  6 +++---
2 files changed, 9 insertions(+), 20 deletions(-)
[PATCH wireless-next] wifi: mac80211: fold tid_ampdu_rx allocations into a flexible array
Posted by Rosen Penev 4 days, 8 hours ago 
Convert the separately-allocated reorder_buf pointer to a C99 flexible
array member at the end of struct tid_ampdu_rx, and place reorder_time
in the same allocation immediately after it. This collapses three
allocations into one and removes the corresponding kfree() pairs from
the error and free paths.

Assisted-by: Claude:Opus-4.7
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 net/mac80211/agg-rx.c   | 23 ++++++-----------------
 net/mac80211/sta_info.h |  6 +++---
 2 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index cecd1c917e45..20669cd8a34f 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -50,8 +50,6 @@ static void ieee80211_free_tid_rx(struct rcu_head *h)

 	for (i = 0; i < tid_rx->buf_size; i++)
 		__skb_queue_purge(&tid_rx->reorder_buf[i]);
-	kfree(tid_rx->reorder_buf);
-	kfree(tid_rx->reorder_time);
 	kfree(tid_rx);
 }

@@ -294,6 +292,7 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 	};
 	int i, ret = -EOPNOTSUPP;
 	u16 status = WLAN_STATUS_REQUEST_DECLINED;
+	size_t alloc_size;
 	u16 max_buf_size;

 	lockdep_assert_wiphy(sta->local->hw.wiphy);
@@ -412,10 +411,14 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 	}

 	/* prepare A-MPDU MLME for Rx aggregation */
-	tid_agg_rx = kzalloc_obj(*tid_agg_rx);
+	alloc_size = struct_size(tid_agg_rx, reorder_buf, buf_size);
+	alloc_size += sizeof(*tid_agg_rx->reorder_time) * buf_size;
+	tid_agg_rx = kzalloc(alloc_size, GFP_KERNEL);
 	if (!tid_agg_rx)
 		goto end;

+	tid_agg_rx->reorder_time = (void *)(tid_agg_rx->reorder_buf + buf_size);
+
 	spin_lock_init(&tid_agg_rx->reorder_lock);

 	/* rx timer */
@@ -426,18 +429,6 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 	timer_setup(&tid_agg_rx->reorder_timer,
 		    sta_rx_agg_reorder_timer_expired, 0);

-	/* prepare reordering buffer */
-	tid_agg_rx->reorder_buf =
-		kzalloc_objs(struct sk_buff_head, buf_size);
-	tid_agg_rx->reorder_time =
-		kcalloc(buf_size, sizeof(unsigned long), GFP_KERNEL);
-	if (!tid_agg_rx->reorder_buf || !tid_agg_rx->reorder_time) {
-		kfree(tid_agg_rx->reorder_buf);
-		kfree(tid_agg_rx->reorder_time);
-		kfree(tid_agg_rx);
-		goto end;
-	}
-
 	for (i = 0; i < buf_size; i++)
 		__skb_queue_head_init(&tid_agg_rx->reorder_buf[i]);

@@ -445,8 +436,6 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 	ht_dbg(sta->sdata, "Rx A-MPDU request on %pM tid %d result %d\n",
 	       sta->sta.addr, tid, ret);
 	if (ret) {
-		kfree(tid_agg_rx->reorder_buf);
-		kfree(tid_agg_rx->reorder_time);
 		kfree(tid_agg_rx);
 		goto end;
 	}
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 39608a0abbb5..66adfc5c89b2 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -207,8 +207,6 @@ struct tid_ampdu_tx {
 /**
  * struct tid_ampdu_rx - TID aggregation information (Rx).
  *
- * @reorder_buf: buffer to reorder incoming aggregated MPDUs. An MPDU may be an
- *	A-MSDU with individually reported subframes.
  * @reorder_buf_filtered: bitmap indicating where there are filtered frames in
  *	the reorder buffer that should be ignored when releasing frames
  * @reorder_time: jiffies when skb was added
@@ -228,6 +226,8 @@ struct tid_ampdu_tx {
  *	and ssn.
  * @removed: this session is removed (but might have been found due to RCU)
  * @started: this session has started (head ssn or higher was received)
+ * @reorder_buf: buffer to reorder incoming aggregated MPDUs. An MPDU may be an
+ *	A-MSDU with individually reported subframes.
  *
  * This structure's lifetime is managed by RCU, assignments to
  * the array holding it must hold the aggregation mutex.
@@ -241,7 +241,6 @@ struct tid_ampdu_rx {
 	struct rcu_head rcu_head;
 	spinlock_t reorder_lock;
 	u64 reorder_buf_filtered;
-	struct sk_buff_head *reorder_buf;
 	unsigned long *reorder_time;
 	struct sta_info *sta;
 	struct timer_list session_timer;
@@ -256,6 +255,7 @@ struct tid_ampdu_rx {
 	u8 auto_seq:1,
 	   removed:1,
 	   started:1;
+	struct sk_buff_head reorder_buf[];
 };

 /**
--
2.54.0
Re: [PATCH wireless-next] wifi: mac80211: fold tid_ampdu_rx allocations into a flexible array
Posted by Johannes Berg 3 days, 20 hours ago 
On Wed, 2026-06-03 at 17:14 -0700, Rosen Penev wrote:
> Convert the separately-allocated reorder_buf pointer to a C99 flexible
> array member at the end of struct tid_ampdu_rx, and place reorder_time
> in the same allocation immediately after it. This collapses three
> allocations into one and removes the corresponding kfree() pairs from
> the error and free paths.

As I pointed out before, I'm not a huge fan of these "doing a minor
improvement for the sake of it" contributions ...

> @@ -241,7 +241,6 @@ struct tid_ampdu_rx {
>  	struct rcu_head rcu_head;
>  	spinlock_t reorder_lock;
>  	u64 reorder_buf_filtered;
> -	struct sk_buff_head *reorder_buf;
>  	unsigned long *reorder_time;
>  	struct sta_info *sta;
>  	struct timer_list session_timer;
> @@ -256,6 +255,7 @@ struct tid_ampdu_rx {
>  	u8 auto_seq:1,
>  	   removed:1,
>  	   started:1;
> +	struct sk_buff_head reorder_buf[];

Given that sizeof(unsigned long) == sizeof(void *), that would probably
be far simpler as

	struct {
		struct sk_buf_head buf;
		unsigned long time;
	} reorder[];

or so.

johannes
Re: [PATCH wireless-next] wifi: mac80211: fold tid_ampdu_rx allocations into a flexible array
Posted by Rosen Penev 3 days, 7 hours ago 
On Thu Jun 4, 2026 at 5:20 AM PDT, Johannes Berg wrote:
> On Wed, 2026-06-03 at 17:14 -0700, Rosen Penev wrote:
>> Convert the separately-allocated reorder_buf pointer to a C99 flexible
>> array member at the end of struct tid_ampdu_rx, and place reorder_time
>> in the same allocation immediately after it. This collapses three
>> allocations into one and removes the corresponding kfree() pairs from
>> the error and free paths.
>
> As I pointed out before, I'm not a huge fan of these "doing a minor
> improvement for the sake of it" contributions ...
You say that but these have resulted in missing kfrees being discovered
throughout the tree.
>
>> @@ -241,7 +241,6 @@ struct tid_ampdu_rx {
>>  	struct rcu_head rcu_head;
>>  	spinlock_t reorder_lock;
>>  	u64 reorder_buf_filtered;
>> -	struct sk_buff_head *reorder_buf;
>>  	unsigned long *reorder_time;
>>  	struct sta_info *sta;
>>  	struct timer_list session_timer;
>> @@ -256,6 +255,7 @@ struct tid_ampdu_rx {
>>  	u8 auto_seq:1,
>>  	   removed:1,
>>  	   started:1;
>> +	struct sk_buff_head reorder_buf[];
>
> Given that sizeof(unsigned long) == sizeof(void *), that would probably
> be far simpler as
>
> 	struct {
> 		struct sk_buf_head buf;
> 		unsigned long time;
> 	} reorder[];
>
> or so.
This is indeed much better. Will make the change.
>
> johannes

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.