[RFC v2 0/3] add TSO / A-MSDU TX for iwlwifi

[RFC v2 0/3] add TSO / A-MSDU TX for iwlwifi

[Emmanuel Grumbach]

We enable TSO to get a lot of data at once to build A-MSDUs.
Our hardware doesn't have (yet) TCP CSUM offload, so we do
it manually. TSO won't be enabled on hardware that don't
support CSUM offload in release code, computing TCP CSUM
in the driver is just a way to start coding the flows.
This is why the "CSUM offload" implementation in the driver
in so bad in terms of efficiency. I preferred to have the
flows as close as they will be when the hardware will be
able to the CSUM than to try to seek efficiency.
The hardware that will have CSUM offload will still require
the driver to split the skb in software including the
IP / TCP header copy and update etc...

We could have enabled A-MSDU based on xmit-more, but the
rationale of using LSO is that when using pfifo-fast,
the Qdisc gets one packet and dequeues is straight away
which limits the possibility to get a lot of packets at
once. (Am I right here?).

A note about A-MSDUs for non-wireless people:
*********************************************
An A-MSDU is a aggregated frame. It is one big 802.11
packet that contains several subframes. Each subframe
is a TCP segment. One A-MSDU is represented by one single
skb which means that we need to copy / duplicate the TCP
/ IP / SNAP headers in one single skb. This is why those
headers are copied to a separate page: that page is added
multiple times to the skb with different offsets. Each
subframes needs at least 2 frags: 1 for the headers, 1 (or
more) for the payload.

I am quite a newbie in skb handling, so I guess that this
code can be improved. I have tested it decently using iperf,
but this doesn't mean that there are no issues using other
applications. We are enabling pktgen on TCP (using patches
that were sent a year ago or so) to test the different
layouts of the skb (payload partition amongst the header
and the different frags).

I'll be very happy to get comments on that code, this is
why I am sending it to netdev as well since the TSO experts
are there :)

Emmanuel Grumbach (3):
  iwlwifi: mvm: add real TSO implementation
  iwlwifi: mvm: allow to create A-MSDUs from a large send
  iwlwifi: mvm: transfer the truesize to the last TSO segment

 drivers/net/wireless/iwlwifi/mvm/mac80211.c |   3 +-
 drivers/net/wireless/iwlwifi/mvm/sta.c      |   4 +-
 drivers/net/wireless/iwlwifi/mvm/sta.h      |   6 +-
 drivers/net/wireless/iwlwifi/mvm/tx.c       | 669 ++++++++++++++++++++++++++--
 4 files changed, 647 insertions(+), 35 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v2 1/3] iwlwifi: mvm: add real TSO implementation

[Emmanuel Grumbach]

The segmentation is done completely in software. The
driver creates several MPDUs out of a single large send.
Each MPDU is a newly allocated SKB.
A page is allocated to create the headers that need to be
duplicated (SNAP / IP / TCP). The WiFi header is in the
header of the newly created SKBs.

type=feature

Change-Id: I238ffa79cacc5bbdacdfbf3e9673c8d4f02b462a
Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
---
 drivers/net/wireless/iwlwifi/mvm/tx.c | 513 +++++++++++++++++++++++++++++++---
 1 file changed, 481 insertions(+), 32 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c
index 90f0ea1..a63686c 100644
--- a/drivers/net/wireless/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/iwlwifi/mvm/tx.c
@@ -65,6 +65,7 @@
 #include <linux/ieee80211.h>
 #include <linux/etherdevice.h>
 #include <net/tcp.h>
+#include <net/ip.h>
 
 #include "iwl-trans.h"
 #include "iwl-eeprom-parse.h"
@@ -435,32 +436,471 @@ int iwl_mvm_tx_skb_non_sta(struct iwl_mvm *mvm, struct sk_buff *skb)
 	return 0;
 }
 
