| Message ID | 1423592732-17256-1-git-send-email-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >
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: