mt76: usb: fix A-MSDU support
Subsystems (2)
Lorenzo Bianconi
maintainerv3 cover mt76: usb: fix A-MSDU support
Reallocate the skb if there is no enough space to manage the AMSDU rx packets. Do not always copy the first part of received frames if A-MSDU is enabled for SG capable devices
Changes since v2:
- simplify mt76u_build_rx_skb
- add patch 2/3: mt76u: introduce mt76u_ep data structure
- align usb buffer size to usb max endpoint length
- set buf_size to PAGE_SIZE even for sg case
Changes since v1:
- do not allocate multiple page buffers but rely on fragmented skbs if there is no enough space to manage the AMSDU rx packets
Lorenzo Bianconi (3):
mt76: usb: fix rx A-MSDU support
mt76: mt76u: introduce mt76u_ep data structure
mt76: usb: do not always copy the first part of received frames
drivers/net/wireless/mediatek/mt76/mt76.h | 17 +++-- drivers/net/wireless/mediatek/mt76/usb.c | 75 +++++++++++++++++------ 2 files changed, 67 insertions(+), 25 deletions(-)
Signature · 1 line
2.21.0
Lorenzo Bianconi
maintainerPATCH v3 1/3 mt76: usb: fix rx A-MSDU support
Commit f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for rx") breaks A-MSDU support. When A-MSDU is enable the device can receive frames up to q->buf_size but they will be discarded in mt76u_process_rx_entry since there is no enough room for skb_shared_info. Fix the issue reallocating the skb and copying in the linear area the first 128B of the received frames and in the frag_list the remaining part.
--- drivers/net/wireless/mediatek/mt76/mt76.h | 1 + drivers/net/wireless/mediatek/mt76/usb.c | 49 ++++++++++++++++++----- 2 files changed, 41 insertions(+), 9 deletions(-)
drivers/net/wireless/mediatek/mt76/mt76.h +1 -0
@@ -30,6 +30,7 @@#define MT_TX_RING_SIZE 256#define MT_MCU_RING_SIZE 32#define MT_RX_BUF_SIZE 2048 +#define MT_SKB_HEAD_LEN 128struct mt76_dev ;struct mt76_wcid ;
drivers/net/wireless/mediatek/mt76/usb.c +40 -9
65 lines not on this page · raw message
Signature · 1 line
2.21.0
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
Quoted 42 lines · Lorenzo Bianconi
Commit
f8f527b16db5("mt76: usb: use EP max packet aligned buffer sizes for rx") breaks A-MSDU support. When A-MSDU is enable the device can receive frames up toq->buf_sizebut they will be discarded inmt76u_process_rx_entrysince there is no enough room forskb_shared_info. Fix the issue reallocating the skb and copying in the linear area the first 128B of the received frames and in thefrag_listthe remaining part.Fixes f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for rx")Signed-off-by Lorenzo Bianconi <[email protected]>--- drivers/net/wireless/mediatek/mt76/mt76.h | 1 + drivers/net/wireless/mediatek/mt76/usb.c | 49 ++++++++++++++++++----- 2 files changed, 41 insertions(+), 9 deletions(-)drivers/net/wireless/mediatek/mt76/mt76.h +1
-0@@ -30,6 +30,7 @@#define MT_TX_RING_SIZE 256#define MT_MCU_RING_SIZE 32#define MT_RX_BUF_SIZE 2048 +#define MT_SKB_HEAD_LEN 128struct mt76_dev ;struct mt76_wcid ;drivers/net/wireless/mediatek/mt76/usb.c +7
-0@@ -429,6 +429,45 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)return dma_len; } +static struct sk_buff * +mt76u_build_rx_skb (u8 *data,int len,int buf_size) +{ +struct sk_buff *skb; + +if (SKB_WITH_OVERHEAD (buf_size) <MT_DMA_HDR_LEN + len) { +struct page *page;
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 448
Quoted 10 lines
drivers/net/wireless/mediatek/mt76/usb.c +10
-0+int offset; + +/* slow path, not enough space for data and +* skb_shared_info +*/ + skb =alloc_skb (MT_SKB_HEAD_LEN ,GFP_ATOMIC ); +if (!skb) +return NULL ; + +skb_put_data (skb, data +MT_DMA_HDR_LEN ,MT_SKB_HEAD_LEN );
I looked how rx amsdu is processed in mac80211 and it is decomposed and copied into newly allocated individual skb's, see ieee80211_amsdu_to_8023s()
So copy L3 & L4 headers doesn't do anything good here, actually seems to be better to have them in frag as __ieee80211_amsdu_copy_frag() do some magic to avid copy.
Stanislaw
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
Quoted 57 lines · Lorenzo Bianconi
Quoted 42 lines
Commit
f8f527b16db5("mt76: usb: use EP max packet aligned buffer sizes for rx") breaks A-MSDU support. When A-MSDU is enable the device can receive frames up toq->buf_sizebut they will be discarded inmt76u_process_rx_entrysince there is no enough room forskb_shared_info. Fix the issue reallocating the skb and copying in the linear area the first 128B of the received frames and in thefrag_listthe remaining part.Fixes f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for rx")Signed-off-by Lorenzo Bianconi <[email protected]>--- drivers/net/wireless/mediatek/mt76/mt76.h | 1 + drivers/net/wireless/mediatek/mt76/usb.c | 49 ++++++++++++++++++----- 2 files changed, 41 insertions(+), 9 deletions(-)drivers/net/wireless/mediatek/mt76/mt76.h +1
-0@@ -30,6 +30,7 @@#define MT_TX_RING_SIZE 256#define MT_MCU_RING_SIZE 32#define MT_RX_BUF_SIZE 2048 +#define MT_SKB_HEAD_LEN 128struct mt76_dev ;struct mt76_wcid ;drivers/net/wireless/mediatek/mt76/usb.c +7
-0@@ -429,6 +429,45 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)return dma_len; } +static struct sk_buff * +mt76u_build_rx_skb (u8 *data,int len,int buf_size) +{ +struct sk_buff *skb; + +if (SKB_WITH_OVERHEAD (buf_size) <MT_DMA_HDR_LEN + len) { +struct page *page;drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 448
Quoted 10 lines
drivers/net/wireless/mediatek/mt76/usb.c +10
-0+int offset; + +/* slow path, not enough space for data and +* skb_shared_info +*/ + skb =alloc_skb (MT_SKB_HEAD_LEN ,GFP_ATOMIC ); +if (!skb) +return NULL ; + +skb_put_data (skb, data +MT_DMA_HDR_LEN ,MT_SKB_HEAD_LEN );I looked how rx amsdu is processed in mac80211 and it is decomposed and copied into newly allocated individual skb's, see
ieee80211_amsdu_to_8023s()
Quoted 3 lines
So copy L3 & L4 headers doesn't do anything good here, actually seems to be better to have them in frag as
__ieee80211_amsdu_copy_frag()do some magic to avid copy.
Looking at __ieee80211_amsdu_copy() now I got why other drivers copy hdrlen + 8, thx :)
In our case reuse_frag is true in __ieee80211_amsdu_copy, so we will end up copying 32B + ether_len. Anyway I think 32 is a little bit too low and we could get better performances increasing it a little bit.
A typical use case (e.g IPv6 + TCP):
IPv6 = 40B, TCP = 32B --> so 72B..I guess 128B is a good value :) @Felix, Johannes: what do you think?
Regarding the patch I guess let's apply it as it is in order to fix the pending issue and then we will figure out how to proceed (copy hdr_len + 3 or increase the value in __ieee80211_amsdu_copy)
Regards,
Lorenzo
Quoted 2 lines
Stanislaw
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
Quoted 9 lines · Lorenzo Bianconi
Looking at
__ieee80211_amsdu_copy()now I got why other drivers copy hdrlen + 8, thx :)
In our casereuse_fragis true in__ieee80211_amsdu_copy, so we will end up copying 32B +ether_len. Anyway I think 32 is a little bit too low and we could get better performances increasing it a little bit.
A typical use case (e.g IPv6 + TCP):IPv6 = 40B, TCP = 32B --> so 72B..I guess 128B is a good value :) @Felix, Johannes: what do you think?
I think while we might *allocate* more, I don't think we should *copy* more, since then the TCP payload will no longer be in pages.
It'd probably be better to implement leaving enough tailroom (allocate 128), but copying nothing, unless the *entire* packet fits.
johannes
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
Quoted 9 lines · Johannes Berg
Quoted 7 lines · Lorenzo Bianconi
Looking at
__ieee80211_amsdu_copy()now I got why other drivers copy hdrlen + 8, thx :)
In our casereuse_fragis true in__ieee80211_amsdu_copy, so we will end up copying 32B +ether_len. Anyway I think 32 is a little bit too low and we could get better performances increasing it a little bit.
A typical use case (e.g IPv6 + TCP):
Quoted 8 lines
Quoted 2 lines
IPv6 = 40B, TCP = 32B --> so 72B..I guess 128B is a good value :) @Felix, Johannes: what do you think?
I think while we might *allocate* more, I don't think we should *copy* more, since then the TCP payload will no longer be in pages.
It'd probably be better to implement leaving enough tailroom (allocate 128), but copying nothing, unless the *entire* packet fits.
iwl4965 put entire packet in fragment in il4965_pass_packet_to_mac80211() . Initially I thought this is a bug, since mac80211 require header be in the linear area, but looks like ieee80211_rx_monitor() copy header before rest of mac80211 check it, so 4965 is fine.
Anyway I think the driver should put ieee80211 header in linear area and iwlwifi & mt7601u implementation is somewhat optimal.
Stanislaw
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
Quoted 16 lines · Stanislaw Gruszka
Quoted 15 lines · Johannes Berg
Quoted 9 lines · Lorenzo Bianconi
Looking at
__ieee80211_amsdu_copy()now I got why other drivers copy hdrlen + 8, thx :)
In our casereuse_fragis true in__ieee80211_amsdu_copy, so we will end up copying 32B +ether_len. Anyway I think 32 is a little bit too low and we could get better performances increasing it a little bit.
A typical use case (e.g IPv6 + TCP):IPv6 = 40B, TCP = 32B --> so 72B..I guess 128B is a good value :) @Felix, Johannes: what do you think?
I think while we might *allocate* more, I don't think we should *copy* more, since then the TCP payload will no longer be in pages.
Quoted 7 lines
Quoted 2 lines
It'd probably be better to implement leaving enough tailroom (allocate 128), but copying nothing, unless the *entire* packet fits.
iwl4965 put entire packet in fragment in
il4965_pass_packet_to_mac80211(). Initially I thought this is a bug, since mac80211 require header be in the linear area, but looks likeieee80211_rx_monitor()copy header before rest of mac80211 check it, so 4965 is fine.
Mac80211 should not assume anything about header being present or not, just like the rest of the network stack.
Quoted 2 lines
Anyway I think the driver should put ieee80211 header in linear area and iwlwifi & mt7601u implementation is somewhat optimal.
Yes, it's just an optimisation to do the copy-break (copy small packets completely) or to copy the header already (since we may have better ways to do so than skb_copy_bits if we still have a virt pointer to the page, rather than just the page pointer).
johannes
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
In reply to Stanislaw GruszkaQuoted 20 lines · Johannes Berg
Quoted 9 lines · Lorenzo Bianconi
Quoted 8 lines
Looking at
__ieee80211_amsdu_copy()now I got why other drivers copy hdrlen + 8, thx :)
In our casereuse_fragis true in__ieee80211_amsdu_copy, so we will end up copying 32B +ether_len. Anyway I think 32 is a little bit too low and we could get better performances increasing it a little bit.
A typical use case (e.g IPv6 + TCP):Quoted 8 lines
Quoted 2 lines
IPv6 = 40B, TCP = 32B --> so 72B..I guess 128B is a good value :) @Felix, Johannes: what do you think?
I think while we might *allocate* more, I don't think we should *copy* more, since then the TCP payload will no longer be in pages.
It'd probably be better to implement leaving enough tailroom (allocate 128), but copying nothing, unless the *entire* packet fits.
Quoted 7 lines
iwl4965 put entire packet in fragment in
il4965_pass_packet_to_mac80211(). Initially I thought this is a bug, since mac80211 require header be in the linear area, but looks likeieee80211_rx_monitor()copy header before rest of mac80211 check it, so 4965 is fine.Anyway I think the driver should put ieee80211 header in linear area and iwlwifi & mt7601u implementation is somewhat optimal.
Actually the case is a little bit different for mt76 since we need even mt76x02_rxwi in the linear area of the received skb. Taking that into account the requested size to copy will be:
32 + 802.11 hdr + SNAP hdr = ~ 70B
Moreover to pass rxwi size to usb module we need to add a field in mt76_driver_ops (e.g rxwi_size).
I will carry out some tests and if there are no differences I will post a single patch to wireless-drivers using 128B as default size
Regards,
Lorenzo
Quoted 2 lines
Stanislaw
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
In reply to Lorenzo BianconiQuoted 57 lines · Lorenzo Bianconi
Quoted 57 lines · Lorenzo Bianconi
Quoted 42 lines
Commit
f8f527b16db5("mt76: usb: use EP max packet aligned buffer sizes for rx") breaks A-MSDU support. When A-MSDU is enable the device can receive frames up toq->buf_sizebut they will be discarded inmt76u_process_rx_entrysince there is no enough room forskb_shared_info. Fix the issue reallocating the skb and copying in the linear area the first 128B of the received frames and in thefrag_listthe remaining part.Fixes f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for rx")Signed-off-by Lorenzo Bianconi <[email protected]>--- drivers/net/wireless/mediatek/mt76/mt76.h | 1 + drivers/net/wireless/mediatek/mt76/usb.c | 49 ++++++++++++++++++----- 2 files changed, 41 insertions(+), 9 deletions(-)drivers/net/wireless/mediatek/mt76/mt76.h +1
-0@@ -30,6 +30,7 @@#define MT_TX_RING_SIZE 256#define MT_MCU_RING_SIZE 32#define MT_RX_BUF_SIZE 2048 +#define MT_SKB_HEAD_LEN 128struct mt76_dev ;struct mt76_wcid ;drivers/net/wireless/mediatek/mt76/usb.c +7
-0@@ -429,6 +429,45 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)return dma_len; } +static struct sk_buff * +mt76u_build_rx_skb (u8 *data,int len,int buf_size) +{ +struct sk_buff *skb; + +if (SKB_WITH_OVERHEAD (buf_size) <MT_DMA_HDR_LEN + len) { +struct page *page;drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 448
Quoted 10 lines
drivers/net/wireless/mediatek/mt76/usb.c +10
-0+int offset; + +/* slow path, not enough space for data and +* skb_shared_info +*/ + skb =alloc_skb (MT_SKB_HEAD_LEN ,GFP_ATOMIC ); +if (!skb) +return NULL ; + +skb_put_data (skb, data +MT_DMA_HDR_LEN ,MT_SKB_HEAD_LEN );I looked how rx amsdu is processed in mac80211 and it is decomposed and copied into newly allocated individual skb's, see
ieee80211_amsdu_to_8023s()
Quoted 7 lines
Quoted 3 lines
So copy L3 & L4 headers doesn't do anything good here, actually seems to be better to have them in frag as
__ieee80211_amsdu_copy_frag()do some magic to avid copy.Looking at
__ieee80211_amsdu_copy()now I got why other drivers copy hdrlen + 8, thx :)
In our casereuse_fragis true in__ieee80211_amsdu_copy, so we will end up
I don't think reuse_frag is true in our case since skb->head_frag is not set when we use alloc_skb().
Stanislaw
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
Quoted 62 lines · Lorenzo Bianconi
Quoted 61 lines
Quoted 60 lines · Lorenzo Bianconi
Quoted 42 lines
Commit
f8f527b16db5("mt76: usb: use EP max packet aligned buffer sizes for rx") breaks A-MSDU support. When A-MSDU is enable the device can receive frames up toq->buf_sizebut they will be discarded inmt76u_process_rx_entrysince there is no enough room forskb_shared_info. Fix the issue reallocating the skb and copying in the linear area the first 128B of the received frames and in thefrag_listthe remaining part.Fixes f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for rx")Signed-off-by Lorenzo Bianconi <[email protected]>--- drivers/net/wireless/mediatek/mt76/mt76.h | 1 + drivers/net/wireless/mediatek/mt76/usb.c | 49 ++++++++++++++++++----- 2 files changed, 41 insertions(+), 9 deletions(-)drivers/net/wireless/mediatek/mt76/mt76.h +1
-0@@ -30,6 +30,7 @@#define MT_TX_RING_SIZE 256#define MT_MCU_RING_SIZE 32#define MT_RX_BUF_SIZE 2048 +#define MT_SKB_HEAD_LEN 128struct mt76_dev ;struct mt76_wcid ;drivers/net/wireless/mediatek/mt76/usb.c +7
-0@@ -429,6 +429,45 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)return dma_len; } +static struct sk_buff * +mt76u_build_rx_skb (u8 *data,int len,int buf_size) +{ +struct sk_buff *skb; + +if (SKB_WITH_OVERHEAD (buf_size) <MT_DMA_HDR_LEN + len) { +struct page *page;drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 448
Quoted 10 lines
drivers/net/wireless/mediatek/mt76/usb.c +10
-0+int offset; + +/* slow path, not enough space for data and +* skb_shared_info +*/ + skb =alloc_skb (MT_SKB_HEAD_LEN ,GFP_ATOMIC ); +if (!skb) +return NULL ; + +skb_put_data (skb, data +MT_DMA_HDR_LEN ,MT_SKB_HEAD_LEN );I looked how rx amsdu is processed in mac80211 and it is decomposed and copied into newly allocated individual skb's, see
ieee80211_amsdu_to_8023s()So copy L3 & L4 headers doesn't do anything good here, actually seems to be better to have them in frag as
__ieee80211_amsdu_copy_frag()do some magic to avid copy.
Quoted 6 lines
Quoted 3 lines
Looking at
__ieee80211_amsdu_copy()now I got why other drivers copy hdrlen + 8, thx :)
In our casereuse_fragis true in__ieee80211_amsdu_copy, so we will end upI don't think
reuse_fragis true in our case sinceskb->head_fragis not set when we usealloc_skb().
Oh, right. In this case it is probably better to use netdev_alloc_skb(). I will repost using the approach used in mt7601u since for the moment it will not make any difference to copy more data.
Regards,
Lorenzo
Quoted 2 lines
Stanislaw
Lorenzo Bianconi
maintainerPATCH v3 2/3 mt76: mt76u: introduce mt76u_ep data structure
In reply to Lorenzo BianconiIntroduce mt76u_ep data structure as a container for usb endpoint info. This is a preliminary patch to compute proper usb buffer size and avoid always copy the first part of received frames
--- drivers/net/wireless/mediatek/mt76/mt76.h | 16 ++++++++++------ drivers/net/wireless/mediatek/mt76/usb.c | 15 +++++++++------ 2 files changed, 19 insertions(+), 12 deletions(-)
drivers/net/wireless/mediatek/mt76/mt76.h +10 -6
@@ -382,6 +382,11 @@ enum mt76u_out_ep { __MT_EP_OUT_MAX, }; +struct mt76u_ep { +u16 max_packet; +u8 ep; +}; +#define MT_SG_MAX_SIZE 8#define MT_NUM_TX_ENTRIES 256#define MT_NUM_RX_ENTRIES 128
@@ -393,10 +398,8 @@ struct mt76_usb {struct tasklet_struct rx_tasklet;struct delayed_work stat_work; -u8 out_ep[__MT_EP_OUT_MAX]; -u16 out_max_packet; -u8 in_ep[__MT_EP_IN_MAX]; -u16 in_max_packet; +struct mt76u_ep out_ep[__MT_EP_OUT_MAX]; +struct mt76u_ep in_ep[__MT_EP_IN_MAX];bool sg_en;struct mt76u_mcu {
@@ -786,9 +789,10 @@ mt76u_bulk_msg(struct mt76_dev *dev, void *data, int len, int *actual_len,unsigned int pipe;if (actual_len) - pipe =usb_rcvbulkpipe (udev, usb->in_ep[MT_EP_IN_CMD_RESP ]); + pipe =usb_rcvbulkpipe (udev, usb->in_ep[MT_EP_IN_CMD_RESP ].ep);else - pipe =usb_sndbulkpipe (udev, usb->out_ep[MT_EP_OUT_INBAND_CMD ]); + pipe =usb_sndbulkpipe (udev, + usb->out_ep[MT_EP_OUT_INBAND_CMD ].ep);return usb_bulk_msg (udev, pipe, data, len, actual_len, timeout); }
drivers/net/wireless/mediatek/mt76/usb.c +9 -6
@@ -260,19 +260,22 @@ mt76u_set_endpoints(struct usb_interface *intf,struct usb_host_interface *intf_desc = intf->cur_altsetting;struct usb_endpoint_descriptor *ep_desc;int i, in_ep =0 , out_ep =0 ; +struct mt76u_ep *ep;for (i =0 ; i < intf_desc->desc.bNumEndpoints; i++) { ep_desc = &intf_desc->endpoint[i].desc;if (usb_endpoint_is_bulk_in (ep_desc) && in_ep < __MT_EP_IN_MAX) { - usb->in_ep[in_ep] =usb_endpoint_num (ep_desc); - usb->in_max_packet =usb_endpoint_maxp (ep_desc); + ep = &usb->in_ep[in_ep]; + ep->max_packet =usb_endpoint_maxp (ep_desc); + ep->ep =usb_endpoint_num (ep_desc); in_ep++; }else if (usb_endpoint_is_bulk_out (ep_desc) && out_ep < __MT_EP_OUT_MAX) { - usb->out_ep[out_ep] =usb_endpoint_num (ep_desc); - usb->out_max_packet =usb_endpoint_maxp (ep_desc); + ep = &usb->out_ep[out_ep]; + ep->max_packet =usb_endpoint_maxp (ep_desc); + ep->ep =usb_endpoint_num (ep_desc); out_ep++; } }
@@ -386,9 +389,9 @@ mt76u_fill_bulk_urb(struct mt76_dev *dev, int dir, int index,unsigned int pipe;if (dir ==USB_DIR_IN ) - pipe =usb_rcvbulkpipe (udev, dev->usb.in_ep[index]); + pipe =usb_rcvbulkpipe (udev, dev->usb.in_ep[index].ep);else - pipe =usb_sndbulkpipe (udev, dev->usb.out_ep[index]); + pipe =usb_sndbulkpipe (udev, dev->usb.out_ep[index].ep); urb->dev = udev; urb->pipe = pipe;
Signature · 1 line
2.21.0
Lorenzo Bianconi
maintainerPATCH v3 3/3 mt76: usb: do not always copy the first part of received frames
In reply to Lorenzo BianconiSet usb buffer size taking into account skb_shared_info in order to not always copy the first part of received frames if A-MSDU is enabled for SG capable devices. Moreover align usb buffer size to max_ep boundaries and set buf_size to PAGE_SIZE even for sg case
--- drivers/net/wireless/mediatek/mt76/usb.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +7 -4
@@ -289,8 +289,10 @@ static int mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb, int nsgs, gfp_t gfp) { - int i; + int i, data_size; + data_size =rounddown (SKB_WITH_OVERHEAD (q->buf_size), + dev->usb.in_ep[MT_EP_IN_PKT_RX ].max_packet);for (i =0 ; i < nsgs; i++) {struct page *page;void *data;
@@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb, page =virt_to_head_page (data); offset = data -page_address (page); -sg_set_page (&urb->sg[i], page, q->buf_size, offset); +sg_set_page (&urb->sg[i], page, data_size, offset); }if (i < nsgs) {
@@ -314,7 +316,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb, } urb->num_sgs =max_t (int, i, urb->num_sgs); - urb->transfer_buffer_length = urb->num_sgs * q->buf_size, + urb->transfer_buffer_length = urb->num_sgs * data_size;sg_init_marker (urb->sg, urb->num_sgs);return i ? : -ENOMEM ;
@@ -611,8 +613,9 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)if (!q->entry)return -ENOMEM ; - q->buf_size = dev->usb.sg_en ?MT_RX_BUF_SIZE :PAGE_SIZE ; q->ndesc =MT_NUM_RX_ENTRIES ; + q->buf_size =PAGE_SIZE ; +for (i =0 ; i < q->ndesc; i++) { err =mt76u_rx_urb_alloc (dev, &q->entry[i]);if (err <0 )
Signature · 1 line
2.21.0
Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
Quoted 4 lines · Lorenzo Bianconi
Set usb buffer size taking into account
skb_shared_infoin order to not always copy the first part of received frames if A-MSDU is enabled for SG capable devices. Moreover align usb buffer size tomax_epboundaries and setbuf_sizetoPAGE_SIZEeven for sg case
I think this should not be applied to wirless-drivers, only first patch that fix the bug and optimizations should be done in -next.
Quoted 13 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-1+int i, data_size; + data_size =rounddown (SKB_WITH_OVERHEAD (q->buf_size), + dev->usb.in_ep[MT_EP_IN_PKT_RX ].max_packet);for (i =0 ; i < nsgs; i++) {struct page *page;void *data;@@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb, page =virt_to_head_page (data); offset = data -page_address (page); -sg_set_page (&urb->sg[i], page, q->buf_size, offset); +sg_set_page (&urb->sg[i], page, data_size, offset);
<snip>
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 618
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1- q->buf_size = dev->usb.sg_en ?MT_RX_BUF_SIZE :PAGE_SIZE ; q->ndesc =MT_NUM_RX_ENTRIES ; + q->buf_size =PAGE_SIZE ; +
This should be associated with decrease of MT_SG_MAX_SIZE to value that is actually needed and currently this is 2 for 4k AMSDU.
However I don't think allocating 2 pages to avoid ieee80211 header and SNAP copy is worth to do. For me best approach would be allocate 1 page for 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without data_size change to avoid 32B copying).
Stanislaw
Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
Quoted 8 lines · Lorenzo Bianconi
Quoted 4 lines
Set usb buffer size taking into account
skb_shared_infoin order to not always copy the first part of received frames if A-MSDU is enabled for SG capable devices. Moreover align usb buffer size tomax_epboundaries and setbuf_sizetoPAGE_SIZEeven for sg caseI think this should not be applied to wirless-drivers, only first patch that fix the bug and optimizations should be done in -next.
ack, right. I think patch 2/3 and 3/3 can go directly in Felix's tree
Quoted 15 lines
Quoted 14 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-1+int i, data_size; + data_size =rounddown (SKB_WITH_OVERHEAD (q->buf_size), + dev->usb.in_ep[MT_EP_IN_PKT_RX ].max_packet);for (i =0 ; i < nsgs; i++) {struct page *page;void *data;@@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb, page =virt_to_head_page (data); offset = data -page_address (page); -sg_set_page (&urb->sg[i], page, q->buf_size, offset); +sg_set_page (&urb->sg[i], page, data_size, offset);<snip>
Quoted 7 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 618
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1- q->buf_size = dev->usb.sg_en ?MT_RX_BUF_SIZE :PAGE_SIZE ; q->ndesc =MT_NUM_RX_ENTRIES ; + q->buf_size =PAGE_SIZE ; +This should be associated with decrease of
MT_SG_MAX_SIZEto value that is actually needed and currently this is 2 for 4k AMSDU.
MT_SG_MAX_SIZE is used even on tx side and I do not think we will end up with a huge difference here
Quoted 5 lines
However I don't think allocating 2 pages to avoid ieee80211 header and SNAP copy is worth to do. For me best approach would be allocate 1 page for 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without
data_sizechange to avoid 32B copying).
From my point of view it is better to avoid copying if it is possible. Are you sure there is no difference?
Regards,
Lorenzo
Quoted 2 lines
Stanislaw
Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
Quoted 34 lines · Lorenzo Bianconi
Quoted 8 lines · Lorenzo Bianconi
Quoted 4 lines
Set usb buffer size taking into account
skb_shared_infoin order to not always copy the first part of received frames if A-MSDU is enabled for SG capable devices. Moreover align usb buffer size tomax_epboundaries and setbuf_sizetoPAGE_SIZEeven for sg caseI think this should not be applied to wirless-drivers, only first patch that fix the bug and optimizations should be done in -next.
ack, right. I think patch 2/3 and 3/3 can go directly in Felix's tree
Quoted 15 lines
Quoted 14 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-1+int i, data_size; + data_size =rounddown (SKB_WITH_OVERHEAD (q->buf_size), + dev->usb.in_ep[MT_EP_IN_PKT_RX ].max_packet);for (i =0 ; i < nsgs; i++) {struct page *page;void *data;@@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb, page =virt_to_head_page (data); offset = data -page_address (page); -sg_set_page (&urb->sg[i], page, q->buf_size, offset); +sg_set_page (&urb->sg[i], page, data_size, offset);<snip>
Quoted 7 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 618
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1- q->buf_size = dev->usb.sg_en ?MT_RX_BUF_SIZE :PAGE_SIZE ; q->ndesc =MT_NUM_RX_ENTRIES ; + q->buf_size =PAGE_SIZE ; +This should be associated with decrease of
MT_SG_MAX_SIZEto value that is actually needed and currently this is 2 for 4k AMSDU.
Quoted 2 lines
MT_SG_MAX_SIZEis used even on tx side and I do not think we will end up with a huge difference here
So use different value as argument for mt76u_fill_rx_sg() in mt76u_rx_urb_alloc(). After changing buf_size to PAGE_SIZE we will allocate 8 pages per rx queue entry, but only 2 pages will be used (with data_size change, 1 without data_size change). Or I'm wrong?
Quoted 7 lines
Quoted 4 lines
However I don't think allocating 2 pages to avoid ieee80211 header and SNAP copy is worth to do. For me best approach would be allocate 1 page for 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without
data_sizechange to avoid 32B copying).From my point of view it is better to avoid copying if it is possible. Are you sure there is no difference?
I do not understand what you mean by difference here.
Stanislaw
Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
Quoted 35 lines · Lorenzo Bianconi
Quoted 34 lines
Quoted 8 lines · Lorenzo Bianconi
Quoted 4 lines
Set usb buffer size taking into account
skb_shared_infoin order to not always copy the first part of received frames if A-MSDU is enabled for SG capable devices. Moreover align usb buffer size tomax_epboundaries and setbuf_sizetoPAGE_SIZEeven for sg caseI think this should not be applied to wirless-drivers, only first patch that fix the bug and optimizations should be done in -next.
ack, right. I think patch 2/3 and 3/3 can go directly in Felix's tree
Quoted 22 lines
Quoted 14 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-1+int i, data_size; + data_size =rounddown (SKB_WITH_OVERHEAD (q->buf_size), + dev->usb.in_ep[MT_EP_IN_PKT_RX ].max_packet);for (i =0 ; i < nsgs; i++) {struct page *page;void *data;@@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb, page =virt_to_head_page (data); offset = data -page_address (page); -sg_set_page (&urb->sg[i], page, q->buf_size, offset); +sg_set_page (&urb->sg[i], page, data_size, offset);<snip>
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 618
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1- q->buf_size = dev->usb.sg_en ?MT_RX_BUF_SIZE :PAGE_SIZE ; q->ndesc =MT_NUM_RX_ENTRIES ; + q->buf_size =PAGE_SIZE ; +This should be associated with decrease of
MT_SG_MAX_SIZEto value that is actually needed and currently this is 2 for 4k AMSDU.
Quoted 7 lines
Quoted 2 lines
MT_SG_MAX_SIZEis used even on tx side and I do not think we will end up with a huge difference hereSo use different value as argument for
mt76u_fill_rx_sg()inmt76u_rx_urb_alloc(). After changingbuf_sizetoPAGE_SIZEwe will allocate 8 pages per rx queue entry, but only 2 pages will be used (withdata_sizechange, 1 withoutdata_sizechange). Or I'm wrong?
yes, it is right (we will use two pages with data_size change). Maybe better to use 4 pages for each rx queue entry? (otherwise we will probably change it in the future)
Quoted 10 lines
Quoted 8 lines
Quoted 5 lines
However I don't think allocating 2 pages to avoid ieee80211 header and SNAP copy is worth to do. For me best approach would be allocate 1 page for 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without
data_sizechange to avoid 32B copying).From my point of view it is better to avoid copying if it is possible. Are you sure there is no difference?
I do not understand what you mean by difference here.
tpt differences, not sure if there are any
Regards,
Lorenzo
Quoted 2 lines
Stanislaw
Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
Quoted 29 lines · Lorenzo Bianconi
Quoted 29 lines
Quoted 28 lines
ack, right. I think patch 2/3 and 3/3 can go directly in Felix's tree
Quoted 15 lines
Quoted 14 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-1+int i, data_size; + data_size =rounddown (SKB_WITH_OVERHEAD (q->buf_size), + dev->usb.in_ep[MT_EP_IN_PKT_RX ].max_packet);for (i =0 ; i < nsgs; i++) {struct page *page;void *data;@@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb, page =virt_to_head_page (data); offset = data -page_address (page); -sg_set_page (&urb->sg[i], page, q->buf_size, offset); +sg_set_page (&urb->sg[i], page, data_size, offset);<snip>
Quoted 7 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 618
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1- q->buf_size = dev->usb.sg_en ?MT_RX_BUF_SIZE :PAGE_SIZE ; q->ndesc =MT_NUM_RX_ENTRIES ; + q->buf_size =PAGE_SIZE ; +This should be associated with decrease of
MT_SG_MAX_SIZEto value that is actually needed and currently this is 2 for 4k AMSDU.
MT_SG_MAX_SIZEis used even on tx side and I do not think we will end up with a huge difference here
Quoted 8 lines
Quoted 4 lines
So use different value as argument for
mt76u_fill_rx_sg()inmt76u_rx_urb_alloc(). After changingbuf_sizetoPAGE_SIZEwe will allocate 8 pages per rx queue entry, but only 2 pages will be used (withdata_sizechange, 1 withoutdata_sizechange). Or I'm wrong?yes, it is right (we will use two pages with
data_sizechange). Maybe better to use 4 pages for each rx queue entry? (otherwise we will probably change it in the future)
We should not allocate more than is required. If support for bigger rx AMSDUs will be added and announced in vht/ht capabilities to remote stations, then increase of number of segments will be needed.
Quoted 11 lines
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
However I don't think allocating 2 pages to avoid ieee80211 header and SNAP copy is worth to do. For me best approach would be allocate 1 page for 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without
data_sizechange to avoid 32B copying).From my point of view it is better to avoid copying if it is possible. Are you sure there is no difference?
I do not understand what you mean by difference here.
tpt differences, not sure if there are any
I would not expect any measurable difference in tpt nor in cpu usage either way.
But I think, if some AMSDU subframe will be spited into two fragments, data most likely will need to be linearised/copied, at some point before passed to application, what will overcome any benefit of avoiding coping 802.11 header. Thought, I don't think this somehow will be visible in benchmarking.
Stanislaw
Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
Quoted 55 lines · Lorenzo Bianconi
Quoted 29 lines
Quoted 29 lines
Quoted 28 lines
ack, right. I think patch 2/3 and 3/3 can go directly in Felix's tree
Quoted 15 lines
Quoted 14 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-1+int i, data_size; + data_size =rounddown (SKB_WITH_OVERHEAD (q->buf_size), + dev->usb.in_ep[MT_EP_IN_PKT_RX ].max_packet);for (i =0 ; i < nsgs; i++) {struct page *page;void *data;@@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb, page =virt_to_head_page (data); offset = data -page_address (page); -sg_set_page (&urb->sg[i], page, q->buf_size, offset); +sg_set_page (&urb->sg[i], page, data_size, offset);<snip>
Quoted 7 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 618
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1- q->buf_size = dev->usb.sg_en ?MT_RX_BUF_SIZE :PAGE_SIZE ; q->ndesc =MT_NUM_RX_ENTRIES ; + q->buf_size =PAGE_SIZE ; +This should be associated with decrease of
MT_SG_MAX_SIZEto value that is actually needed and currently this is 2 for 4k AMSDU.
MT_SG_MAX_SIZEis used even on tx side and I do not think we will end up with a huge difference hereQuoted 8 lines
Quoted 4 lines
So use different value as argument for
mt76u_fill_rx_sg()inmt76u_rx_urb_alloc(). After changingbuf_sizetoPAGE_SIZEwe will allocate 8 pages per rx queue entry, but only 2 pages will be used (withdata_sizechange, 1 withoutdata_sizechange). Or I'm wrong?yes, it is right (we will use two pages with
data_sizechange). Maybe better to use 4 pages for each rx queue entry? (otherwise we will probably change it in the future)We should not allocate more than is required. If support for bigger rx AMSDUs will be added and announced in vht/ht capabilities to remote stations, then increase of number of segments will be needed.
Quoted 11 lines
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
However I don't think allocating 2 pages to avoid ieee80211 header and SNAP copy is worth to do. For me best approach would be allocate 1 page for 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without
data_sizechange to avoid 32B copying).From my point of view it is better to avoid copying if it is possible. Are you sure there is no difference?
I do not understand what you mean by difference here.
tpt differences, not sure if there are any
Quoted 8 lines
I would not expect any measurable difference in tpt nor in cpu usage either way.
But I think, if some AMSDU subframe will be spited into two fragments, data most likely will need to be linearised/copied, at some point before passed to application, what will overcome any benefit of avoiding coping 802.11 header. Thought, I don't think this somehow will be visible in benchmarking.
Sorry for the late reply. I think so.
I will post a v4 soon.
Regards,
Lorenzo
Quoted 2 lines
Stanislaw