+/*
+ * Update the IP / TCP headers and recompute the IP header CSUM +
+ * pseudo header CSUM.
+ */
+static void iwl_update_ip_tcph(void *iph, struct tcphdr *tcph, bool ipv6,
+			       unsigned int len, unsigned int tcp_seq_offset,
+			       u16 num_segment)
+{
+	be32_add_cpu(&tcph->seq, tcp_seq_offset);
+
+	if (ipv6) {
+		struct ipv6hdr *iphv6 = iph;
+
+		iphv6->payload_len = cpu_to_be16(len + tcph->doff * 4);
+
+		/* Compute CSUM on the the pseudo-header */
+		tcph->check = ~csum_ipv6_magic(&iphv6->saddr, &iphv6->daddr,
+					       len + tcph->doff * 4,
+					       IPPROTO_TCP, 0);
+	} else {
+		struct iphdr *iphv4 = iph;
+
+		iphv4->tot_len =
+			cpu_to_be16(len + tcph->doff * 4 + iphv4->ihl * 4);
+		be16_add_cpu(&iphv4->id, num_segment);
+		ip_send_check(iphv4);
+
+		/* Compute CSUM on the the pseudo-header */
+		tcph->check = ~csum_tcpudp_magic(iphv4->saddr, iphv4->daddr,
+						 len + tcph->doff * 4,
+						 IPPROTO_TCP, 0);
+	}
+}
+
+/**
+ * struct iwl_lso_splitter - state of the split.
+ * @linear_payload_len: The length of the payload inside the header of the
+ *	original GSO skb.
+ * @gso_frag_num: The fragment number from which to take the data in the
+ *	original GSO skb.
+ * @gso_payload_len: The length of the payload in the original GSO skb.
+ * @gso_payload_pos: The incrementing position in the payload of the original
+ *	GSO skb.
+ * @gso_offset_in_page: The offset in the page of &gso_frag_num.
+ * @gso_current_frag_size: The size of &gso_frag_num.
+ * @gso_offset_in_frag: The offset in the &gso_frag_num.
+ * @frag_in_mpdu: The index of the frag inside the new (split) MPDU.
+ * @mss: The maximal segment size.
+ * @si: Points to the the shared info of the original GSO skb.
+ * @ieee80211_hdr *hdr: Points to the WiFi header.
+ * @gso_nr_frags: The number of frags in the original GSO skb.
+ * @wifi_hdr_iv_len: The length of the WiFi header including IV.
+ * @tcp_fin: True if TCP_FIN is set in the original GSO skb.
+ * @tcp_push: True if TCP_PSH is set in the original GSO skb.
+ */
+struct iwl_lso_splitter {
+	unsigned int linear_payload_len;
+	unsigned int gso_frag_num;
+	unsigned int gso_payload_len;
+	unsigned int gso_payload_pos;
+	unsigned int gso_offset_in_page;
+	unsigned int gso_current_frag_size;
+	unsigned int gso_offset_in_frag;
+	unsigned int frag_in_mpdu;
+	unsigned int mss;
+	struct skb_shared_info *si;
+	struct ieee80211_hdr *hdr;
+	u8 gso_nr_frags;
+	u8 wifi_hdr_iv_len;
+	bool tcp_fin;
+	bool tcp_push;
+};
+
+/*
+ * Adds a TCP segment from skb_gso to skb. All the state is taken from
+ * and fed back to p. This function takes care about the payload only.
+ * This MSDU might already have msdu_sz bytes of payload that come from
+ * the original GSO skb's header.
+ */
+static unsigned int
+iwl_add_tcp_segment(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
+		    struct sk_buff *skb, struct iwl_lso_splitter *p,
+		    unsigned int msdu_sz)
+{
+	while (msdu_sz < p->mss) {
+		unsigned int frag_sz =
+			min_t(unsigned int, p->gso_current_frag_size,
+			      p->mss - msdu_sz);
+
+		if (p->frag_in_mpdu >= mvm->trans->max_skb_frags)
+			return msdu_sz;
+
+		skb_add_rx_frag(skb, p->frag_in_mpdu,
+				skb_frag_page(&p->si->frags[p->gso_frag_num]),
+				p->gso_offset_in_page, frag_sz, 0);
+
+		/* We just added one frag to the mpdu ... */
+		p->frag_in_mpdu++;
+
+		/* ... which is frag_sz byte long ... */
+		msdu_sz += frag_sz;
+
+		/* ... we progress inside the gso frag ... */
+		p->gso_offset_in_page += frag_sz;
+
+		/* ... which is now getting smaller ... */
+		p->gso_current_frag_size -= frag_sz;
+
+		/* ... and we also progress in the global pos counting. */
+		p->gso_payload_pos += frag_sz;
+
+		/*
+		 * This frag has some more bytes that can't fit into this MSDU,
+		 * get a new one, and take a ref since this frag will be
+		 * attached to two different skbs.
+		 */
+		if (p->gso_current_frag_size) {
+			skb_frag_ref(skb_gso, p->gso_frag_num);
+			break;
+		}
+
+		/* We exhausted this frag go the next one ... */
+		p->gso_frag_num++;
+
+		/* ... if it exists ... */
+		if (p->gso_frag_num == p->gso_nr_frags)
+			break;
+
+		/* ... consider its full size ... */
+		p->gso_current_frag_size =
+			skb_frag_size(&p->si->frags[p->gso_frag_num]);
+		/* ... and start from its beginning. */
+		p->gso_offset_in_page =
+			p->si->frags[p->gso_frag_num].page_offset;
+	}
+	return msdu_sz;
+}
+
+/* Hack to compute the CSUM in software. For early testing */
+static __sum16
+iwl_sw_tcp_csum_offload(struct sk_buff *skb,
+			int tcph_offset, int len, int tcp_hdrlen)
+{
+	struct sk_buff *skb1;
+	__wsum csum;
+
+	len += tcp_hdrlen;
+	skb1 = alloc_skb(len, GFP_ATOMIC);
+	BUG_ON(!skb1);
+
+	skb_copy_bits(skb, tcph_offset, skb_put(skb1, len), len);
+
+	skb_set_transport_header(skb1, 0);
+	skb1->csum_start = (unsigned char *)tcp_hdr(skb1) - skb1->head;
+	csum = skb_checksum(skb1, skb_checksum_start_offset(skb1),
+			    skb1->len - skb_checksum_start_offset(skb1),
+			    0);
+	dev_kfree_skb(skb1);
+
+	return csum_fold(csum);
+}
+
+static u8 *get_page_pos(struct page **page, u8 *page_pos, size_t len)
+{
+	if (!*page)
+		goto alloc;
+
+	if (page_pos + len < (u8 *)page_address(*page) + PAGE_SIZE)
+		return page_pos;
+
+	__free_pages(*page, 0);
+
+alloc:
+	*page = alloc_pages(GFP_ATOMIC, 0);
+	if (!*page)
+		return NULL;
+
+	return page_address(*page);
+}
+
+static int iwl_add_msdu(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
+			struct sk_buff *skb, struct page **hdr_page,
+			u8 **hdr_page_pos, int ip_id,
+			struct iwl_lso_splitter *p)
+{
+	unsigned int tcp_seg_sz, snap_ip_tcp_len, copy_sz = 0;
+	bool ipv6 = p->si->gso_type & SKB_GSO_TCPV6;
+	u8 *start_hdr;
+	struct tcphdr *tcph;
+	struct iphdr *iph;
+
+	snap_ip_tcp_len =
+		8 + skb_network_header_len(skb_gso) + tcp_hdrlen(skb_gso);
+	copy_sz = min_t(unsigned int, p->linear_payload_len, p->mss);
+
+	*hdr_page_pos =
+		get_page_pos(hdr_page, *hdr_page_pos,
+			     snap_ip_tcp_len + copy_sz +
+			     sizeof(struct ethhdr) + 4);
+	if (!*hdr_page_pos)
+		return -ENOMEM;
+
+	start_hdr = *hdr_page_pos;
+
+	/*
+	 * Copy SNAP / IP / TCP headers from the original GSO skb to the
+	 * header page.
+	 */
+	skb_copy_bits(skb_gso, p->wifi_hdr_iv_len,
+		      *hdr_page_pos, snap_ip_tcp_len);
+
+	*hdr_page_pos += 8;
+
+	iph = (void *)*hdr_page_pos;
+	*hdr_page_pos += skb_network_header_len(skb_gso);
+
+	tcph = (void *)*hdr_page_pos;
+	*hdr_page_pos += tcp_hdrlen(skb_gso);
+
+	/*
+	 * If the original GSO skb had more than MSS bytes of payload in its
+	 * header, consume it now.
+	 */
+	if (copy_sz) {
+		/*
+		 * Since we copy from the tail pointer and don't update it,
+		 * we can't have more than 1 mss in the linear_payload_len
+		 * at this stage. Hence the limitation of 2 MSS in
+		 * iwl_mvm_tx_tso.
+		 */
+		memcpy(*hdr_page_pos, skb_tail_pointer(skb_gso), copy_sz);
+		*hdr_page_pos += copy_sz;
+		p->gso_payload_pos += copy_sz;
+		p->linear_payload_len -= copy_sz;
+	}
+
+	/* Add frag for SNAP / IP / TCP headers and possibly some payload .. */
+	get_page(*hdr_page);
+	skb_add_rx_frag(skb, p->frag_in_mpdu, *hdr_page,
+			(u8 *)start_hdr - (u8 *)page_address(*hdr_page),
+			*hdr_page_pos - start_hdr, 0);
+	p->frag_in_mpdu++;
+
+	/* .. and now add the payload coming from the frags. */
+	tcp_seg_sz = iwl_add_tcp_segment(mvm, skb_gso, skb, p, copy_sz);
+
+	iwl_update_ip_tcph(iph, tcph, ipv6, tcp_seg_sz,
+			   p->gso_payload_pos - tcp_seg_sz, ip_id);
+
+	/* Last segment, apply the TCP flags that may have been delayed */
+	if (p->gso_payload_pos == p->gso_payload_len) {
+		if (p->tcp_push)
+			tcph->psh = 1;
+		if (p->tcp_fin)
+			tcph->fin = 1;
+	}
+
+	if (IWL_MVM_SW_TX_CSUM_OFFLOAD)
+		tcph->check =
+			iwl_sw_tcp_csum_offload(skb, skb->len -
+						     tcp_seg_sz -
+						     tcp_hdrlen(skb_gso),
+						tcp_seg_sz,
+						tcp_hdrlen(skb_gso));
+
+	return 0;
+}
+
 static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
 			  struct ieee80211_sta *sta,
 			  struct sk_buff_head *mpdus_skb)
 {
-	struct sk_buff *tmp, *next;
-	char cb[sizeof(skb_gso->cb)];
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb_gso);
+	struct ieee80211_key_conf *keyconf = info->control.hw_key;
+	struct ieee80211_hdr *wifi_hdr = (void *)skb_gso->data;
+	bool ipv6 = skb_shinfo(skb_gso)->gso_type & SKB_GSO_TCPV6;
+	struct iwl_lso_splitter s = {};
+	struct page *hdr_page;
+	unsigned int mpdu_sz;
+	u8 *hdr_page_pos;
+	int i, ret;
+
+	s.si = skb_shinfo(skb_gso);
+	s.mss = skb_shinfo(skb_gso)->gso_size;
+	s.gso_nr_frags = skb_shinfo(skb_gso)->nr_frags;
+	s.linear_payload_len = skb_tail_pointer(skb_gso) -
+			  skb_transport_header(skb_gso) - tcp_hdrlen(skb_gso);
+	s.gso_payload_len = s.linear_payload_len + skb_gso->data_len;
+
+	/* more than 2 * mss in the header or no frags ? segment w/o GSO */
+	if (s.linear_payload_len >= 2 * s.mss || !skb_gso->data_len) {
+		struct sk_buff *tmp, *next;
+		char cb[sizeof(skb_gso->cb)];
+
+		memcpy(cb, skb_gso->cb, sizeof(cb));
+		next = skb_gso_segment(skb_gso, 0);
+		if (IS_ERR(next))
+			return -EINVAL;
+		else if (next)
+			consume_skb(skb_gso);
+
+		while (next) {
+			tmp = next;
+			next = tmp->next;
+			memcpy(tmp->cb, cb, sizeof(tmp->cb));
+
+			tmp->prev = NULL;
+			tmp->next = NULL;
+
+			__skb_queue_tail(mpdus_skb, tmp);
+		}
+		return 0;
+	}
+
+	/*
+	 * We got an GSO skb that may or may not have payload in its header.
+	 * Re-use the same skb and chop the payload that can't fit into 1 MSS.
+	 * First take the payload from the header...
+	 */
+	mpdu_sz = min_t(unsigned int, s.linear_payload_len, s.mss);
+	s.linear_payload_len -= mpdu_sz;
+	skb_gso->data_len = 0;
+
+	/*
+	 * ... and add now payload from the frags to get up to 1 MSS.
+	 * If we have one mss already, mpdu_sz will be s.mss and
+	 * this loop won't do much.
+	 * Make sure we don't go beyond the limit of the origin GSO
+	 * skb.
+	 */
+	s.gso_offset_in_frag = 0;
+	while (s.gso_frag_num < s.gso_nr_frags &&
+	       s.gso_frag_num <= mvm->trans->max_skb_frags) {
+		unsigned int frag_sz =
+			skb_frag_size(&s.si->frags[s.gso_frag_num]);
+
+		if ((s.mss - mpdu_sz) >= frag_sz) {
+			/* there is enough room for this entire frag */
+			mpdu_sz += frag_sz;
+			skb_gso->data_len += frag_sz;
+			s.gso_frag_num++;
+			continue;
+		}
 
-	memcpy(cb, skb_gso->cb, sizeof(cb));
-	next = skb_gso_segment(skb_gso, 0);
-	if (IS_ERR(next))
+		/*
+		 * Only part of this frag will fit and
+		 * we now have a complete mss
+		 */
+		s.gso_offset_in_frag = s.mss - mpdu_sz;
+		skb_gso->data_len += (s.mss - mpdu_sz);
+		mpdu_sz = s.mss;
+		break;
+	}
+
+	/* We have only one mss or even less? */
+	if (WARN(s.gso_frag_num == s.gso_nr_frags,
+		 "frag: %d payload: %d\n", s.gso_frag_num, s.gso_payload_len))
 		return -EINVAL;
-	else if (next)
-		consume_skb(skb_gso);
 
-	while (next) {
-		tmp = next;
-		next = tmp->next;
-		memcpy(tmp->cb, cb, sizeof(tmp->cb));
+	/* remember the size of the remainder of the frag */
+	s.gso_current_frag_size = skb_frag_size(&s.si->frags[s.gso_frag_num]) -
+		s.gso_offset_in_frag;
+
+	/* shorten the skb_gso's frag to fit the mss */
+	skb_frag_size_set(&s.si->frags[s.gso_frag_num], s.gso_offset_in_frag);
+
+	/* translate to offset from the start of the page */
+	s.gso_offset_in_page =
+		s.gso_offset_in_frag + s.si->frags[s.gso_frag_num].page_offset;
+
+	/* remove all the other frags from skb_gso */
+	skb_shinfo(skb_gso)->nr_frags = s.gso_frag_num + 1;
+
+	/* FIXME: do we need to ref the last frag if no bytes of it were added
+	 * to skb_gso: e.g. when the previous segment was completely consumed
+	 * closed the first MSS?
+	 */
+
+	/* take a temp ref to the last frag */
+	skb_frag_ref(skb_gso, s.gso_frag_num);
+
+	/* update the skb_gso's length fields */
+	skb_gso->len -= (s.gso_payload_len - mpdu_sz);
+
+	/*
+	 * If there is still some payload in the header, update the tail to the
+	 * end of MSS.
+	 */
+	if (s.linear_payload_len)
+		skb_gso->tail -= s.linear_payload_len;
+
+	/*
+	 * Clear the bits in the TCP header that we delay for
+	 * the last segment.
+	 */
+	s.tcp_push = tcp_hdr(skb_gso)->psh;
+	s.tcp_fin = tcp_hdr(skb_gso)->fin;
+	tcp_hdr(skb_gso)->psh = 0;
+	tcp_hdr(skb_gso)->fin = 0;
+
+	/* update the IP / TCP header with the new length */
+	iwl_update_ip_tcph(ip_hdr(skb_gso), tcp_hdr(skb_gso),
+			   ipv6, mpdu_sz, 0, 0);
+
+	if (IWL_MVM_SW_TX_CSUM_OFFLOAD)
+		tcp_hdr(skb_gso)->check =
+			iwl_sw_tcp_csum_offload(skb_gso,
+						(u8 *)tcp_hdr(skb_gso) -
+						(u8 *)skb_gso->data,
+						mpdu_sz, tcp_hdrlen(skb_gso));
+
+	__skb_queue_tail(mpdus_skb, skb_gso);
+
+	/* mss bytes have been consumed from the data */
+	s.gso_payload_pos = s.mss;
+	i = 0;
+
+	hdr_page = NULL;
+	hdr_page_pos = get_page_pos(&hdr_page, NULL, 1);
+	if (!hdr_page_pos)
+		return -ENOMEM;
+
+	s.hdr = wifi_hdr;
+	s.wifi_hdr_iv_len = ieee80211_hdrlen(wifi_hdr->frame_control);
+	if (keyconf && (keyconf->cipher == WLAN_CIPHER_SUITE_CCMP ||
+			keyconf->cipher == WLAN_CIPHER_SUITE_CCMP_256))
+		s.wifi_hdr_iv_len += IEEE80211_CCMP_HDR_LEN;
+
+	while (s.gso_payload_pos < s.gso_payload_len) {
+		struct sk_buff *skb = dev_alloc_skb(s.wifi_hdr_iv_len);
+		int l;
+
+		s.frag_in_mpdu = 0;
 
-		tmp->prev = NULL;
-		tmp->next = NULL;
+		if (WARN_ON(s.gso_frag_num >= s.gso_nr_frags))
+			break;
+
+		if (!skb) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
-		__skb_queue_tail(mpdus_skb, tmp);
+		memcpy(skb->cb, skb_gso->cb, sizeof(skb->cb));
+
+		/* new MPDU - add WiFi header + IV */
+		memcpy(skb_put(skb, s.wifi_hdr_iv_len),
+		       wifi_hdr, s.wifi_hdr_iv_len);
+
+		l = iwl_add_msdu(mvm, skb_gso, skb, &hdr_page,
+				 &hdr_page_pos, i++, &s);
+		if (l < 0) {
+			skb_queue_purge(mpdus_skb);
+			ret = l;
+			goto out;
+		}
+
+		__skb_queue_tail(mpdus_skb, skb);
 	}
 
-	return 0;
+	ret = 0;
+
+out:
+	if (hdr_page)
+		__free_pages(hdr_page, 0);
+	return ret;
 }
 
 static int iwl_skb_ensure_writable(struct sk_buff *skb, int write_len)
