| Message ID | 1439989181-4444-4-git-send-email-emmanuel.grumbach@intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
On Wed, 2015-08-19 at 15:59 +0300, Emmanuel Grumbach wrote: > This allows to release the backpressure on the socket only > when the last segment is released. > Now the truesize looks like this: > if the truesize of the original skb is 65420, all the > segments will have a truesize of 704 (skb itself) and the > last one will have 65420. > > Change-Id: I3c894cf2afc0aedfe7b2a5b992ba41653ff79c0e > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > --- > drivers/net/wireless/iwlwifi/mvm/tx.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c > index 5046833..046e50d 100644 > --- a/drivers/net/wireless/iwlwifi/mvm/tx.c > +++ b/drivers/net/wireless/iwlwifi/mvm/tx.c > @@ -764,7 +764,7 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, > bool ipv6 = skb_shinfo(skb_gso)->gso_type & SKB_GSO_TCPV6; > struct iwl_lso_splitter s = {}; > struct page *hdr_page; > - unsigned int mpdu_sz; > + unsigned int mpdu_sz, sum_truesize = 0; > u8 *hdr_page_pos, *qc, tid; > int i, ret; > > @@ -898,6 +898,7 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, > mpdu_sz, tcp_hdrlen(skb_gso)); > > __skb_queue_tail(mpdus_skb, skb_gso); > + sum_truesize += skb_gso->truesize; > > /* mss bytes have been consumed from the data */ > s.gso_payload_pos = s.mss; > @@ -1034,6 +1035,20 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, > } > > __skb_queue_tail(mpdus_skb, skb); > + sum_truesize += skb->truesize; > + } > + > + /* Release the backpressure on the socket only when > + * the last segment is released. > + */ > + if (skb_gso->destructor == sock_wfree) { > + struct sk_buff *tail = mpdus_skb->prev; > + > + swap(tail->truesize, skb_gso->truesize); > + swap(tail->destructor, skb_gso->destructor); > + swap(tail->sk, skb_gso->sk); > + atomic_add(sum_truesize - skb_gso->truesize, > + &skb_gso->sk->sk_wmem_alloc); > } > > ret = 0; Using existing net/core/tso.c helpers would avoid using this. (BTW TCP packets do not have sock_wfree as destructor but tcp_wfree(), yet we want backpressure mostly for TCP stack (TCP Small Queues)) -- 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
Hello. On 08/19/2015 03:59 PM, Emmanuel Grumbach wrote: > This allows to release the backpressure on the socket only > when the last segment is released. > Now the truesize looks like this: > if the truesize of the original skb is 65420, all the > segments will have a truesize of 704 (skb itself) and the > last one will have 65420. > > Change-Id: I3c894cf2afc0aedfe7b2a5b992ba41653ff79c0e > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > --- > drivers/net/wireless/iwlwifi/mvm/tx.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c > index 5046833..046e50d 100644 > --- a/drivers/net/wireless/iwlwifi/mvm/tx.c > +++ b/drivers/net/wireless/iwlwifi/mvm/tx.c [...] > @@ -1034,6 +1035,20 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, > } > > __skb_queue_tail(mpdus_skb, skb); > + sum_truesize += skb->truesize; > + } > + > + /* Release the backpressure on the socket only when > + * the last segment is released. > + */ > + if (skb_gso->destructor == sock_wfree) { > + struct sk_buff *tail = mpdus_skb->prev; > + > + swap(tail->truesize, skb_gso->truesize); > + swap(tail->destructor, skb_gso->destructor); > + swap(tail->sk, skb_gso->sk); > + atomic_add(sum_truesize - skb_gso->truesize, Please indent using tabs, not spaces. > + &skb_gso->sk->sk_wmem_alloc); > } > > ret = 0; MBR, Sergei -- 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
Hi, On 08/19/2015 10:10 PM, Sergei Shtylyov wrote: > Hello. > > On 08/19/2015 03:59 PM, Emmanuel Grumbach wrote: > >> This allows to release the backpressure on the socket only >> when the last segment is released. >> Now the truesize looks like this: >> if the truesize of the original skb is 65420, all the >> segments will have a truesize of 704 (skb itself) and the >> last one will have 65420. >> >> Change-Id: I3c894cf2afc0aedfe7b2a5b992ba41653ff79c0e >> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> >> --- >> drivers/net/wireless/iwlwifi/mvm/tx.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c >> index 5046833..046e50d 100644 >> --- a/drivers/net/wireless/iwlwifi/mvm/tx.c >> +++ b/drivers/net/wireless/iwlwifi/mvm/tx.c > [...] >> @@ -1034,6 +1035,20 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, >> } >> >> __skb_queue_tail(mpdus_skb, skb); >> + sum_truesize += skb->truesize; >> + } >> + >> + /* Release the backpressure on the socket only when >> + * the last segment is released. >> + */ >> + if (skb_gso->destructor == sock_wfree) { I need to change the destructor function here as per Eric's comment. >> + struct sk_buff *tail = mpdus_skb->prev; >> + >> + swap(tail->truesize, skb_gso->truesize); >> + swap(tail->destructor, skb_gso->destructor); >> + swap(tail->sk, skb_gso->sk); >> + atomic_add(sum_truesize - skb_gso->truesize, > > Please indent using tabs, not spaces. I am happy this is the only flaw you found ;) Sure - I'll fix. And I'll remove the Change-ID and checkpatch will be happy. No worries ;) > >> + &skb_gso->sk->sk_wmem_alloc); >> } >> >> ret = 0; > > MBR, Sergei > > -- 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 08/19/2015 10:12 PM, Grumbach, Emmanuel wrote: >>> This allows to release the backpressure on the socket only >>> when the last segment is released. >>> Now the truesize looks like this: >>> if the truesize of the original skb is 65420, all the >>> segments will have a truesize of 704 (skb itself) and the >>> last one will have 65420. >>> >>> Change-Id: I3c894cf2afc0aedfe7b2a5b992ba41653ff79c0e >>> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> >>> --- >>> drivers/net/wireless/iwlwifi/mvm/tx.c | 17 ++++++++++++++++- >>> 1 file changed, 16 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c >>> index 5046833..046e50d 100644 >>> --- a/drivers/net/wireless/iwlwifi/mvm/tx.c >>> +++ b/drivers/net/wireless/iwlwifi/mvm/tx.c >> [...] >>> @@ -1034,6 +1035,20 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, >>> } >>> >>> __skb_queue_tail(mpdus_skb, skb); >>> + sum_truesize += skb->truesize; >>> + } >>> + >>> + /* Release the backpressure on the socket only when >>> + * the last segment is released. >>> + */ >>> + if (skb_gso->destructor == sock_wfree) { > > I need to change the destructor function here as per Eric's comment. > >>> + struct sk_buff *tail = mpdus_skb->prev; >>> + >>> + swap(tail->truesize, skb_gso->truesize); >>> + swap(tail->destructor, skb_gso->destructor); >>> + swap(tail->sk, skb_gso->sk); >>> + atomic_add(sum_truesize - skb_gso->truesize, >> >> Please indent using tabs, not spaces. > > I am happy this is the only flaw you found ;) It just jumped into my eyes. :-) > Sure - I'll fix. And I'll remove the Change-ID and checkpatch will be > happy. DaveM allows that tag in the networking patches, IIRC. > No worries ;) >> >>> + &skb_gso->sk->sk_wmem_alloc); >>> } >>> >>> ret = 0; MBR, Sergei -- 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 08/19/2015 05:24 PM, Eric Dumazet wrote: > On Wed, 2015-08-19 at 15:59 +0300, Emmanuel Grumbach wrote: >> This allows to release the backpressure on the socket only >> when the last segment is released. >> Now the truesize looks like this: >> if the truesize of the original skb is 65420, all the >> segments will have a truesize of 704 (skb itself) and the >> last one will have 65420. >> >> Change-Id: I3c894cf2afc0aedfe7b2a5b992ba41653ff79c0e >> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> >> --- >> drivers/net/wireless/iwlwifi/mvm/tx.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c >> index 5046833..046e50d 100644 >> --- a/drivers/net/wireless/iwlwifi/mvm/tx.c >> +++ b/drivers/net/wireless/iwlwifi/mvm/tx.c >> @@ -764,7 +764,7 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, >> bool ipv6 = skb_shinfo(skb_gso)->gso_type & SKB_GSO_TCPV6; >> struct iwl_lso_splitter s = {}; >> struct page *hdr_page; >> - unsigned int mpdu_sz; >> + unsigned int mpdu_sz, sum_truesize = 0; >> u8 *hdr_page_pos, *qc, tid; >> int i, ret; >> >> @@ -898,6 +898,7 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, >> mpdu_sz, tcp_hdrlen(skb_gso)); >> >> __skb_queue_tail(mpdus_skb, skb_gso); >> + sum_truesize += skb_gso->truesize; >> >> /* mss bytes have been consumed from the data */ >> s.gso_payload_pos = s.mss; >> @@ -1034,6 +1035,20 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, >> } >> >> __skb_queue_tail(mpdus_skb, skb); >> + sum_truesize += skb->truesize; >> + } >> + >> + /* Release the backpressure on the socket only when >> + * the last segment is released. >> + */ >> + if (skb_gso->destructor == sock_wfree) { >> + struct sk_buff *tail = mpdus_skb->prev; >> + >> + swap(tail->truesize, skb_gso->truesize); >> + swap(tail->destructor, skb_gso->destructor); >> + swap(tail->sk, skb_gso->sk); >> + atomic_add(sum_truesize - skb_gso->truesize, >> + &skb_gso->sk->sk_wmem_alloc); >> } >> >> ret = 0; > > Using existing net/core/tso.c helpers would avoid using this. Hm.. how would net/core/tso.c avoid this? I can't see anything related to truesize there. Note that this work since it is guaranteed that we release the skbs in order. > > (BTW TCP packets do not have sock_wfree as destructor but tcp_wfree(), > yet we want backpressure mostly for TCP stack (TCP Small Queues)) > > I am not sure I follow here. You want me to test: if (skb_gso->destructor == tcp_wfree) ? I checked that code using iperf and saw that I don't get into this if, but I (probably wrongly) assumed that other applications would set a flag on the socket (forgive my ignorance) that would make this if be taken. -- 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 Wed, 2015-08-19 at 19:17 +0000, Grumbach, Emmanuel wrote: > Hm.. how would net/core/tso.c avoid this? Because a driver using these helpers keep around the original LSO packet and frees it normally at TX completion time. > I can't see anything related to truesize there. > Note that this work since it is guaranteed that we release the skbs in > order. > > > > > (BTW TCP packets do not have sock_wfree as destructor but tcp_wfree(), > > yet we want backpressure mostly for TCP stack (TCP Small Queues)) > > > > > > I am not sure I follow here. > You want me to test: > if (skb_gso->destructor == tcp_wfree) ? Yes. Look for example at tcp_gso_segment() (called from skb_gso_segment()) copy_destructor = gso_skb->destructor == tcp_wfree; ... /* Following permits TCP Small Queues to work well with GSO : * The callback to TCP stack will be called at the time last frag * is freed at TX completion, and not right now when gso_skb * is freed by GSO engine */ if (copy_destructor) { swap(gso_skb->sk, skb->sk); swap(gso_skb->destructor, skb->destructor); sum_truesize += skb->truesize; atomic_add(sum_truesize - gso_skb->truesize, &skb->sk->sk_wmem_alloc); } > > I checked that code using iperf and saw that I don't get into this if, > but I (probably wrongly) assumed that other applications would set a > flag on the socket (forgive my ignorance) that would make this if be taken. If you do not see skb->destructor == tcp_wfree, then something is definitely wrong on your setup. -- 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 08/19/2015 11:39 PM, Eric Dumazet wrote: > On Wed, 2015-08-19 at 19:17 +0000, Grumbach, Emmanuel wrote: > >> Hm.. how would net/core/tso.c avoid this? > > Because a driver using these helpers keep around the original LSO packet > and frees it normally at TX completion time. > Which is why I can't really use it. The complexity is that I have to (ieee802.11 specification) split an LSO is several 802.11 packets. The maximal 802.11 packet I can send under ideal condition is 11K long or so. So I *must* generate several 802.11 frames from one single LSO packet. OTOH, I can have more than MSS bytes in a 802.11 A-MSDU. Maybe what would help would be to be able to dynamically change the maximal size of an LSO packet. That would allow the wifi driver to ensure that the LSO can fit in a single 802.11 packet. Note that since the maximal length of the A-MSDU can vary based on link conditions (since there is only one CRC for the whole A-MSDU, you don't want long A-MSDUs in bad link conditions) the driver would need to be able to tell the TCP stack to modify the length of an LSO packet. To me, this sounds to be ... an overkill? I'll sum up all this considerations in the v3 I'll send later today. >> I can't see anything related to truesize there. >> Note that this work since it is guaranteed that we release the skbs in >> order. >> >>> >>> (BTW TCP packets do not have sock_wfree as destructor but tcp_wfree(), >>> yet we want backpressure mostly for TCP stack (TCP Small Queues)) >>> >>> >> >> I am not sure I follow here. >> You want me to test: >> if (skb_gso->destructor == tcp_wfree) ? > > > Yes. > > Look for example at tcp_gso_segment() (called from skb_gso_segment()) > > copy_destructor = gso_skb->destructor == tcp_wfree; > ... > /* Following permits TCP Small Queues to work well with GSO : > * The callback to TCP stack will be called at the time last frag > * is freed at TX completion, and not right now when gso_skb > * is freed by GSO engine > */ > if (copy_destructor) { > swap(gso_skb->sk, skb->sk); > swap(gso_skb->destructor, skb->destructor); > sum_truesize += skb->truesize; > atomic_add(sum_truesize - gso_skb->truesize, > &skb->sk->sk_wmem_alloc); > } > > >> >> I checked that code using iperf and saw that I don't get into this if, >> but I (probably wrongly) assumed that other applications would set a >> flag on the socket (forgive my ignorance) that would make this if be taken. > > If you do not see skb->destructor == tcp_wfree, then something is > definitely wrong on your setup. > I'll check this today. Thanks. > > > -- 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 08/19/2015 11:39 PM, Eric Dumazet wrote: > On Wed, 2015-08-19 at 19:17 +0000, Grumbach, Emmanuel wrote: > >> Hm.. how would net/core/tso.c avoid this? > > Because a driver using these helpers keep around the original LSO packet > and frees it normally at TX completion time. > >> I can't see anything related to truesize there. >> Note that this work since it is guaranteed that we release the skbs in >> order. >> >>> >>> (BTW TCP packets do not have sock_wfree as destructor but tcp_wfree(), >>> yet we want backpressure mostly for TCP stack (TCP Small Queues)) >>> >>> >> >> I am not sure I follow here. >> You want me to test: >> if (skb_gso->destructor == tcp_wfree) ? > > > Yes. > > Look for example at tcp_gso_segment() (called from skb_gso_segment()) > > copy_destructor = gso_skb->destructor == tcp_wfree; > ... > /* Following permits TCP Small Queues to work well with GSO : > * The callback to TCP stack will be called at the time last frag > * is freed at TX completion, and not right now when gso_skb > * is freed by GSO engine > */ > if (copy_destructor) { > swap(gso_skb->sk, skb->sk); > swap(gso_skb->destructor, skb->destructor); > sum_truesize += skb->truesize; > atomic_add(sum_truesize - gso_skb->truesize, > &skb->sk->sk_wmem_alloc); > } > > >> >> I checked that code using iperf and saw that I don't get into this if, >> but I (probably wrongly) assumed that other applications would set a >> flag on the socket (forgive my ignorance) that would make this if be taken. > > If you do not see skb->destructor == tcp_wfree, then something is > definitely wrong on your setup. > tcp_wfree isn't exported. I can change that. It will be a challenge for backport though. Hm.... -- 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 08/20/2015 10:21 AM, Grumbach, Emmanuel wrote: > > > On 08/19/2015 11:39 PM, Eric Dumazet wrote: >> On Wed, 2015-08-19 at 19:17 +0000, Grumbach, Emmanuel wrote: >> >>> Hm.. how would net/core/tso.c avoid this? >> >> Because a driver using these helpers keep around the original LSO packet >> and frees it normally at TX completion time. >> >>> I can't see anything related to truesize there. >>> Note that this work since it is guaranteed that we release the skbs in >>> order. >>> >>>> >>>> (BTW TCP packets do not have sock_wfree as destructor but tcp_wfree(), >>>> yet we want backpressure mostly for TCP stack (TCP Small Queues)) >>>> >>>> >>> >>> I am not sure I follow here. >>> You want me to test: >>> if (skb_gso->destructor == tcp_wfree) ? >> >> >> Yes. >> >> Look for example at tcp_gso_segment() (called from skb_gso_segment()) >> >> copy_destructor = gso_skb->destructor == tcp_wfree; >> ... >> /* Following permits TCP Small Queues to work well with GSO : >> * The callback to TCP stack will be called at the time last frag >> * is freed at TX completion, and not right now when gso_skb >> * is freed by GSO engine >> */ >> if (copy_destructor) { >> swap(gso_skb->sk, skb->sk); >> swap(gso_skb->destructor, skb->destructor); >> sum_truesize += skb->truesize; >> atomic_add(sum_truesize - gso_skb->truesize, >> &skb->sk->sk_wmem_alloc); >> } >> >> >>> >>> I checked that code using iperf and saw that I don't get into this if, >>> but I (probably wrongly) assumed that other applications would set a >>> flag on the socket (forgive my ignorance) that would make this if be taken. >> >> If you do not see skb->destructor == tcp_wfree, then something is >> definitely wrong on your setup. >> > > tcp_wfree isn't exported. I can change that. It will be a challenge for > backport though. Hm.... > But you seem to say that gso_skb->destructor *should* point to tcp_wfree, so maybe testing gso_skb->destructor isn't NULL is good enough? -- 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 Thu, 2015-08-20 at 06:21 +0000, Grumbach, Emmanuel wrote: > > On 08/19/2015 11:39 PM, Eric Dumazet wrote: > > On Wed, 2015-08-19 at 19:17 +0000, Grumbach, Emmanuel wrote: > > > >> Hm.. how would net/core/tso.c avoid this? > > > > Because a driver using these helpers keep around the original LSO packet > > and frees it normally at TX completion time. > > > > Which is why I can't really use it. The complexity is that I have to > (ieee802.11 specification) split an LSO is several 802.11 packets. The > maximal 802.11 packet I can send under ideal condition is 11K long or > so. So I *must* generate several 802.11 frames from one single LSO > packet. OTOH, I can have more than MSS bytes in a 802.11 A-MSDU. > Who said you had to free original packet ? Just keep it around. TCP will work better ( check skb_still_in_host_queue() helper if you want to know why) > Maybe what would help would be to be able to dynamically change the > maximal size of an LSO packet. That would allow the wifi driver to > ensure that the LSO can fit in a single 802.11 packet. Note that since > the maximal length of the A-MSDU can vary based on link conditions > (since there is only one CRC for the whole A-MSDU, you don't want long > A-MSDUs in bad link conditions) the driver would need to be able to tell > the TCP stack to modify the length of an LSO packet. > To me, this sounds to be ... an overkill? It is already doable. Check dev->gso_max_size ( and netif_set_gso_max_size()) Make sure you do not reinvent the wheel ;) -- 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 08/20/2015 04:11 PM, Eric Dumazet wrote: > On Thu, 2015-08-20 at 06:21 +0000, Grumbach, Emmanuel wrote: >> >> On 08/19/2015 11:39 PM, Eric Dumazet wrote: >>> On Wed, 2015-08-19 at 19:17 +0000, Grumbach, Emmanuel wrote: >>> >>>> Hm.. how would net/core/tso.c avoid this? >>> >>> Because a driver using these helpers keep around the original LSO packet >>> and frees it normally at TX completion time. >>> >> >> Which is why I can't really use it. The complexity is that I have to >> (ieee802.11 specification) split an LSO is several 802.11 packets. The >> maximal 802.11 packet I can send under ideal condition is 11K long or >> so. So I *must* generate several 802.11 frames from one single LSO >> packet. OTOH, I can have more than MSS bytes in a 802.11 A-MSDU. >> > > Who said you had to free original packet ? Just keep it around. > > TCP will work better ( check skb_still_in_host_queue() helper if you > want to know why) I do keep the original skb: it becomes the first 802.11 packet generated from that LSO skb. Thing is that it will be freed first and I wanted the *last packet* to release the pressure on the socket. So I guess that skb_still_in_host_queue will still find it and avoid retransmissions at least until the first skb of the LSO is freed. But unless you are fine with releasing the pressing on the socket as soon as the *first* 802.11 skb is freed, I need that code. I'll try to look at dev->gso_max_size that you mentioned below. This can really be a game changer for me. > >> Maybe what would help would be to be able to dynamically change the >> maximal size of an LSO packet. That would allow the wifi driver to >> ensure that the LSO can fit in a single 802.11 packet. Note that since >> the maximal length of the A-MSDU can vary based on link conditions >> (since there is only one CRC for the whole A-MSDU, you don't want long >> A-MSDUs in bad link conditions) the driver would need to be able to tell >> the TCP stack to modify the length of an LSO packet. >> To me, this sounds to be ... an overkill? > > It is already doable. Check dev->gso_max_size ( and > netif_set_gso_max_size()) > > Make sure you do not reinvent the wheel ;) > > > -- 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 Thu, 2015-08-20 at 13:53 +0000, Grumbach, Emmanuel wrote: > I do keep the original skb: it becomes the first 802.11 packet generated > from that LSO skb. Thing is that it will be freed first and I wanted the > *last packet* to release the pressure on the socket. Just change this to free it last. It is that simple. > So I guess that skb_still_in_host_queue will still find it and avoid > retransmissions at least until the first skb of the LSO is freed. > But unless you are fine with releasing the pressing on the socket as > soon as the *first* 802.11 skb is freed, I need that code. > > I'll try to look at dev->gso_max_size that you mentioned below. This can > really be a game changer for me. Honestly, if your TSO patches do not use existing infra, I will NACK them. If existing infra is not good enough, you have the power to change it. Fully re-implementing TSO (or GRO) in a device driver is a non starter. -- 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 08/20/2015 04:53 PM, Grumbach, Emmanuel wrote: > > > On 08/20/2015 04:11 PM, Eric Dumazet wrote: >> On Thu, 2015-08-20 at 06:21 +0000, Grumbach, Emmanuel wrote: >>> >>> On 08/19/2015 11:39 PM, Eric Dumazet wrote: >>>> On Wed, 2015-08-19 at 19:17 +0000, Grumbach, Emmanuel wrote: >>>> >>>>> Hm.. how would net/core/tso.c avoid this? >>>> >>>> Because a driver using these helpers keep around the original LSO packet >>>> and frees it normally at TX completion time. >>>> >>> >>> Which is why I can't really use it. The complexity is that I have to >>> (ieee802.11 specification) split an LSO is several 802.11 packets. The >>> maximal 802.11 packet I can send under ideal condition is 11K long or >>> so. So I *must* generate several 802.11 frames from one single LSO >>> packet. OTOH, I can have more than MSS bytes in a 802.11 A-MSDU. >>> >> >> Who said you had to free original packet ? Just keep it around. >> >> TCP will work better ( check skb_still_in_host_queue() helper if you >> want to know why) > > I do keep the original skb: it becomes the first 802.11 packet generated > from that LSO skb. Thing is that it will be freed first and I wanted the > *last packet* to release the pressure on the socket. > So I guess that skb_still_in_host_queue will still find it and avoid > retransmissions at least until the first skb of the LSO is freed. > But unless you are fine with releasing the pressing on the socket as > soon as the *first* 802.11 skb is freed, I need that code. > > I'll try to look at dev->gso_max_size that you mentioned below. This can > really be a game changer for me. > Err... no :( It won't work for me because the MSS impacts the number of segments which in turns impact the number of time the headers have to be copied which impacts... the A-MSDU maximal size which must be bigger than gso_max_size. So basically, a connection parameter (MSS) impacts a device parameter (gso_max_size). Now, of course, I could give up on small MSS connections and skb_gso_segment() skbs whose MSS is smaller than the default. This means that I give up on LSO in certain cases... I will get back to the drawing board and check what I can do to use / enhance the core infra... But it will be hard to lose functionality / efficiency just to use the core infra... -- 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 Thu, 2015-08-20 at 19:34 +0000, Grumbach, Emmanuel wrote: > > Err... no :( It won't work for me because the MSS impacts the number of > segments which in turns impact the number of time the headers have to be > copied which impacts... the A-MSDU maximal size which must be bigger > than gso_max_size. So basically, a connection parameter (MSS) impacts a > device parameter (gso_max_size). Now, of course, I could give up on > small MSS connections and skb_gso_segment() skbs whose MSS is smaller > than the default. This means that I give up on LSO in certain cases... > We also have dev->gso_max_segs for this kind of problems. Really, you should take closer look. And _if_ some LSO packets must be segmented using skb_gso_segment() in some rare cases, who cares ? > I will get back to the drawing board and check what I can do to use / > enhance the core infra... But it will be hard to lose functionality / > efficiency just to use the core infra... Please take your time. Pushing hard some local hacks is not sustainable. -- 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
diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c index 5046833..046e50d 100644 --- a/drivers/net/wireless/iwlwifi/mvm/tx.c +++ b/drivers/net/wireless/iwlwifi/mvm/tx.c @@ -764,7 +764,7 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, bool ipv6 = skb_shinfo(skb_gso)->gso_type & SKB_GSO_TCPV6; struct iwl_lso_splitter s = {}; struct page *hdr_page; - unsigned int mpdu_sz; + unsigned int mpdu_sz, sum_truesize = 0; u8 *hdr_page_pos, *qc, tid; int i, ret; @@ -898,6 +898,7 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, mpdu_sz, tcp_hdrlen(skb_gso)); __skb_queue_tail(mpdus_skb, skb_gso); + sum_truesize += skb_gso->truesize; /* mss bytes have been consumed from the data */ s.gso_payload_pos = s.mss; @@ -1034,6 +1035,20 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso, } __skb_queue_tail(mpdus_skb, skb); + sum_truesize += skb->truesize; + } + + /* Release the backpressure on the socket only when + * the last segment is released. + */ + if (skb_gso->destructor == sock_wfree) { + struct sk_buff *tail = mpdus_skb->prev; + + swap(tail->truesize, skb_gso->truesize); + swap(tail->destructor, skb_gso->destructor); + swap(tail->sk, skb_gso->sk); + atomic_add(sum_truesize - skb_gso->truesize, + &skb_gso->sk->sk_wmem_alloc); } ret = 0;
This allows to release the backpressure on the socket only when the last segment is released. Now the truesize looks like this: if the truesize of the original skb is 65420, all the segments will have a truesize of 704 (skb itself) and the last one will have 65420. Change-Id: I3c894cf2afc0aedfe7b2a5b992ba41653ff79c0e Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> --- drivers/net/wireless/iwlwifi/mvm/tx.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)