| Message ID | 1584524612-24470-10-git-send-email-ilpo.jarvinen@helsinki.fi |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | : Accurate ECN for TCP | expand |
On 3/18/20 2:43 AM, Ilpo Järvinen wrote: > From: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi> > > Handling the CWR flag differs between RFC 3168 ECN and AccECN. > Take it into account in GSO by not clearing the CWR bit. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi> > Does it means TCP segmentation offload is disabled on all the NIC around ? Why tun driver is changed and not others ? I believe you need to give much more details in changelog in general, because many changes are not that obvious.
On Wed, 18 Mar 2020, Eric Dumazet wrote: > On 3/18/20 2:43 AM, Ilpo Järvinen wrote: > > From: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi> > > > > Handling the CWR flag differs between RFC 3168 ECN and AccECN. > > Take it into account in GSO by not clearing the CWR bit. > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi> > > > > Does it means TCP segmentation offload is disabled on all the NIC > around ? On general level, yes. HW TSO should be disabled with AccECN (or when CWR flag is set for an incoming packet). The reason is how CWR flag is handled by RFC 3168 ECN-aware TSO. It splits the segment such that CWR is cleared starting from the 2nd segment which is incompatible how AccECN handles the CWR flag. With AccECN, CWR flag (or more accurately, the ACE field that also includes ECE & AE flags) changes only when new packet(s) with CE mark arrives so the flag should not be changed within a super-skb. The new feature flag is necessary to prevent such TSO engines happily clearing CWRs (if the CWR handling feature cannot be turned off). If NIC is completely unaware of RFC3168 ECN (doesn't support NETIF_F_TSO_ECN) or its TSO engine can be set to not touch CWR flag despite supporting also NETIF_F_TSO_ECN, TSO could be safely used with AccECN on such NIC. I've little expertise on TSO HW so I don't know if some/what NICs can do it. Nonetheless, there's nothing fundamental preventing TSO being enabled with AccECN by NICs (if either of those conditions is met). In the cases, where TSO would fail to keep its hands off CWR flag, it should fallback to GSO which has always on AccECN support (similar to always on ECN support). I think that the current GSO changes are capable of handling AccECN skbs. For the other parts of the idea above I'm not so sure. There is this NETIF_F_GSO_SOFTWARE with comment that seems to indicate it's doing what I want but I'm not fully sure if adding a flag there is enough to achieve the desired effect? On the rx side, supporting both RFC3168 and AccECN style CWR handling is slightly more complicated (and possibly not worth the effort given CWRs should be relatively rare with RFC3168-style ECN). > Why tun driver is changed and not others ? I think I didn't really understand why tun.c plays with the TSO_ECN flag until now (after finding a related comment from tap.c) and so that change now doesn't make much sense for me now any more. So I'll just remove that part. > I believe you need to give much more details in changelog in general, > because many changes are not that obvious. I'll try to. Thanks.
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 228fe449dc6d..d376a7cb0d63 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -2788,7 +2788,8 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | TUN_USER_FEATURES | NETIF_F_HW_VLAN_CTAG_TX | - NETIF_F_HW_VLAN_STAG_TX; + NETIF_F_HW_VLAN_STAG_TX | + NETIF_F_GSO_ACCECN; dev->features = dev->hw_features | NETIF_F_LLTX; dev->vlan_features = dev->features & ~(NETIF_F_HW_VLAN_CTAG_TX | diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h index 34d050bb1ae6..c7065b468d21 100644 --- a/include/linux/netdev_features.h +++ b/include/linux/netdev_features.h @@ -83,6 +83,7 @@ enum { NETIF_F_HW_TLS_RECORD_BIT, /* Offload TLS record */ NETIF_F_GRO_FRAGLIST_BIT, /* Fraglist GRO */ + NETIF_F_GSO_ACCECN_BIT, /* ... TCP AccECN support */ /* * Add your fresh new feature above and remember to update * netdev_features_strings[] in net/core/ethtool.c and maybe @@ -124,6 +125,7 @@ enum { #define NETIF_F_SG __NETIF_F(SG) #define NETIF_F_TSO6 __NETIF_F(TSO6) #define NETIF_F_TSO_ECN __NETIF_F(TSO_ECN) +#define NETIF_F_GSO_ACCECN __NETIF_F(GSO_ACCECN) #define NETIF_F_TSO __NETIF_F(TSO) #define NETIF_F_VLAN_CHALLENGED __NETIF_F(VLAN_CHALLENGED) #define NETIF_F_RXFCS __NETIF_F(RXFCS) @@ -205,6 +207,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start) /* List of features with software fallbacks. */ #define NETIF_F_GSO_SOFTWARE (NETIF_F_ALL_TSO | \ + NETIF_F_GSO_ACCECN | \ NETIF_F_GSO_SCTP) /* diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 21749b2cdc9b..fdd73dc126a2 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -594,6 +594,8 @@ enum { SKB_GSO_UDP_L4 = 1 << 17, SKB_GSO_FRAGLIST = 1 << 18, + + SKB_GSO_TCP_ACCECN = 1 << 19, }; #if BITS_PER_LONG > 32 diff --git a/net/ethtool/common.c b/net/ethtool/common.c index 7b6969af5ae7..26241b5d62a4 100644 --- a/net/ethtool/common.c +++ b/net/ethtool/common.c @@ -27,6 +27,7 @@ const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = { [NETIF_F_TSO_BIT] = "tx-tcp-segmentation", [NETIF_F_GSO_ROBUST_BIT] = "tx-gso-robust", [NETIF_F_TSO_ECN_BIT] = "tx-tcp-ecn-segmentation", + [NETIF_F_GSO_ACCECN_BIT] = "tx-tcp-accecn-segmentation", [NETIF_F_TSO_MANGLEID_BIT] = "tx-tcp-mangleid-segmentation", [NETIF_F_TSO6_BIT] = "tx-tcp6-segmentation", [NETIF_F_FSO_BIT] = "tx-fcoe-segmentation", diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index e09147ac9a99..7a81cf438010 100644 --- a/net/ipv4/tcp_offload.c +++ b/net/ipv4/tcp_offload.c @@ -65,6 +65,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, struct sk_buff *gso_skb = skb; __sum16 newcheck; bool ooo_okay, copy_destructor; + bool ecn_cwr_mask; th = tcp_hdr(skb); thlen = th->doff * 4; @@ -121,6 +122,8 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, newcheck = ~csum_fold((__force __wsum)((__force u32)th->check + (__force u32)delta)); + ecn_cwr_mask = !!(skb_shinfo(gso_skb)->gso_type & SKB_GSO_TCP_ACCECN); + while (skb->next) { th->fin = th->psh = 0; th->check = newcheck; @@ -140,7 +143,8 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, th = tcp_hdr(skb); th->seq = htonl(seq); - th->cwr = 0; + + th->cwr &= ecn_cwr_mask; } /* Following permits TCP Small Queues to work well with GSO :