@@ -564,6 +1004,26 @@ drop:
 	return -1;
 }
 
+static int iwl_mvm_tx_csum_and_send(struct iwl_mvm *mvm, struct sk_buff *skb,
+				    struct ieee80211_sta *sta)
+{
+	if (skb->ip_summed == CHECKSUM_PARTIAL &&
+	    IWL_MVM_SW_TX_CSUM_OFFLOAD) {
+		int offs = skb_checksum_start_offset(skb);
+		int csum_offs = offs + skb->csum_offset;
+		__wsum csum;
+
+		if (iwl_skb_ensure_writable(skb, csum_offs + sizeof(__sum16)))
+			return -1;
+
+		csum = skb_checksum(skb, offs, skb->len - offs, 0);
+
+		*(__sum16 *)(skb->data + csum_offs) = csum_fold(csum);
+	}
+
+	return iwl_mvm_tx_mpdu(mvm, skb, sta);
+}
+
 /*
  * Sets the fields in the Tx cmd that are crypto related
  */
@@ -581,26 +1041,15 @@ int iwl_mvm_tx_skb(struct iwl_mvm *mvm, struct sk_buff *skb,
 	if (WARN_ON_ONCE(mvmsta->sta_id == IWL_MVM_STATION_COUNT))
 		return -1;
 
+	if (!skb_is_gso(skb))
+		return iwl_mvm_tx_csum_and_send(mvm, skb, sta);
+
 	payload_len = skb_tail_pointer(skb) - skb_transport_header(skb) -
 		tcp_hdrlen(skb) + skb->data_len;
 
-	if (!skb_is_gso(skb) || payload_len <= skb_shinfo(skb)->gso_size) {
-		if (skb->ip_summed == CHECKSUM_PARTIAL) {
-			int offs = skb_checksum_start_offset(skb);
-			int csum_offs = offs + skb->csum_offset;
-			__wsum csum;
-
-			if (iwl_skb_ensure_writable(skb, csum_offs +
-							 sizeof(__sum16)))
-				return -1;
-
-			csum = skb_checksum(skb, offs, skb->len - offs, 0);
-
-			*(__sum16 *)(skb->data + csum_offs) = csum_fold(csum);
-		}
-
-		return iwl_mvm_tx_mpdu(mvm, skb, sta);
-	}
+	/* This packet is smaller than MSS, one MPDU is enough */
+	if (payload_len <= skb_shinfo(skb)->gso_size)
+		return iwl_mvm_tx_csum_and_send(mvm, skb, sta);
 
 	__skb_queue_head_init(&mpdus_skbs);
 
-- 
2.1.4

Re: [RFC v2 1/3] iwlwifi: mvm: add real TSO implementation

[Eric Dumazet]

On Wed, 2015-08-19 at 15:59 +0300, Emmanuel Grumbach wrote:
> The segmentation is done completely in software. The
> driver creates several MPDUs out of a single large send.
> Each MPDU is a newly allocated SKB.
> A page is allocated to create the headers that need to be
> duplicated (SNAP / IP / TCP). The WiFi header is in the
> header of the newly created SKBs.
> 
> type=feature
> 
> Change-Id: I238ffa79cacc5bbdacdfbf3e9673c8d4f02b462a
> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/net/wireless/iwlwifi/mvm/tx.c | 513 +++++++++++++++++++++++++++++++---
>  1 file changed, 481 insertions(+), 32 deletions(-)

Ouch dynamic allocations while doing xmit are certainly not needed.
Your driver should pre-allocated space for headers.

Drivers willing to implement tso have to use net/core/tso.c provided
helpers.

$ git grep -n tso_build_hdr
drivers/net/ethernet/cavium/thunder/nicvf_queues.c:1030:                tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
drivers/net/ethernet/freescale/fec_main.c:729:          tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
drivers/net/ethernet/marvell/mv643xx_eth.c:842:         tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
drivers/net/ethernet/marvell/mvneta.c:1650:             tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
include/net/tso.h:15:void tso_build_hdr(struct sk_buff *skb, char *hdr, struct tso_t *tso,
net/core/tso.c:14:void tso_build_hdr(struct sk_buff *skb, char *hdr, struct tso_t *tso,
net/core/tso.c:37:EXPORT_SYMBOL(tso_build_hdr);


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 1/3] iwlwifi: mvm: add real TSO implementation

[Eric Dumazet]

On Wed, 2015-08-19 at 07:17 -0700, Eric Dumazet wrote:
> On Wed, 2015-08-19 at 15:59 +0300, Emmanuel Grumbach wrote:
> > The segmentation is done completely in software. The
> > driver creates several MPDUs out of a single large send.
> > Each MPDU is a newly allocated SKB.
> > A page is allocated to create the headers that need to be
> > duplicated (SNAP / IP / TCP). The WiFi header is in the
> > header of the newly created SKBs.
> > 
> > type=feature
> > 
> > Change-Id: I238ffa79cacc5bbdacdfbf3e9673c8d4f02b462a
> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/net/wireless/iwlwifi/mvm/tx.c | 513 +++++++++++++++++++++++++++++++---
> >  1 file changed, 481 insertions(+), 32 deletions(-)
> 
> Ouch dynamic allocations while doing xmit are certainly not needed.
> Your driver should pre-allocated space for headers.
> 
> Drivers willing to implement tso have to use net/core/tso.c provided
> helpers.
> 
> $ git grep -n tso_build_hdr
> drivers/net/ethernet/cavium/thunder/nicvf_queues.c:1030:                tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
> drivers/net/ethernet/freescale/fec_main.c:729:          tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
> drivers/net/ethernet/marvell/mv643xx_eth.c:842:         tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
> drivers/net/ethernet/marvell/mvneta.c:1650:             tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
> include/net/tso.h:15:void tso_build_hdr(struct sk_buff *skb, char *hdr, struct tso_t *tso,
> net/core/tso.c:14:void tso_build_hdr(struct sk_buff *skb, char *hdr, struct tso_t *tso,
> net/core/tso.c:37:EXPORT_SYMBOL(tso_build_hdr);
> 

Look at commit 2adb719d74f6e174071e5c913290b9bbd8c2c0e8 for a typical
use of these helpers.


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 1/3] iwlwifi: mvm: add real TSO implementation

[Grumbach, Emmanuel]

On 08/19/2015 05:18 PM, Eric Dumazet wrote:
> On Wed, 2015-08-19 at 15:59 +0300, Emmanuel Grumbach wrote:
>> The segmentation is done completely in software. The
>> driver creates several MPDUs out of a single large send.
>> Each MPDU is a newly allocated SKB.
>> A page is allocated to create the headers that need to be
>> duplicated (SNAP / IP / TCP). The WiFi header is in the
>> header of the newly created SKBs.
>>
>> type=feature
>>
>> Change-Id: I238ffa79cacc5bbdacdfbf3e9673c8d4f02b462a
>> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/net/wireless/iwlwifi/mvm/tx.c | 513 +++++++++++++++++++++++++++++++---
>>  1 file changed, 481 insertions(+), 32 deletions(-)
> 
> Ouch dynamic allocations while doing xmit are certainly not needed.
> Your driver should pre-allocated space for headers.

This is right as long as you don't need *several* headers in one single
skb. In the case of A-MSDU, I need to have several TCP / IP / SNAP
headers in the same skb. At least that's how my HW layer in the driver
is built. See the other thread.

> 
> Drivers willing to implement tso have to use net/core/tso.c provided
> helpers.
> 
> $ git grep -n tso_build_hdr
> drivers/net/ethernet/cavium/thunder/nicvf_queues.c:1030:                tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
> drivers/net/ethernet/freescale/fec_main.c:729:          tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
> drivers/net/ethernet/marvell/mv643xx_eth.c:842:         tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
> drivers/net/ethernet/marvell/mvneta.c:1650:             tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
> include/net/tso.h:15:void tso_build_hdr(struct sk_buff *skb, char *hdr, struct tso_t *tso,
> net/core/tso.c:14:void tso_build_hdr(struct sk_buff *skb, char *hdr, struct tso_t *tso,
> net/core/tso.c:37:EXPORT_SYMBOL(tso_build_hdr);
> 
> 

This looks promising indeed. I'll take a close look.
Thanks a bunch.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v2 2/3] iwlwifi: mvm: allow to create A-MSDUs from a large send

[Emmanuel Grumbach]

Now that we can get a big chunk of data from the network
stack, we can create an A-MSDU out of it. The purpose is to
get a throughput improvement since sending one single A-MSDU
is more efficient than sending several MSDUs at least under
ideal link conditions.

type=feature

Change-Id: I5ea1b1132a57542187cd4c34c5299dbf44fe8b01
Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
---
 drivers/net/wireless/iwlwifi/mvm/mac80211.c |   3 +-
 drivers/net/wireless/iwlwifi/mvm/sta.c      |   4 +-
 drivers/net/wireless/iwlwifi/mvm/sta.h      |   6 +-
 drivers/net/wireless/iwlwifi/mvm/tx.c       | 159 ++++++++++++++++++++++++++--
 4 files changed, 160 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/iwlwifi/mvm/mac80211.c
index 3dd4e97..dd15e04 100644
--- a/drivers/net/wireless/iwlwifi/mvm/mac80211.c
+++ b/drivers/net/wireless/iwlwifi/mvm/mac80211.c
@@ -925,7 +925,8 @@ static int iwl_mvm_mac_ampdu_action(struct ieee80211_hw *hw,
 		ret = iwl_mvm_sta_tx_agg_flush(mvm, vif, sta, tid);
 		break;
 	case IEEE80211_AMPDU_TX_OPERATIONAL:
-		ret = iwl_mvm_sta_tx_agg_oper(mvm, vif, sta, tid, buf_size);
+		ret = iwl_mvm_sta_tx_agg_oper(mvm, vif, sta, tid,
+					      buf_size, amsdu);
 		break;
 	default:
 		WARN_ON_ONCE(1);
diff --git a/drivers/net/wireless/iwlwifi/mvm/sta.c b/drivers/net/wireless/iwlwifi/mvm/sta.c
index df216cd..606fc09 100644
--- a/drivers/net/wireless/iwlwifi/mvm/sta.c
+++ b/drivers/net/wireless/iwlwifi/mvm/sta.c
@@ -976,7 +976,8 @@ int iwl_mvm_sta_tx_agg_start(struct iwl_mvm *mvm, struct ieee80211_vif *vif,
 }
 
 int iwl_mvm_sta_tx_agg_oper(struct iwl_mvm *mvm, struct ieee80211_vif *vif,
-			    struct ieee80211_sta *sta, u16 tid, u8 buf_size)
+			    struct ieee80211_sta *sta, u16 tid, u8 buf_size,
+			    bool amsdu)
 {
 	struct iwl_mvm_sta *mvmsta = iwl_mvm_sta_from_mac80211(sta);
 	struct iwl_mvm_tid_data *tid_data = &mvmsta->tid_data[tid];
@@ -995,6 +996,7 @@ int iwl_mvm_sta_tx_agg_oper(struct iwl_mvm *mvm, struct ieee80211_vif *vif,
 	queue = tid_data->txq_id;
 	tid_data->state = IWL_AGG_ON;
 	mvmsta->agg_tids |= BIT(tid);
+	tid_data->amsdu_in_ampdu_allowed = amsdu;
 	tid_data->ssn = 0xffff;
 	spin_unlock_bh(&mvmsta->lock);
 
diff --git a/drivers/net/wireless/iwlwifi/mvm/sta.h b/drivers/net/wireless/iwlwifi/mvm/sta.h
index eedb215..26d1e31 100644
--- a/drivers/net/wireless/iwlwifi/mvm/sta.h
+++ b/drivers/net/wireless/iwlwifi/mvm/sta.h
@@ -258,6 +258,8 @@ enum iwl_mvm_agg_state {
  *	Tx response (TX_CMD), and the block ack notification (COMPRESSED_BA).
  * @reduced_tpc: Reduced tx power. Holds the data between the
  *	Tx response (TX_CMD), and the block ack notification (COMPRESSED_BA).
+ * @amsdu_in_ampdu_allowed: true if A-MSDU in A-MPDU is allowed. Relevant only
+ *	if &state is %IWL_AGG_ON.
  * @state: state of the BA agreement establishment / tear down.
  * @txq_id: Tx queue used by the BA session
  * @ssn: the first packet to be sent in AGG HW queue in Tx AGG start flow, or
@@ -272,6 +274,7 @@ struct iwl_mvm_tid_data {
 	/* The rest is Tx AGG related */
 	u32 rate_n_flags;
 	u8 reduced_tpc;
+	bool amsdu_in_ampdu_allowed;
 	enum iwl_mvm_agg_state state;
 	u16 txq_id;
 	u16 ssn;
@@ -387,7 +390,8 @@ int iwl_mvm_sta_rx_agg(struct iwl_mvm *mvm, struct ieee80211_sta *sta,
 int iwl_mvm_sta_tx_agg_start(struct iwl_mvm *mvm, struct ieee80211_vif *vif,
 			struct ieee80211_sta *sta, u16 tid, u16 *ssn);
 int iwl_mvm_sta_tx_agg_oper(struct iwl_mvm *mvm, struct ieee80211_vif *vif,
-			struct ieee80211_sta *sta, u16 tid, u8 buf_size);
+			    struct ieee80211_sta *sta, u16 tid, u8 buf_size,
+			    bool amsdu);
 int iwl_mvm_sta_tx_agg_stop(struct iwl_mvm *mvm, struct ieee80211_vif *vif,
 			    struct ieee80211_sta *sta, u16 tid);
 int iwl_mvm_sta_tx_agg_flush(struct iwl_mvm *mvm, struct ieee80211_vif *vif,
diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c
index a63686c..5046833 100644
--- a/drivers/net/wireless/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/iwlwifi/mvm/tx.c
@@ -488,8 +488,10 @@ static void iwl_update_ip_tcph(void *iph, struct tcphdr *tcph, bool ipv6,
  * @ieee80211_hdr *hdr: Points to the WiFi header.
  * @gso_nr_frags: The number of frags in the original GSO skb.
  * @wifi_hdr_iv_len: The length of the WiFi header including IV.
+ * @amsdu_pad: Number of bytes for the A-MSDU subframe
  * @tcp_fin: True if TCP_FIN is set in the original GSO skb.
  * @tcp_push: True if TCP_PSH is set in the original GSO skb.
+ * @amsdu: True if we are building an A-MSDU
  */
 struct iwl_lso_splitter {
 	unsigned int linear_payload_len;
@@ -505,8 +507,10 @@ struct iwl_lso_splitter {
 	struct ieee80211_hdr *hdr;
 	u8 gso_nr_frags;
 	u8 wifi_hdr_iv_len;
+	u8 amsdu_pad;
 	bool tcp_fin;
 	bool tcp_push;
+	bool amsdu;
 };
 
 /*
@@ -621,9 +625,10 @@ static int iwl_add_msdu(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
 			u8 **hdr_page_pos, int ip_id,
 			struct iwl_lso_splitter *p)
 {
-	unsigned int tcp_seg_sz, snap_ip_tcp_len, copy_sz = 0;
+	unsigned int tcp_seg_sz, snap_ip_tcp_len, subframe_sz, copy_sz = 0;
 	bool ipv6 = p->si->gso_type & SKB_GSO_TCPV6;
 	u8 *start_hdr;
+	__be16 *length = NULL;
 	struct tcphdr *tcph;
 	struct iphdr *iph;
 
@@ -640,6 +645,45 @@ static int iwl_add_msdu(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
 
 	start_hdr = *hdr_page_pos;
 
+	subframe_sz = snap_ip_tcp_len;
+
+	if (p->amsdu) {
+		memset(*hdr_page_pos, 0, p->amsdu_pad);
+		*hdr_page_pos += p->amsdu_pad;
+		switch (p->hdr->frame_control &
+			cpu_to_le16(IEEE80211_FCTL_TODS |
+				    IEEE80211_FCTL_FROMDS)) {
+		/* STA */
+		case cpu_to_le16(IEEE80211_FCTL_TODS):
+			memcpy(*hdr_page_pos, p->hdr->addr3, ETH_ALEN);
+			*hdr_page_pos += ETH_ALEN;
+
+			memcpy(*hdr_page_pos, p->hdr->addr2, ETH_ALEN);
+			*hdr_page_pos += ETH_ALEN;
+			break;
+		/* AP */
+		case cpu_to_le16(IEEE80211_FCTL_FROMDS):
+			memcpy(*hdr_page_pos, p->hdr->addr1, ETH_ALEN);
+			*hdr_page_pos += ETH_ALEN;
+
+			memcpy(*hdr_page_pos, p->hdr->addr3, ETH_ALEN);
+			*hdr_page_pos += ETH_ALEN;
+			break;
+		/* TDLS or IBSS */
+		case cpu_to_le16(0):
+			memcpy(*hdr_page_pos, p->hdr->addr1, ETH_ALEN);
+			*hdr_page_pos += ETH_ALEN;
+
+			memcpy(*hdr_page_pos, p->hdr->addr2, ETH_ALEN);
+			*hdr_page_pos += ETH_ALEN;
+			break;
+		}
+
+		length = (void *)*hdr_page_pos;
+		*hdr_page_pos += sizeof(*length);
+		subframe_sz += sizeof(struct ethhdr);
+	}
+
 	/*
 	 * Copy SNAP / IP / TCP headers from the original GSO skb to the
 	 * header page.
@@ -681,6 +725,11 @@ static int iwl_add_msdu(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
 
 	/* .. and now add the payload coming from the frags. */
 	tcp_seg_sz = iwl_add_tcp_segment(mvm, skb_gso, skb, p, copy_sz);
+	subframe_sz += tcp_seg_sz;
+	p->amsdu_pad = (4 - (subframe_sz)) & 0x3;
+
+	if (length)
+		*length = cpu_to_be16(subframe_sz - sizeof(struct ethhdr));
 
 	iwl_update_ip_tcph(iph, tcph, ipv6, tcp_seg_sz,
 			   p->gso_payload_pos - tcp_seg_sz, ip_id);
@@ -701,13 +750,14 @@ static int iwl_add_msdu(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
 						tcp_seg_sz,
 						tcp_hdrlen(skb_gso));
 
-	return 0;
+	return subframe_sz;
 }
 
 static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
 			  struct ieee80211_sta *sta,
 			  struct sk_buff_head *mpdus_skb)
 {
+	struct iwl_mvm_sta *mvmsta = iwl_mvm_sta_from_mac80211(sta);
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb_gso);
 	struct ieee80211_key_conf *keyconf = info->control.hw_key;
 	struct ieee80211_hdr *wifi_hdr = (void *)skb_gso->data;
@@ -715,7 +765,7 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
 	struct iwl_lso_splitter s = {};
 	struct page *hdr_page;
 	unsigned int mpdu_sz;
-	u8 *hdr_page_pos;
+	u8 *hdr_page_pos, *qc, tid;
 	int i, ret;
 
 	s.si = skb_shinfo(skb_gso);
@@ -864,9 +914,22 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
 			keyconf->cipher == WLAN_CIPHER_SUITE_CCMP_256))
 		s.wifi_hdr_iv_len += IEEE80211_CCMP_HDR_LEN;
 
+	qc = ieee80211_get_qos_ctl(s.hdr);
+	tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
+	if (WARN_ON_ONCE(tid >= IWL_MAX_TID_COUNT)) {
+		ret = -1;
+		goto out;
+	}
+
+	spin_lock(&mvmsta->lock);
+	s.amsdu = !(info->flags & IEEE80211_TX_CTL_AMPDU) ||
+		mvmsta->tid_data[tid].amsdu_in_ampdu_allowed;
+
+	spin_unlock(&mvmsta->lock);
+
 	while (s.gso_payload_pos < s.gso_payload_len) {
 		struct sk_buff *skb = dev_alloc_skb(s.wifi_hdr_iv_len);
-		int l;
+		unsigned int ip_tcp_snap_hdrlen, amsdu_sz, max_amsdu_len;
 
 		s.frag_in_mpdu = 0;
 
@@ -884,14 +947,92 @@ static int iwl_mvm_tx_tso(struct iwl_mvm *mvm, struct sk_buff *skb_gso,
 		memcpy(skb_put(skb, s.wifi_hdr_iv_len),
 		       wifi_hdr, s.wifi_hdr_iv_len);
 
-		l = iwl_add_msdu(mvm, skb_gso, skb, &hdr_page,
-				 &hdr_page_pos, i++, &s);
-		if (l < 0) {
-			skb_queue_purge(mpdus_skb);
-			ret = l;
+		/* No need to have an AMSDU if we have at most mss bytes */
+		if (s.gso_payload_len - s.gso_payload_pos <= s.mss)
+			s.amsdu = false;
+
+		/*
+		 * Limit A-MSDU in A-MPDU to 4095 bytes when VHT is not
+		 * supported. This is a spec requirement (IEEE 802.11-2015
+		 * section 8.7.3 NOTE 3).
+		 */
+		if (info->flags & IEEE80211_TX_CTL_AMPDU &&
+		    mvmsta->tid_data[tid].amsdu_in_ampdu_allowed &&
+		    !sta->vht_cap.vht_supported)
+			max_amsdu_len = 4095;
+		else
+			max_amsdu_len = 0;
+
+		if (max_amsdu_len)
+			max_amsdu_len = min_t(unsigned int, sta->max_amsdu_len,
+					      max_amsdu_len);
+		else
+			max_amsdu_len = sta->max_amsdu_len;
+
+		/*
+		 * Technically, this will allow to have an A-MSDU will only
+		 * one subframe. But this won't happen on valid limits. Only
+		 * on custom limit set by debugfs. We could test that there is
+		 * enough room for the subframe header + data headers etc...
+		 * but these tests cost, and this is a hot path.
+		 */
+		if (!max_amsdu_len || !s.amsdu) {
+			int l;
+
+			s.amsdu = false;
+			l = iwl_add_msdu(mvm, skb_gso, skb, &hdr_page,
+					 &hdr_page_pos, i++, &s);
+			if (l < 0) {
+				skb_queue_purge(mpdus_skb);
+				ret = l;
+				goto out;
+			}
+
+			__skb_queue_tail(mpdus_skb, skb);
+			continue;
+		}
+
+		if (WARN_ON_ONCE(max_amsdu_len < s.mss)) {
+			ret = -1;
 			goto out;
 		}
 
+		qc = ieee80211_get_qos_ctl((void *)skb->data);
+		*qc |= IEEE80211_QOS_CTL_A_MSDU_PRESENT;
+
+		amsdu_sz = 0;
+		s.amsdu_pad = 0;
+		ip_tcp_snap_hdrlen =
+			8 + ip_hdrlen(skb_gso) + tcp_hdrlen(skb_gso);
+
+		i = 0;
+
+		/*
+		 * Make sure we have enough room for
+		 * ethernet header, SNAP header, IP header, TCP header and MSS.
+		 * Make sure we don't add more MSDUs than allowed
+		 */
+		while (amsdu_sz + sizeof(struct ethhdr) + s.mss +
+		       ip_tcp_snap_hdrlen < max_amsdu_len &&
+		       (!sta->max_amsdu_subframes ||
+			i < sta->max_amsdu_subframes) &&
+		       s.gso_payload_pos < s.gso_payload_len) {
+			unsigned int l;
+
+			if (s.frag_in_mpdu + 1 >= mvm->trans->max_skb_frags)
+				break;
+
+			l = iwl_add_msdu(mvm, skb_gso, skb, &hdr_page,
+					 &hdr_page_pos, i++, &s);
+			if (l < 0) {
+				skb_queue_purge(mpdus_skb);
+				ret = l;
+				goto out;
+			}
+
+			amsdu_sz += l + s.amsdu_pad;
+		}
+
 		__skb_queue_tail(mpdus_skb, skb);
 	}
 
-- 
2.1.4

[RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Emmanuel Grumbach]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Eric Dumazet]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Grumbach, Emmanuel]

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.

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Eric Dumazet]

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.

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Grumbach, Emmanuel]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Eric Dumazet]

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 ;)

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Grumbach, Emmanuel]

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 ;)
> 
> 
> 

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Eric Dumazet]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Grumbach, Emmanuel]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Eric Dumazet]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Grumbach, Emmanuel]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Grumbach, Emmanuel]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Sergei Shtylyov]

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

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Grumbach, Emmanuel]

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
> 
> 

