[PATCH] mac80211-hwsim: Don't enqueue pkts that do not want txstatus.

[PATCH] mac80211-hwsim: Don't enqueue pkts that do not want txstatus.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Otherwise, skb is not cleaned up until there is some timeout
and the tx-queue quickly becomes overly full.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

This is against 3.17.8+ kernel...it looks like same problem is in
the 3.19.0-rc7 kernel as well.

 drivers/net/wireless/mac80211_hwsim.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 989a6ba..7ea8948 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -928,10 +928,14 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
 	genlmsg_end(skb, msg_head);
 	genlmsg_unicast(&init_net, skb, dst_portid);
 
-	/* Enqueue the packet */
-	skb_queue_tail(&data->pending, my_skb);
 	data->tx_pkts++;
 	data->tx_bytes += my_skb->len;
+
+	/* Enqueue the packet if we are expecting a tx-status response */
+	if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
+		skb_queue_tail(&data->pending, my_skb);
+	else
+		ieee80211_free_txskb(hw, my_skb);
 	return;
 
 nla_put_failure:
-- 
1.7.11.7

Re: [PATCH] mac80211-hwsim: Don't enqueue pkts that do not want txstatus.

[Johannes Berg]

On Tue, 2015-02-10 at 10:25 -0800, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> Otherwise, skb is not cleaned up until there is some timeout
> and the tx-queue quickly becomes overly full.

> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -928,10 +928,14 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
>  	genlmsg_end(skb, msg_head);
>  	genlmsg_unicast(&init_net, skb, dst_portid);
>  
> -	/* Enqueue the packet */
> -	skb_queue_tail(&data->pending, my_skb);
>  	data->tx_pkts++;
>  	data->tx_bytes += my_skb->len;
> +
> +	/* Enqueue the packet if we are expecting a tx-status response */
> +	if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> +		skb_queue_tail(&data->pending, my_skb);
> +	else
> +		ieee80211_free_txskb(hw, my_skb);

This doesn't really seem right - essentially it means that whatever you
just gave to userspace is now completely useless?

It seems skb_orphan() could/should be put here.

johannes

Re: [PATCH] mac80211-hwsim: Don't enqueue pkts that do not want txstatus.

[Ben Greear]

On 02/23/2015 08:08 AM, Johannes Berg wrote:
> On Tue, 2015-02-10 at 10:25 -0800, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Otherwise, skb is not cleaned up until there is some timeout
>> and the tx-queue quickly becomes overly full.
> 
>> +++ b/drivers/net/wireless/mac80211_hwsim.c
>> @@ -928,10 +928,14 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
>>  	genlmsg_end(skb, msg_head);
>>  	genlmsg_unicast(&init_net, skb, dst_portid);
>>  
>> -	/* Enqueue the packet */
>> -	skb_queue_tail(&data->pending, my_skb);
>>  	data->tx_pkts++;
>>  	data->tx_bytes += my_skb->len;
>> +
>> +	/* Enqueue the packet if we are expecting a tx-status response */
>> +	if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
>> +		skb_queue_tail(&data->pending, my_skb);
>> +	else
>> +		ieee80211_free_txskb(hw, my_skb);
> 
> This doesn't really seem right - essentially it means that whatever you
> just gave to userspace is now completely useless?
> 
> It seems skb_orphan() could/should be put here.

I don't understand your complaint, why is what I gave to user-space useless?

If we just orphan them, does that clean up the skb memory properly?

If not, what eventually frees the skb?

Thanks,
Ben

> 
> johannes
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] mac80211-hwsim: Don't enqueue pkts that do not want txstatus.

[Johannes Berg]

On Mon, 2015-02-23 at 09:38 -0800, Ben Greear wrote:

> > This doesn't really seem right - essentially it means that whatever you
> > just gave to userspace is now completely useless?
> > 
> > It seems skb_orphan() could/should be put here.
> 
> I don't understand your complaint, why is what I gave to user-space useless?

userspace is supposed to give back the SKB pointer as a cookie when it
reports whether or not it was transmitted correctly, and then we will
free the SKB afterwards.

Note that I also just merged a patch from Bob in this area.

> If we just orphan them, does that clean up the skb memory properly?

No, of course not, but it removes it from socket accounting. This may be
what you want, although I'm not really sure it's the right thing to do
since it would make this behave differently than other drivers which
also don't orphan the SKB (and shouldn't - you can consider the SKB on
this queue as being on the HW DMA queue where it should still be
accounted for correctly)

I think perhaps you just want Bob's patch to fix the case of too slow
userspace.

> If not, what eventually frees the skb?

The tx status path does.

johannes

Re: [PATCH] mac80211-hwsim: Don't enqueue pkts that do not want txstatus.

[Ben Greear]

On 02/24/2015 02:14 AM, Johannes Berg wrote:
> On Mon, 2015-02-23 at 09:38 -0800, Ben Greear wrote:
>
>>> This doesn't really seem right - essentially it means that whatever you
>>> just gave to userspace is now completely useless?
>>>
>>> It seems skb_orphan() could/should be put here.
>>
>> I don't understand your complaint, why is what I gave to user-space useless?
>
> userspace is supposed to give back the SKB pointer as a cookie when it
> reports whether or not it was transmitted correctly, and then we will
> free the SKB afterwards.

If there is no status to return, then why would user-space call back at all?

Should user-space *always* return a status even when not requested to?

Thanks,
Ben


> Note that I also just merged a patch from Bob in this area.
>
>> If we just orphan them, does that clean up the skb memory properly?
>
> No, of course not, but it removes it from socket accounting. This may be
> what you want, although I'm not really sure it's the right thing to do
> since it would make this behave differently than other drivers which
> also don't orphan the SKB (and shouldn't - you can consider the SKB on
> this queue as being on the HW DMA queue where it should still be
> accounted for correctly)
>
> I think perhaps you just want Bob's patch to fix the case of too slow
> userspace.
>
>> If not, what eventually frees the skb?
>
> The tx status path does.
>
> johannes
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] mac80211-hwsim: Don't enqueue pkts that do not want txstatus.

[Johannes Berg]

On Tue, 2015-02-24 at 06:28 -0800, Ben Greear wrote:

> If there is no status to return, then why would user-space call back at all?
> 
> Should user-space *always* return a status even when not requested to?

I'd certainly expect so from hwsim.

johannes

Re: [PATCH] mac80211-hwsim: Don't enqueue pkts that do not want txstatus.

[Ben Greear]

On 02/24/2015 06:34 AM, Johannes Berg wrote:
> On Tue, 2015-02-24 at 06:28 -0800, Ben Greear wrote:
>
>> If there is no status to return, then why would user-space call back at all?
>>
>> Should user-space *always* return a status even when not requested to?
>
> I'd certainly expect so from hwsim.

I expected just the opposite.  But, I can change my user-space code easily enough.

Thanks,
Ben

>
> johannes
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com