mt76: usb: fix A-MSDU support
Subsystems (2)
Lorenzo Bianconi
maintainerv2 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 v1:
- do not allocate multiple page buffers but rely on fragmented skbs if there is no enough space to manage the AMSDU rx packets
@Felix: do you prefer to take this series in your tree or is it better to merge it in wireless-drivers?
Lorenzo Bianconi (2):
mt76: usb: fix rx A-MSDU support
mt76: usb: do not always copy the first part of received frames
drivers/net/wireless/mediatek/mt76/mt76.h | 4 ++ drivers/net/wireless/mediatek/mt76/usb.c | 64 ++++++++++++++++++----- 2 files changed, 55 insertions(+), 13 deletions(-)
Signature · 1 line
2.21.0
Lorenzo Bianconi
maintainerPATCH v2 1/2 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 | 52 +++++++++++++++++++---- 2 files changed, 44 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 +43 -9
68 lines not on this page · raw message
Signature · 1 line
2.21.0
Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
Hi and sorry for late comment.
Quoted 52 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 | 52 +++++++++++++++++++---- 2 files changed, 44 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 +17
-0@@ -429,6 +429,48 @@ 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, +int *nsgs) +{ +int data_len =min (len,MT_SKB_HEAD_LEN ); +struct sk_buff *skb; + +if (SKB_WITH_OVERHEAD (buf_size) >=MT_DMA_HDR_LEN + len) { +/* fast path */ + skb =build_skb (data, buf_size); +if (!skb) +return NULL ; + +skb_reserve (skb,MT_DMA_HDR_LEN ); +__skb_put (skb, len); + +return skb;
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 458
Quoted 10 lines
drivers/net/wireless/mediatek/mt76/usb.c +10
-0+ } + +/* slow path, not enough space for data and +* skb_shared_info +*/ + skb =alloc_skb (data_len,GFP_ATOMIC ); +if (!skb) +return NULL ; + +skb_put_data (skb, data +MT_DMA_HDR_LEN , data_len);
mt7601u and iwlmvm just copy hdrlen + 8 and put the rest of the buffer in fragment, which supose to be more efficient, see comment in iwl_mvm_pass_packet_to_mac80211().
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 463
Quoted 5 lines
drivers/net/wireless/mediatek/mt76/usb.c +5
-0+ data += (data_len +MT_DMA_HDR_LEN ); + len -= data_len; +if (len >0 ) { +struct page *page =virt_to_head_page (data); +int offset = data - (u8 *)page_address (page);
u8 cast not needed.
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 468
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-0+skb_add_rx_frag (skb,skb_shinfo (skb)->nr_frags, + page, offset, len, buf_size); + }else { + *nsgs =0 ;
This seems to be not necessary or a bug (use first buffer 2 times).
Stanislaw
Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
Quoted 2 lines
Hi and sorry for late comment.
Hi Stanislaw,
Quoted 58 lines · Lorenzo Bianconi
Quoted 57 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 | 52 +++++++++++++++++++---- 2 files changed, 44 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 +22
-0@@ -429,6 +429,48 @@ 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, +int *nsgs) +{ +int data_len =min (len,MT_SKB_HEAD_LEN ); +struct sk_buff *skb; + +if (SKB_WITH_OVERHEAD (buf_size) >=MT_DMA_HDR_LEN + len) { +/* fast path */ + skb =build_skb (data, buf_size); +if (!skb) +return NULL ; + +skb_reserve (skb,MT_DMA_HDR_LEN ); +__skb_put (skb, len); + +return skb; + } + +/* slow path, not enough space for data and +* skb_shared_info +*/
Quoted 8 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 458
Quoted 5 lines
drivers/net/wireless/mediatek/mt76/usb.c +5
-0+ skb =alloc_skb (data_len,GFP_ATOMIC ); +if (!skb) +return NULL ; + +skb_put_data (skb, data +MT_DMA_HDR_LEN , data_len);mt7601u and iwlmvm just copy hdrlen + 8 and put the rest of the buffer in fragment, which supose to be more efficient, see comment in
iwl_mvm_pass_packet_to_mac80211().
Right here we copy 128B instead of 32 but I think it is good to have L3 and L4 header in the linear area of the skb since otherwise the stack will need to align them
Quoted 8 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 463
Quoted 6 lines
drivers/net/wireless/mediatek/mt76/usb.c +5
-0+ data += (data_len +MT_DMA_HDR_LEN ); + len -= data_len; +if (len >0 ) { +struct page *page =virt_to_head_page (data); +int offset = data - (u8 *)page_address (page);u8 cast not needed.
Quoted 5 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 468
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-0+skb_add_rx_frag (skb,skb_shinfo (skb)->nr_frags, + page, offset, len, buf_size); + }else { + *nsgs =0 ;This seems to be not necessary or a bug (use first buffer 2 times).
maybe I have been a little bit too paranoid here :)
Regards,
Lorenzo
Quoted 2 lines
Stanislaw
Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
Quoted 4 lines · Lorenzo Bianconi
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 436
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-0+mt76u_build_rx_skb(u8 *data, int len, int buf_size, + int *nsgs) +{ +int data_len =min (len,MT_SKB_HEAD_LEN );
Oh, and this looks unneeded as well as for len < MT_SKB_HEAD_LEN=128 we will go through fast path.
Quoted 7 lines
Quoted 3 lines
mt7601u and iwlmvm just copy hdrlen + 8 and put the rest of the buffer in fragment, which supose to be more efficient, see comment in
iwl_mvm_pass_packet_to_mac80211().Right here we copy 128B instead of 32 but I think it is good to have L3 and L4 header in the linear area of the skb since otherwise the stack will need to align them
Not sure if understand, I think aliment of L3 & L4 headers will be the same, assuming ieee80211 header is aligned the same in fragment buffer and in linear area. But if you think this is better to copy those to linear area I'm ok with that.
Stanislaw
Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
Quoted 8 lines · Stanislaw Gruszka
Quoted 4 lines · Lorenzo Bianconi
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 436
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-0+mt76u_build_rx_skb(u8 *data, int len, int buf_size, + int *nsgs) +{ +int data_len =min (len,MT_SKB_HEAD_LEN );Oh, and this looks unneeded as well as for len < MT_SKB_HEAD_LEN=128 we will go through fast path.
I guess if we remove data_len = min(len, MT_SKB_HEAD_LEN) and even *nsgs = 0 at the end we are making some assumptions on the value of MT_SKB_HEAD_LEN and buf_size. In the patch I just avoided them but maybe we can just assume that MT_SKB_HEAD_LEN and buf_size will not changed in the future. What do you think?
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
mt7601u and iwlmvm just copy hdrlen + 8 and put the rest of the buffer in fragment, which supose to be more efficient, see comment in
iwl_mvm_pass_packet_to_mac80211().
Quoted 8 lines
Quoted 3 lines
Right here we copy 128B instead of 32 but I think it is good to have L3 and L4 header in the linear area of the skb since otherwise the stack will need to align them
Not sure if understand, I think aliment of L3 & L4 headers will be the same, assuming ieee80211 header is aligned the same in fragment buffer and in linear area. But if you think this is better to copy those to linear area I'm ok with that.
Sorry I have not been so clear. I mean in the stack before accessing a given header we will run pskb_may_pull() that can end up copying the skb if there is not enough space in the skb->head
Regards,
Lorenzo
Quoted 2 lines
Stanislaw
Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
Quoted 10 lines · Lorenzo Bianconi
Quoted 8 lines · Stanislaw Gruszka
Quoted 4 lines · Lorenzo Bianconi
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 436
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-0+mt76u_build_rx_skb(u8 *data, int len, int buf_size, + int *nsgs) +{ +int data_len =min (len,MT_SKB_HEAD_LEN );Oh, and this looks unneeded as well as for len < MT_SKB_HEAD_LEN=128 we will go through fast path.
Quoted 5 lines
I guess if we remove
data_len=min(len, MT_SKB_HEAD_LEN)and even *nsgs = 0 at the end we are making some assumptions on the value ofMT_SKB_HEAD_LENandbuf_size. In the patch I just avoided them but maybe we can just assume thatMT_SKB_HEAD_LENandbuf_sizewill not changed in the future. What do you think?
Yes, sure. Other drivers just use 128 value directly and don't even create a macro for that. And if somebody will decide to change buf_size it will not be small value.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
mt7601u and iwlmvm just copy hdrlen + 8 and put the rest of the buffer in fragment, which supose to be more efficient, see comment in
iwl_mvm_pass_packet_to_mac80211().Right here we copy 128B instead of 32 but I think it is good to have L3 and L4 header in the linear area of the skb since otherwise the stack will need to align them
Quoted 8 lines
Quoted 4 lines
Not sure if understand, I think aliment of L3 & L4 headers will be the same, assuming ieee80211 header is aligned the same in fragment buffer and in linear area. But if you think this is better to copy those to linear area I'm ok with that.
Sorry I have not been so clear. I mean in the stack before accessing a given header we will run
pskb_may_pull()that can end up copying the skb if there is not enough space in theskb->head
Ok, so L3 and L4 headers should be in linear area of skb and if not network stack will copy them from fragment. But I wonder why other drivers just copy ieee80211_hdr and SNAP ? Isn't that if we copy 128B then is possible that part of the payload will be in linear area and part in fragment, whereas is expected that payload will not be broken into two parts?
Stanislaw
Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
Quoted 17 lines · Lorenzo Bianconi
Quoted 10 lines · Stanislaw Gruszka
Quoted 8 lines · Lorenzo Bianconi
Quoted 4 lines
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_get_rx_entry_len(u8 *data, u32 data_len) · line 436
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/usb.c +4
-0+mt76u_build_rx_skb(u8 *data, int len, int buf_size, + int *nsgs) +{ +int data_len =min (len,MT_SKB_HEAD_LEN );Oh, and this looks unneeded as well as for len < MT_SKB_HEAD_LEN=128 we will go through fast path.
Quoted 5 lines
I guess if we remove
data_len=min(len, MT_SKB_HEAD_LEN)and even *nsgs = 0 at the end we are making some assumptions on the value ofMT_SKB_HEAD_LENandbuf_size. In the patch I just avoided them but maybe we can just assume thatMT_SKB_HEAD_LENandbuf_sizewill not changed in the future. What do you think?
Quoted 3 lines
Yes, sure. Other drivers just use 128 value directly and don't even create a macro for that. And if somebody will decide to change
buf_sizeit will not be small value.
Ok, I will simplify it in v2
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
mt7601u and iwlmvm just copy hdrlen + 8 and put the rest of the buffer in fragment, which supose to be more efficient, see comment in
iwl_mvm_pass_packet_to_mac80211().Right here we copy 128B instead of 32 but I think it is good to have L3 and L4 header in the linear area of the skb since otherwise the stack will need to align them
Quoted 8 lines
Quoted 4 lines
Not sure if understand, I think aliment of L3 & L4 headers will be the same, assuming ieee80211 header is aligned the same in fragment buffer and in linear area. But if you think this is better to copy those to linear area I'm ok with that.
Sorry I have not been so clear. I mean in the stack before accessing a given header we will run
pskb_may_pull()that can end up copying the skb if there is not enough space in theskb->head
Quoted 6 lines
Ok, so L3 and L4 headers should be in linear area of skb and if not network stack will copy them from fragment. But I wonder why other drivers just copy
ieee80211_hdrand SNAP ? Isn't that if we copy 128B then is possible that part of the payload will be in linear area and part in fragment, whereas is expected that payload will not be broken into two parts?
I think the payload can be split in multiple skb fragments, the constraint is just related to header parsing
Regards,
Lorenzo
Quoted 2 lines
Stanislaw
Lorenzo Bianconi
maintainerPATCH v2 2/2 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
--- drivers/net/wireless/mediatek/mt76/mt76.h | 3 +++ drivers/net/wireless/mediatek/mt76/usb.c | 12 ++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-)
drivers/net/wireless/mediatek/mt76/mt76.h +3 -0
@@ -32,6 +32,9 @@#define MT_RX_BUF_SIZE 2048#define MT_SKB_HEAD_LEN 128 +#define MT_BUF_WITH_OVERHEAD (x) \ + ((x) + SKB_DATA_ALIGN(sizeof(struct skb_shared_info))) +struct mt76_dev ;struct mt76_wcid ;
drivers/net/wireless/mediatek/mt76/usb.c +8 -4
@@ -286,7 +286,7 @@ 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 = SKB_WITH_OVERHEAD(q->buf_size);for (i =0 ; i < nsgs; i++) {struct page *page;
@@ -299,7 +299,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) {
@@ -311,7 +311,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 +611,12 @@ 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 ; +if (dev->usb.sg_en) + q->buf_size =MT_BUF_WITH_OVERHEAD (MT_RX_BUF_SIZE ); +else + q->buf_size =PAGE_SIZE ; q->ndesc =MT_NUM_RX_ENTRIES ; +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 v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 56 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 devicesSigned-off-by Lorenzo Bianconi <[email protected]>--- drivers/net/wireless/mediatek/mt76/mt76.h | 3 +++ drivers/net/wireless/mediatek/mt76/usb.c | 12 ++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-)drivers/net/wireless/mediatek/mt76/mt76.h +3
-0@@ -32,6 +32,9 @@#define MT_RX_BUF_SIZE 2048#define MT_SKB_HEAD_LEN 128 +#define MT_BUF_WITH_OVERHEAD (x) \ + ((x) + SKB_DATA_ALIGN(sizeof(struct skb_shared_info))) +struct mt76_dev ;struct mt76_wcid ;drivers/net/wireless/mediatek/mt76/usb.c +3
-3@@ -286,7 +286,7 @@ 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 = SKB_WITH_OVERHEAD(q->buf_size);for (i =0 ; i < nsgs; i++) {struct page *page;@@ -299,7 +299,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) {@@ -311,7 +311,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 ;
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 615
Quoted 6 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1@@ -611,8 +611,12 @@ 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 ; +if (dev->usb.sg_en) + q->buf_size =MT_BUF_WITH_OVERHEAD (MT_RX_BUF_SIZE );
I strongly recommend to not doing this. While this should work in theory creating buffer with size of 2k + some bytes might trigger various bugs in dma mapping or other low level code.
And skb_shered_info is needed only in first buffer IIUC.
Also this patch seems to make first patch unnecessary except for non sg_en case (in which I think rx AMSDU is broken anyway), so I would prefer just to apply first patch.
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 1 line · Lorenzo Bianconi
[...]
Quoted 12 lines
Quoted 12 lines
drivers/net/wireless/mediatek/mt76/usb.c +1
-1} 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 +611,12 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)if (!q->entry)return -ENOMEM ;
Quoted 7 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 615
Quoted 3 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1- q->buf_size = dev->usb.sg_en ?MT_RX_BUF_SIZE :PAGE_SIZE ; +if (dev->usb.sg_en) + q->buf_size =MT_BUF_WITH_OVERHEAD (MT_RX_BUF_SIZE );I strongly recommend to not doing this. While this should work in theory creating buffer with size of 2k + some bytes might trigger various bugs in dma mapping or other low level code.
even in practice actually :) but we can be more cautious since probably copying the first 128B will not make any difference
Quoted 6 lines
And
skb_shered_infois needed only in first buffer IIUC.Also this patch seems to make first patch unnecessary except for non
sg_encase (in which I think rx AMSDU is broken anyway), so I would prefer just to apply first patch.
I do not think rx AMSDU is broken for non sg_en case since the max rx value allowed should be 3839 IIRC and we alloc one page in this case
Regards,
Lorenzo
Quoted 3 lines
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 24 lines · Lorenzo Bianconi
Quoted 1 line · Lorenzo Bianconi
[...]
Quoted 12 lines
Quoted 12 lines
drivers/net/wireless/mediatek/mt76/usb.c +1
-1} 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 +611,12 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)if (!q->entry)return -ENOMEM ;Quoted 7 lines
drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 615
Quoted 3 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1- q->buf_size = dev->usb.sg_en ?MT_RX_BUF_SIZE :PAGE_SIZE ; +if (dev->usb.sg_en) + q->buf_size =MT_BUF_WITH_OVERHEAD (MT_RX_BUF_SIZE );I strongly recommend to not doing this. While this should work in theory creating buffer with size of 2k + some bytes might trigger various bugs in dma mapping or other low level code.
Quoted 1 line
even in practice actually :)
I wouldn't be sure about this. It's not common to have buffers of such size and crossing pages boundaries. It really can trigger nasty bugs on various IOMMU drivers.
Quoted 2 lines
but we can be more cautious since probably copying the first 128B will not make any difference
Not sure if I understand what you mean.
Quoted 8 lines
Quoted 5 lines
And
skb_shered_infois needed only in first buffer IIUC.Also this patch seems to make first patch unnecessary except for non
sg_encase (in which I think rx AMSDU is broken anyway), so I would prefer just to apply first patch.I do not think rx AMSDU is broken for non
sg_encase since the max rx value allowed should be 3839 IIRC and we alloc one page in this case
If that's the case we should be fine, but then I do not understand why we allocate 8*2k buffers for sg_en case, isn't that AP can sent AMSDU frame 16k big?
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 25 lines · Lorenzo Bianconi
Quoted 24 lines
Quoted 1 line · Lorenzo Bianconi
[...]
Quoted 19 lines
Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +1
-1} 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 ;drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 615
Quoted 6 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1@@ -611,8 +611,12 @@ 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 ; +if (dev->usb.sg_en) + q->buf_size =MT_BUF_WITH_OVERHEAD (MT_RX_BUF_SIZE );I strongly recommend to not doing this. While this should work in theory creating buffer with size of 2k + some bytes might trigger various bugs in dma mapping or other low level code.
Quoted 5 lines
Quoted 1 line
even in practice actually :)
I wouldn't be sure about this. It's not common to have buffers of such size and crossing pages boundaries. It really can trigger nasty bugs on various IOMMU drivers.
I was just joking, I mean that it worked in the tests I carried out, but I agree it can trigger some issues in buggy IOMMU drivers
Quoted 5 lines
Quoted 3 lines
but we can be more cautious since probably copying the first 128B will not make any difference
Not sure if I understand what you mean.
Please correct me if I am wrong but I think max amsdu rx size is 3839B for mt76. For the sg_en case this frame will span over multiple sg buffers since sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into account skb_shared_info when configuring the sg buffer size we will need to always copy the first 128B of the first buffer since received len will be set to 2048 and the following if condition will always fail:
if (SKB_WITH_OVERHEAD(buf_size) >= MT_DMA_HDR_LEN + len) { }
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
And
skb_shered_infois needed only in first buffer IIUC.Also this patch seems to make first patch unnecessary except for non
sg_encase (in which I think rx AMSDU is broken anyway), so I would prefer just to apply first patch.
Quoted 6 lines
Quoted 2 lines
I do not think rx AMSDU is broken for non
sg_encase since the max rx value allowed should be 3839 IIRC and we alloc one page in this caseIf that's the case we should be fine, but then I do not understand why we allocate 8*2k buffers for
sg_encase, isn't that AP can sent AMSDU frame 16k big?
Sorry I did not get what you mean here, could you please explain?
Regards,
Lorenzo
Quoted 3 lines
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 25 lines · Lorenzo Bianconi
Quoted 25 lines · Lorenzo Bianconi
Quoted 24 lines
Quoted 1 line · Lorenzo Bianconi
[...]
Quoted 19 lines
Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +1
-1} 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 ;drivers/net/wireless/mediatek/mt76/usb.c static int mt76u_alloc_rx(struct mt76_dev *dev) · line 615
Quoted 6 lines
drivers/net/wireless/mediatek/mt76/usb.c +2
-1@@ -611,8 +611,12 @@ 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 ; +if (dev->usb.sg_en) + q->buf_size =MT_BUF_WITH_OVERHEAD (MT_RX_BUF_SIZE );I strongly recommend to not doing this. While this should work in theory creating buffer with size of 2k + some bytes might trigger various bugs in dma mapping or other low level code.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
even in practice actually :)
I wouldn't be sure about this. It's not common to have buffers of such size and crossing pages boundaries. It really can trigger nasty bugs on various IOMMU drivers.
I was just joking, I mean that it worked in the tests I carried out, but I agree it can trigger some issues in buggy IOMMU drivers
My sense of humor declined quite drastically lastly ;-/
Quoted 12 lines
Quoted 4 lines
Quoted 2 lines
but we can be more cautious since probably copying the first 128B will not make any difference
Not sure if I understand what you mean.
Please correct me if I am wrong but I think max amsdu rx size is 3839B for mt76. For the
sg_encase this frame will span over multiple sg buffers since sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into accountskb_shared_infowhen configuring the sg buffer size we will need to always copy the first 128B of the first buffer since received len will be set to 2048 and the following if condition will always fail:
Quoted 2 lines
if (
SKB_WITH_OVERHEAD(buf_size)>=MT_DMA_HDR_LEN+ len) { }
Ok, that I understand.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
And
skb_shered_infois needed only in first buffer IIUC.Also this patch seems to make first patch unnecessary except for non
sg_encase (in which I think rx AMSDU is broken anyway), so I would prefer just to apply first patch.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
I do not think rx AMSDU is broken for non
sg_encase since the max rx value allowed should be 3839 IIRC and we alloc one page in this caseIf that's the case we should be fine, but then I do not understand why we allocate 8*2k buffers for
sg_encase, isn't that AP can sent AMSDU frame 16k big?Sorry I did not get what you mean here, could you please explain?
If max RX AMSDU size is 3839B I do not see reason why we allocate MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for sg_en case. I thought the reason is that max AMSDU size is 16kB so it fit into 8 sg buffers of 2k.
In other words, for me, looks like either
- we can not handle AMSDU for non sg case because we do not allocate big enough buffer
or
- we can just use one PAGE_SIZE buffer for rx and remove sg buffers for rx completely
Do I miss something ?
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
[...]
Quoted 35 lines
My sense of humor declined quite drastically lastly ;-/
Quoted 12 lines
Quoted 4 lines
Quoted 2 lines
but we can be more cautious since probably copying the first 128B will not make any difference
Not sure if I understand what you mean.
Please correct me if I am wrong but I think max amsdu rx size is 3839B for mt76. For the
sg_encase this frame will span over multiple sg buffers since sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into accountskb_shared_infowhen configuring the sg buffer size we will need to always copy the first 128B of the first buffer since received len will be set to 2048 and the following if condition will always fail:Quoted 2 lines
if (
SKB_WITH_OVERHEAD(buf_size)>=MT_DMA_HDR_LEN+ len) { }Ok, that I understand.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
And
skb_shered_infois needed only in first buffer IIUC.Also this patch seems to make first patch unnecessary except for non
sg_encase (in which I think rx AMSDU is broken anyway), so I would prefer just to apply first patch.Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
I do not think rx AMSDU is broken for non
sg_encase since the max rx value allowed should be 3839 IIRC and we alloc one page in this caseIf that's the case we should be fine, but then I do not understand why we allocate 8*2k buffers for
sg_encase, isn't that AP can sent AMSDU frame 16k big?Sorry I did not get what you mean here, could you please explain?
Quoted 8 lines
If max RX AMSDU size is 3839B I do not see reason why we allocate MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for
sg_encase. I thought the reason is that max AMSDU size is 16kB so it fit into 8 sg buffers of 2k.In other words, for me, looks like either
- we can not handle AMSDU for non sg case because we do not allocate big enough buffer
I think AMSDU is mandatory and we currently support it even for non-sg case (since max rx AMSDU is 3839B)
Quoted 3 lines
or
- we can just use onePAGE_SIZEbuffer for rx and remove sg buffers for rx completely
using sg buffers we can support bigger rx AMSDU size in the future without using huge buffers (e.g. we can try to use IEEE80211_MAX_MPDU_LEN_HT_7935 with mt76x2u)
Regards,
Lorenzo
Quoted 4 lines
Do I miss something ?
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 48 lines · Lorenzo Bianconi
[...]
Quoted 35 lines
My sense of humor declined quite drastically lastly ;-/
Quoted 12 lines
Quoted 4 lines
Quoted 2 lines
but we can be more cautious since probably copying the first 128B will not make any difference
Not sure if I understand what you mean.
Please correct me if I am wrong but I think max amsdu rx size is 3839B for mt76. For the
sg_encase this frame will span over multiple sg buffers since sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into accountskb_shared_infowhen configuring the sg buffer size we will need to always copy the first 128B of the first buffer since received len will be set to 2048 and the following if condition will always fail:Quoted 2 lines
if (
SKB_WITH_OVERHEAD(buf_size)>=MT_DMA_HDR_LEN+ len) { }Ok, that I understand.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
And
skb_shered_infois needed only in first buffer IIUC.Also this patch seems to make first patch unnecessary except for non
sg_encase (in which I think rx AMSDU is broken anyway), so I would prefer just to apply first patch.Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
I do not think rx AMSDU is broken for non
sg_encase since the max rx value allowed should be 3839 IIRC and we alloc one page in this caseIf that's the case we should be fine, but then I do not understand why we allocate 8*2k buffers for
sg_encase, isn't that AP can sent AMSDU frame 16k big?Sorry I did not get what you mean here, could you please explain?
Quoted 8 lines
If max RX AMSDU size is 3839B I do not see reason why we allocate MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for
sg_encase. I thought the reason is that max AMSDU size is 16kB so it fit into 8 sg buffers of 2k.In other words, for me, looks like either
- we can not handle AMSDU for non sg case because we do not allocate big enough bufferI think AMSDU is mandatory and we currently support it even for non-sg case (since max rx AMSDU is 3839B)
Quoted 7 lines
Quoted 3 lines
or
- we can just use onePAGE_SIZEbuffer for rx and remove sg buffers for rx completelyusing sg buffers we can support bigger rx AMSDU size in the future without using huge buffers (e.g. we can try to use
IEEE80211_MAX_MPDU_LEN_HT_7935with mt76x2u)
I think it would be simpler just to allocate 2 pages for 7935B .
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 16 lines · Stanislaw Gruszka
Quoted 16 lines
Quoted 8 lines
If max RX AMSDU size is 3839B I do not see reason why we allocate MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for
sg_encase. I thought the reason is that max AMSDU size is 16kB so it fit into 8 sg buffers of 2k.In other words, for me, looks like either
- we can not handle AMSDU for non sg case because we do not allocate big enough bufferI think AMSDU is mandatory and we currently support it even for non-sg case (since max rx AMSDU is 3839B)
Quoted 3 lines
or
- we can just use onePAGE_SIZEbuffer for rx and remove sg buffers for rx completely
Quoted 5 lines
Quoted 3 lines
using sg buffers we can support bigger rx AMSDU size in the future without using huge buffers (e.g. we can try to use
IEEE80211_MAX_MPDU_LEN_HT_7935with mt76x2u)I think it would be simpler just to allocate 2 pages for 7935B .
And if we could determine that there is no true need to use sg for rx, I think best approach here would be revert f8f527b16db5 in v5.2 to fix regression and remove rx sg in -next. That would make code simpler, allocate 4k instead 16k per packet, allow to use build_skb (4096 - 3839 give enough space for shared info) and not use usb hcd bounce buffer.
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 21 lines · Stanislaw Gruszka
Quoted 20 lines · Stanislaw Gruszka
Quoted 19 lines
Quoted 8 lines
If max RX AMSDU size is 3839B I do not see reason why we allocate MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for
sg_encase. I thought the reason is that max AMSDU size is 16kB so it fit into 8 sg buffers of 2k.In other words, for me, looks like either
- we can not handle AMSDU for non sg case because we do not allocate big enough bufferI think AMSDU is mandatory and we currently support it even for non-sg case (since max rx AMSDU is 3839B)
Quoted 3 lines
or
- we can just use onePAGE_SIZEbuffer for rx and remove sg buffers for rx completelyusing sg buffers we can support bigger rx AMSDU size in the future without using huge buffers (e.g. we can try to use
IEEE80211_MAX_MPDU_LEN_HT_7935with mt76x2u)
Quoted 7 lines
Quoted 1 line
I think it would be simpler just to allocate 2 pages for 7935B .
And if we could determine that there is no true need to use sg for rx, I think best approach here would be revert
f8f527b16db5in v5.2 to fix regression and remove rx sg in -next. That would make code simpler, allocate 4k instead 16k per packet, allow to usebuild_skb(4096 - 3839 give enough space for shared info) and not use usb hcd bounce buffer.
I do not think we should drop sg support since:
- it allow us to rx huge amsdu frames (e.g. IEEE80211_MAX_MPDU_LEN_VHT_11454) using multiple one page buffer. I think there will be new usb devices where we can increase amsdu size (we can increase it even on mt76x2u usb 3.0 devices)
- without SG we can't use build_skb() without copying a given size of the packet since the space needed for skb_shared_info is 320B on a x86_64 device
- the fix for f8f527b16db5 has been already tested
Regards,
Lorenzo
Quoted 2 lines
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 30 lines · Lorenzo Bianconi
Quoted 21 lines · Stanislaw Gruszka
Quoted 20 lines · Stanislaw Gruszka
Quoted 19 lines
Quoted 8 lines
If max RX AMSDU size is 3839B I do not see reason why we allocate MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for
sg_encase. I thought the reason is that max AMSDU size is 16kB so it fit into 8 sg buffers of 2k.In other words, for me, looks like either
- we can not handle AMSDU for non sg case because we do not allocate big enough bufferI think AMSDU is mandatory and we currently support it even for non-sg case (since max rx AMSDU is 3839B)
Quoted 3 lines
or
- we can just use onePAGE_SIZEbuffer for rx and remove sg buffers for rx completelyusing sg buffers we can support bigger rx AMSDU size in the future without using huge buffers (e.g. we can try to use
IEEE80211_MAX_MPDU_LEN_HT_7935with mt76x2u)Quoted 7 lines
Quoted 1 line
I think it would be simpler just to allocate 2 pages for 7935B .
And if we could determine that there is no true need to use sg for rx, I think best approach here would be revert
f8f527b16db5in v5.2 to fix regression and remove rx sg in -next. That would make code simpler, allocate 4k instead 16k per packet, allow to usebuild_skb(4096 - 3839 give enough space for shared info) and not use usb hcd bounce buffer.
Quoted 4 lines
I do not think we should drop sg support since:
- it allow us to rx huge amsdu frames (e.g.IEEE80211_MAX_MPDU_LEN_VHT_11454) using multiple one page buffer. I think there will be new usb devices where we can increase amsdu size (we can increase it even on mt76x2u usb 3.0 devices)
I looked at intel wifi drivers and this is handled by amsdu_size module parameter, supported values are 4k, 8k and 12k. RX allocation size and proper values in vht_cap & ht_cap are set accordingly. Assuming (some) mt76 HW and FW can handle bigger AMSDUs I think we should do similar thing.
Otherwise looks for me, we just waste memory and have not needed code for no true reason.
Quoted 1 line
space needed for
skb_shared_infois 320B on ax86_64device
Uhh, I haven't expected that sk_shared_info() is that big, so indeed build_skb could not used and 128B copy fallback will be necessary.
Stanislaw
Lorenzo Bianconi
maintainerRe: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
[...]
Quoted 6 lines
I looked at intel wifi drivers and this is handled by
amsdu_sizemodule parameter, supported values are 4k, 8k and 12k. RX allocation size and proper values invht_cap&ht_capare set accordingly. Assuming (some) mt76 HW and FW can handle bigger AMSDUs I think we should do similar thing.
Quoted 7 lines
Otherwise looks for me, we just waste memory and have not needed code for no true reason.
Quoted 1 line
space needed for
skb_shared_infois 320B on ax86_64deviceUhh, I haven't expected that
sk_shared_info()is that big, so indeedbuild_skbcould not used and 128B copy fallback will be necessary.
Hi Stanislaw,
reviewing the original patch I think we can't trigger any IOMMU bug since the usb buffer length is actually 2048 and not 2048 + skb_shared_info_size:
in mt76u_fill_rx_sg()data_size = SKB_WITH_OVERHEAD(q->buf_size);
q->buf_size = MT_BUF_WITH_OVERHEAD(MT_RX_BUF_SIZE);
where MT_BUF_WITH_OVERHEAD is
((x) + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
anyway we can even setq->buf_size = PAGE_SIZEdata_size = rounddown(SKB_WITH_OVERHEAD(q->buf_size), usb_endpoint_maxp())
Regards,
Lorenzo
Quoted 3 lines
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Hi
Quoted 13 lines · Lorenzo Bianconi
[...]
Quoted 11 lines
I looked at intel wifi drivers and this is handled by
amsdu_sizemodule parameter, supported values are 4k, 8k and 12k. RX allocation size and proper values invht_cap&ht_capare set accordingly. Assuming (some) mt76 HW and FW can handle bigger AMSDUs I think we should do similar thing.Otherwise looks for me, we just waste memory and have not needed code for no true reason.
Quoted 1 line
space needed for
skb_shared_infois 320B on ax86_64device
Quoted 7 lines
Quoted 2 lines
Uhh, I haven't expected that
sk_shared_info()is that big, so indeedbuild_skbcould not used and 128B copy fallback will be necessary.Hi Stanislaw,
reviewing the original patch I think we can't trigger any IOMMU bug since the usb buffer length is actually 2048 and not 2048 +
skb_shared_info_size:
I'm concerned about alignment and crossing pages boundaries. If you allocate via page_frag_alloc() buffers, except first one, will have 'not natural' alignment and every second will be spanned across two pages.
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 21 lines
Hi
Quoted 18 lines · Lorenzo Bianconi
[...]
Quoted 6 lines
I looked at intel wifi drivers and this is handled by
amsdu_sizemodule parameter, supported values are 4k, 8k and 12k. RX allocation size and proper values invht_cap&ht_capare set accordingly. Assuming (some) mt76 HW and FW can handle bigger AMSDUs I think we should do similar thing.Quoted 7 lines
Otherwise looks for me, we just waste memory and have not needed code for no true reason.
Quoted 1 line
space needed for
skb_shared_infois 320B on ax86_64deviceUhh, I haven't expected that
sk_shared_info()is that big, so indeedbuild_skbcould not used and 128B copy fallback will be necessary.Hi Stanislaw,
Quoted 7 lines
Quoted 2 lines
reviewing the original patch I think we can't trigger any IOMMU bug since the usb buffer length is actually 2048 and not 2048 +
skb_shared_info_size:I'm concerned about alignment and crossing pages boundaries. If you allocate via
page_frag_alloc()buffers, except first one, will have 'not natural' alignment and every second will be spanned across two pages.
ack, so I think the second approach will be safer (using roundup instead of rounddown :))
Regards,
Lorenzo
Quoted 3 lines
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
In reply to Stanislaw GruszkaQuoted 1 line · Lorenzo Bianconi
[...]
Quoted 7 lines
Quoted 4 lines
using sg buffers we can support bigger rx AMSDU size in the future without using huge buffers (e.g. we can try to use
IEEE80211_MAX_MPDU_LEN_HT_7935with mt76x2u)I think it would be simpler just to allocate 2 pages for 7935B .
We should avoid increasing buffer size to more than PAGE_SIZE for performance reasons. As suggested by Felix what about of setting buf_size to PAGE_SIZE for both sg and non-sg cases and for sg setting usb data size to
SKB_WITH_OVERHEAD(q->buf_size) & (usb_endpoint_maxp() - 1);
Regards,
Lorenzo
Quoted 1 line
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Quoted 5 lines · Lorenzo Bianconi
Quoted 1 line · Lorenzo Bianconi
[...]
Quoted 1 line
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
using sg buffers we can support bigger rx AMSDU size in the future without using huge buffers (e.g. we can try to use
IEEE80211_MAX_MPDU_LEN_HT_7935with mt76x2u)I think it would be simpler just to allocate 2 pages for 7935B .
We should avoid increasing buffer size to more than
PAGE_SIZEfor performance reasons.
Interesting, at what place and how this affect performance negatively ?
Quoted 4 lines
As suggested by Felix what about of setting
buf_sizetoPAGE_SIZEfor both sg and non-sg cases and for sg setting usb data size to
SKB_WITH_OVERHEAD(q->buf_size)& (usb_endpoint_maxp()- 1);
Increasing to PAGE_SIZE for sg looks reasonable to me.
Not sure if understand data_size logic. If this mean 'PAGE_SIZE - usb_endpint_maxp()', looks ok to me as well (for first segment), but would require one extra segment to avoid coping (i.e. 2 pages for 3839 , 3 pages for 7935 ...) If we would like to stay with 128B copy fallback, we can have 1 page for 3839 , 2 for 7935 ...
It would be interesting how frequently AMSDU of max size is sent by remote station. If this is rare situation I would be opting for smaller allocation and 128B copy fallback. If this is frequent for bigger allocation to assure we can always use build_skb().
Stanislaw
Quoted 10 lines · Lorenzo Bianconi
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 v1:
- do not allocate multiple page buffers but rely on fragmented skbs if there is no enough space to manage the AMSDU rx packets@Felix: do you prefer to take this series in your tree or is it better to merge it in wireless-drivers?
Going through wireless-drivers is fine with me.
- Felix