Re: [RFC v2 3/3] iwlwifi: mvm: transfer the truesize to the last TSO segment

[Sergei Shtylyov]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> ---
>>>    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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 0/3] add TSO / A-MSDU TX for iwlwifi

[Eric Dumazet]

On Wed, 2015-08-19 at 15:59 +0300, Emmanuel Grumbach wrote:

> We could have enabled A-MSDU based on xmit-more, but the
> rationale of using LSO is that when using pfifo-fast,
> the Qdisc gets one packet and dequeues is straight away
> which limits the possibility to get a lot of packets at
> once. (Am I right here?).

No, you are not ;)

Key point for xmit_more is BQL being implemented in your driver.

Relevant code is in try_bulk_dequeue_skb()

Re: [RFC v2 0/3] add TSO / A-MSDU TX for iwlwifi

[Grumbach, Emmanuel]

Hi Eric,

First, thank you a lot for your comments.

On 08/19/2015 05:14 PM, Eric Dumazet wrote:
> On Wed, 2015-08-19 at 15:59 +0300, Emmanuel Grumbach wrote:
> 
>> We could have enabled A-MSDU based on xmit-more, but the
>> rationale of using LSO is that when using pfifo-fast,
>> the Qdisc gets one packet and dequeues is straight away
>> which limits the possibility to get a lot of packets at
>> once. (Am I right here?).
> 
> No, you are not ;)
> 
> Key point for xmit_more is BQL being implemented in your driver.
> 
> Relevant code is in try_bulk_dequeue_skb()
> 

