| Message ID | 1445803371-19778-1-git-send-email-emmanuel.grumbach@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 2015/10/26 5:02, Emmanuel Grumbach wrote: > Adding IPv6 for the TSO helper API is trivial: > * Don't play with the id (which doesn't exist in IPv6) > * Correctly update the payload_len (don't include the > length of the IP header itself) ... > memcpy(hdr, skb->data, hdr_len); > - iph = (struct iphdr *)(hdr + mac_hdr_len); > - iph->id = htons(tso->ip_id); > - iph->tot_len = htons(size + hdr_len - mac_hdr_len); > + if (skb->protocol == htons(ETH_P_IP)) { I guess this should be vlan_get_protocol(skb). > + struct iphdr *iph = (void *)(hdr + mac_hdr_len); > + > + iph->id = htons(tso->ip_id); > + iph->tot_len = htons(size + hdr_len - mac_hdr_len); > + tso->ip_id++; > + } else if (skb->protocol == htons(ETH_P_IPV6)) { Likewise. Toshiaki Makita -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/26/2015 06:03 AM, Toshiaki Makita wrote: > On 2015/10/26 5:02, Emmanuel Grumbach wrote: >> Adding IPv6 for the TSO helper API is trivial: >> * Don't play with the id (which doesn't exist in IPv6) >> * Correctly update the payload_len (don't include the >> length of the IP header itself) > ... >> memcpy(hdr, skb->data, hdr_len); >> - iph = (struct iphdr *)(hdr + mac_hdr_len); >> - iph->id = htons(tso->ip_id); >> - iph->tot_len = htons(size + hdr_len - mac_hdr_len); >> + if (skb->protocol == htons(ETH_P_IP)) { > > I guess this should be vlan_get_protocol(skb). I truly don't know. I guess we could have VLANs, but I'd need to check how the packet would look like after it exits mac80211. If we need that, I'll likely do this check once in tso_start() and add a variable to struct tso_t. > >> + struct iphdr *iph = (void *)(hdr + mac_hdr_len); >> + >> + iph->id = htons(tso->ip_id); >> + iph->tot_len = htons(size + hdr_len - mac_hdr_len); >> + tso->ip_id++; >> + } else if (skb->protocol == htons(ETH_P_IPV6)) { > > Likewise. > > Toshiaki Makita > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/10/26 16:47, Grumbach, Emmanuel wrote: > On 10/26/2015 06:03 AM, Toshiaki Makita wrote: >> On 2015/10/26 5:02, Emmanuel Grumbach wrote: >>> Adding IPv6 for the TSO helper API is trivial: >>> * Don't play with the id (which doesn't exist in IPv6) >>> * Correctly update the payload_len (don't include the >>> length of the IP header itself) >> ... >>> memcpy(hdr, skb->data, hdr_len); >>> - iph = (struct iphdr *)(hdr + mac_hdr_len); >>> - iph->id = htons(tso->ip_id); >>> - iph->tot_len = htons(size + hdr_len - mac_hdr_len); >>> + if (skb->protocol == htons(ETH_P_IP)) { >> >> I guess this should be vlan_get_protocol(skb). > > I truly don't know. I guess we could have VLANs, but I'd need to check > how the packet would look like after it exits mac80211. I don't know much about mac80211. What I see is that mvneta has TSO in vlan_features and it uses tso_build_hdr(). When vlan device is used, we cannot access network protocol by skb->protocol without HW vlan acceleration. So it looks like this change corrupts TSO functionality on mvneta. > If we need that, I'll likely do this check once in tso_start() and add a > variable to struct tso_t. I'm not sure if an additional variable is needed. At least, skb_network_offset()/ip_hdr() should correctly handle (skip) vlan headers. Toshiaki Makita -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/10/26 17:13, Toshiaki Makita wrote: > On 2015/10/26 16:47, Grumbach, Emmanuel wrote: >> On 10/26/2015 06:03 AM, Toshiaki Makita wrote: >>> On 2015/10/26 5:02, Emmanuel Grumbach wrote: >>>> Adding IPv6 for the TSO helper API is trivial: >>>> * Don't play with the id (which doesn't exist in IPv6) >>>> * Correctly update the payload_len (don't include the >>>> length of the IP header itself) >>> ... >>>> memcpy(hdr, skb->data, hdr_len); >>>> - iph = (struct iphdr *)(hdr + mac_hdr_len); >>>> - iph->id = htons(tso->ip_id); >>>> - iph->tot_len = htons(size + hdr_len - mac_hdr_len); >>>> + if (skb->protocol == htons(ETH_P_IP)) { >>> >>> I guess this should be vlan_get_protocol(skb). >> >> I truly don't know. I guess we could have VLANs, but I'd need to check >> how the packet would look like after it exits mac80211. > > I don't know much about mac80211. > > What I see is that mvneta has TSO in vlan_features and it uses > tso_build_hdr(). When vlan device is used, we cannot access network > protocol by skb->protocol without HW vlan acceleration. > So it looks like this change corrupts TSO functionality on mvneta. > >> If we need that, I'll likely do this check once in tso_start() and add a >> variable to struct tso_t. > > I'm not sure if an additional variable is needed. > At least, skb_network_offset()/ip_hdr() should correctly handle (skip) > vlan headers. Ah, sorry, I misread your suggestion. Additional variable would make sense to me. Toshiaki Makita -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/26/2015 10:14 AM, Toshiaki Makita wrote: > On 2015/10/26 16:47, Grumbach, Emmanuel wrote: >> On 10/26/2015 06:03 AM, Toshiaki Makita wrote: >>> On 2015/10/26 5:02, Emmanuel Grumbach wrote: >>>> Adding IPv6 for the TSO helper API is trivial: >>>> * Don't play with the id (which doesn't exist in IPv6) >>>> * Correctly update the payload_len (don't include the >>>> length of the IP header itself) >>> ... >>>> memcpy(hdr, skb->data, hdr_len); >>>> - iph = (struct iphdr *)(hdr + mac_hdr_len); >>>> - iph->id = htons(tso->ip_id); >>>> - iph->tot_len = htons(size + hdr_len - mac_hdr_len); >>>> + if (skb->protocol == htons(ETH_P_IP)) { >>> >>> I guess this should be vlan_get_protocol(skb). >> >> I truly don't know. I guess we could have VLANs, but I'd need to check >> how the packet would look like after it exits mac80211. > > I don't know much about mac80211. > > What I see is that mvneta has TSO in vlan_features and it uses > tso_build_hdr(). When vlan device is used, we cannot access network > protocol by skb->protocol without HW vlan acceleration. > So it looks like this change corrupts TSO functionality on mvneta. Convincing enough :) > >> If we need that, I'll likely do this check once in tso_start() and add a >> variable to struct tso_t. > > I'm not sure if an additional variable is needed. > At least, skb_network_offset()/ip_hdr() should correctly handle (skip) > vlan headers. > Adding a bool to tso_t is more an optimisation than something needed for correctness. It avoid to call vlan_get_protocol(skb) for each MSS. > Toshiaki Makita > > -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/net/core/tso.c b/net/core/tso.c index 630b30b..bf71bdc5 100644 --- a/net/core/tso.c +++ b/net/core/tso.c @@ -14,18 +14,24 @@ EXPORT_SYMBOL(tso_count_descs); void tso_build_hdr(struct sk_buff *skb, char *hdr, struct tso_t *tso, int size, bool is_last) { - struct iphdr *iph; struct tcphdr *tcph; int hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb); int mac_hdr_len = skb_network_offset(skb); memcpy(hdr, skb->data, hdr_len); - iph = (struct iphdr *)(hdr + mac_hdr_len); - iph->id = htons(tso->ip_id); - iph->tot_len = htons(size + hdr_len - mac_hdr_len); + if (skb->protocol == htons(ETH_P_IP)) { + struct iphdr *iph = (void *)(hdr + mac_hdr_len); + + iph->id = htons(tso->ip_id); + iph->tot_len = htons(size + hdr_len - mac_hdr_len); + tso->ip_id++; + } else if (skb->protocol == htons(ETH_P_IPV6)) { + struct ipv6hdr *iph = (void *)(hdr + mac_hdr_len); + + iph->payload_len = htons(size + tcp_hdrlen(skb)); + } tcph = (struct tcphdr *)(hdr + skb_transport_offset(skb)); put_unaligned_be32(tso->tcp_seq, &tcph->seq); - tso->ip_id++; if (!is_last) { /* Clear all special flags for not last packet */
Adding IPv6 for the TSO helper API is trivial: * Don't play with the id (which doesn't exist in IPv6) * Correctly update the payload_len (don't include the length of the IP header itself) Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> --- v2: add else if NOTE: instead of checking the skb->protocol, I can add a bool in tso_t. This might be better in terms of cache handling. Let me know if you prefer that option --- net/core/tso.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)