I'll look at it.
I was almost starting to implement that but then I thought with another
(good?) reason to use LSO. LSO gives me the guarantee that the packet is
directed to one peer, which might not be the case with xmit_more since
we have one Qdisc for several clients in case we are in AP mode.
Building an A-MSDU for several clients is not possible, at least not for
several client in the L2 (different MAC addresses).
LSO avoids this problem completely.

Re: [RFC v2 0/3] add TSO / A-MSDU TX for iwlwifi

[Eric Dumazet]

On Wed, 2015-08-19 at 15:07 +0000, Grumbach, Emmanuel wrote:

> I'll look at it.
> I was almost starting to implement that but then I thought with another
> (good?) reason to use LSO. LSO gives me the guarantee that the packet is
> directed to one peer, which might not be the case with xmit_more since
> we have one Qdisc for several clients in case we are in AP mode.
> Building an A-MSDU for several clients is not possible, at least not for
> several client in the L2 (different MAC addresses).
> LSO avoids this problem completely.

Then, simply calling skb_gso_segment() from the driver might be enough,
and less work for you.

This would even support TSO on IPv6

segs = skb_gso_segment(skb, tp->dev->features &
                            ~(NETIF_F_TSO | NETIF_F_TSO6));

Re: [RFC v2 0/3] add TSO / A-MSDU TX for iwlwifi

[Grumbach, Emmanuel]

On 08/19/2015 07:08 PM, Eric Dumazet wrote:
> On Wed, 2015-08-19 at 15:07 +0000, Grumbach, Emmanuel wrote:
> 
>> I'll look at it.
>> I was almost starting to implement that but then I thought with another
>> (good?) reason to use LSO. LSO gives me the guarantee that the packet is
>> directed to one peer, which might not be the case with xmit_more since
>> we have one Qdisc for several clients in case we are in AP mode.
>> Building an A-MSDU for several clients is not possible, at least not for
>> several client in the L2 (different MAC addresses).
>> LSO avoids this problem completely.
> 
> Then, simply calling skb_gso_segment() from the driver might be enough,
> and less work for you.
> 
> This would even support TSO on IPv6
> 

Well... I did take care of IPv6.

> segs = skb_gso_segment(skb, tp->dev->features &
>                             ~(NETIF_F_TSO | NETIF_F_TSO6));
> 
> 

Thing is that our HW layers are currently implemented to receive one skb
per 802.11 packet. So that if I call skb_gso_segment, I'd have to
re-assemble the segs into one A-MSDU which would translate one skb.
I guess I could change the HW layer in the driver to be able to get a
list of skbs and make a single packet out of it, but that'd be tricky or
wasteful. skb_gso_segment will duplicate the wifi header while it is not
needed. Only the TCP / IP / SNAP headers need to be duplicated.
Moreover, each subframe in the A-MSDU needs it own subframe header (same
format as ethhdr) and there is also some padding in there. So that would
be even more complicated IMHO.
My code doesn't copy any payload. Only the headers. This is why I
thought it'd be better than segmenting and then re-assembling.
I did call skb_gso_segment if I get lots of payload in the header (more
than 2 * mss) in order to simplify the implementation.

Re: [RFC v2 0/3] add TSO / A-MSDU TX for iwlwifi

[Eric Dumazet]

On Wed, 2015-08-19 at 17:00 +0000, Grumbach, Emmanuel wrote:
> 
> On 08/19/2015 07:08 PM, Eric Dumazet wrote:
> > On Wed, 2015-08-19 at 15:07 +0000, Grumbach, Emmanuel wrote:
> > 
> >> I'll look at it.
> >> I was almost starting to implement that but then I thought with another
> >> (good?) reason to use LSO. LSO gives me the guarantee that the packet is
> >> directed to one peer, which might not be the case with xmit_more since
> >> we have one Qdisc for several clients in case we are in AP mode.
> >> Building an A-MSDU for several clients is not possible, at least not for
> >> several client in the L2 (different MAC addresses).
> >> LSO avoids this problem completely.
> > 
> > Then, simply calling skb_gso_segment() from the driver might be enough,
> > and less work for you.
> > 
> > This would even support TSO on IPv6
> > 
> 
> Well... I did take care of IPv6.

net/core/tso.c does not yet handle IPv6

Re: [RFC v2 0/3] add TSO / A-MSDU TX for iwlwifi

[Grumbach, Emmanuel]

On 08/19/2015 08:20 PM, Eric Dumazet wrote:
> On Wed, 2015-08-19 at 17:00 +0000, Grumbach, Emmanuel wrote:
>>
>> On 08/19/2015 07:08 PM, Eric Dumazet wrote:
>>> On Wed, 2015-08-19 at 15:07 +0000, Grumbach, Emmanuel wrote:
>>>
>>>> I'll look at it.
>>>> I was almost starting to implement that but then I thought with another
>>>> (good?) reason to use LSO. LSO gives me the guarantee that the packet is
>>>> directed to one peer, which might not be the case with xmit_more since
>>>> we have one Qdisc for several clients in case we are in AP mode.
>>>> Building an A-MSDU for several clients is not possible, at least not for
>>>> several client in the L2 (different MAC addresses).
>>>> LSO avoids this problem completely.
>>>
>>> Then, simply calling skb_gso_segment() from the driver might be enough,
>>> and less work for you.
>>>
>>> This would even support TSO on IPv6
>>>
>>
>> Well... I did take care of IPv6.
> 
> net/core/tso.c does not yet handle IPv6
> 

Yeah - I can see that now.
I can teach him - that's not a big deal. The bigger problem is that
net/core/tso.c doesn't do what I really need: it does only a small
portion. Since I need to add one frag to several skbs, I need to
refcount the frags' page. net/core/tso.c hides the page from me.

I can try to use tso_build_hdr but it will copy the entire header where
I need only SNAP / IP / TCP (and not 802.11).
I am getting the feeling that net/core/tso.c is close to what I need,
but not close enough to be usable without making changes that would make
the implementation too complicated and changing net/core/tso.c in a way
that would be much less readable for other users.
I know that our device is quite "unique" in the sense that most other
vendors do all the header twiddling in hardware. We unfortunately don't.
The A-MSDU's format is also somewhat unusual:

<802.11 HDR>
	<ETH><SNAP><IP><TCP><PAYLOAD><PAD (variable length)
	<ETH><SNAP><IP><TCP><PAYLOAD><PAD (variable length)
	<ETH><SNAP><IP><TCP><PAYLOAD><PAD (variable length)
	etc...

So I feel that making net/core/tso.c more complicated just because of
our craziness seems an overkill to me.
I'll try a bit harder to see how I can use net/core/tso.c, but I have to
say I am pessimistic.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 0/3] add TSO / A-MSDU TX for iwlwifi

[Eric Dumazet]

On Wed, 2015-08-19 at 17:56 +0000, Grumbach, Emmanuel wrote:
> 

> So I feel that making net/core/tso.c more complicated just because of
> our craziness seems an overkill to me.
> I'll try a bit harder to see how I can use net/core/tso.c, but I have to
> say I am pessimistic.

net/core/tso.c is WIP, feel free to expand it to make it more generic
and meet your needs.

The point is : we want a core infrastructure, not something that each
individual driver implements in ~500 lines of code :(


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 0/3] add TSO / A-MSDU TX for iwlwifi

[Grumbach, Emmanuel]

On 08/19/2015 09:02 PM, Eric Dumazet wrote:
> On Wed, 2015-08-19 at 17:56 +0000, Grumbach, Emmanuel wrote:
>>
> 
>> So I feel that making net/core/tso.c more complicated just because of
>> our craziness seems an overkill to me.
>> I'll try a bit harder to see how I can use net/core/tso.c, but I have to
>> say I am pessimistic.
> 
> net/core/tso.c is WIP, feel free to expand it to make it more generic
> and meet your needs.

Yeah - trying to see what can be done.

>
> The point is : we want a core infrastructure, not something that each
> individual driver implements in ~500 lines of code :(
> 
> 

I totally understand that :) I just claim to be unique in a way that
"each individual driver" is ... only me :)

I guess that if we would build the DMA descriptors directly from the
skb_gso (the skb coming from the stack), that's be easier. Our HW
abstraction layer wants an skb and I need to pass several skbs (because
skb->len is very likely not to fit in one single 802.11 packet even if
it is an A-MSDU).
So, trying to use net/core/tso.c basically means, to open the arch of
our driver... Not impossible, but quite a bit of work.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html