Currently, NETIF_F_TSO_MANGLEID indicates that the inner-most ID can
be mangled. Outer IDs can always be mangled.
Make GSO preserve outer IDs by default, with NETIF_F_TSO_MANGLEID allowing
both inner and outer IDs to be mangled.
This commit also modifies a few drivers that use SKB_GSO_FIXEDID directly.
Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
---
.../networking/segmentation-offloads.rst | 9 ++++-----
drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 8 ++++++--
drivers/net/ethernet/sfc/ef100_tx.c | 17 +++++++++++++----
include/linux/netdevice.h | 9 +++++++--
include/linux/skbuff.h | 6 +++++-
net/core/dev.c | 4 +++-
net/ipv4/af_inet.c | 13 ++++++-------
net/ipv4/tcp_offload.c | 5 +----
8 files changed, 45 insertions(+), 26 deletions(-)
diff --git a/Documentation/networking/segmentation-offloads.rst b/Documentation/networking/segmentation-offloads.rst
index 085e8fab03fd..d5dccfc6b82b 100644
--- a/Documentation/networking/segmentation-offloads.rst
+++ b/Documentation/networking/segmentation-offloads.rst
@@ -46,7 +46,9 @@ GSO type SKB_GSO_TCP_FIXEDID is specified then we will not increment the IP
ID and all segments will use the same IP ID. If a device has
NETIF_F_TSO_MANGLEID set then the IP ID can be ignored when performing TSO
and we will either increment the IP ID for all frames, or leave it at a
-static value based on driver preference.
+static value based on driver preference. For outer headers of encapsulated
+packets, the device drivers must guarantee that the IPv4 ID field is
+incremented in the case that a given header does not have the DF bit set.
UDP Fragmentation Offload
@@ -124,10 +126,7 @@ Generic Receive Offload
Generic receive offload is the complement to GSO. Ideally any frame
assembled by GRO should be segmented to create an identical sequence of
frames using GSO, and any sequence of frames segmented by GSO should be
-able to be reassembled back to the original by GRO. The only exception to
-this is IPv4 ID in the case that the DF bit is set for a given IP header.
-If the value of the IPv4 ID is not sequentially incrementing it will be
-altered so that it is when a frame assembled via GRO is segmented via GSO.
+able to be reassembled back to the original by GRO.
Partial Generic Segmentation Offload
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index b8c609d91d11..505c4ce7cef8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -1289,8 +1289,12 @@ static void mlx5e_shampo_update_ipv4_tcp_hdr(struct mlx5e_rq *rq, struct iphdr *
tcp->check = ~tcp_v4_check(skb->len - tcp_off, ipv4->saddr,
ipv4->daddr, 0);
skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV4;
- if (ntohs(ipv4->id) == rq->hw_gro_data->second_ip_id)
- skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_FIXEDID;
+ if (ntohs(ipv4->id) == rq->hw_gro_data->second_ip_id) {
+ bool encap = rq->hw_gro_data->fk.control.flags & FLOW_DIS_ENCAPSULATION;
+
+ skb_shinfo(skb)->gso_type |= encap ?
+ SKB_GSO_TCP_FIXEDID_INNER : SKB_GSO_TCP_FIXEDID;
+ }
skb->csum_start = (unsigned char *)tcp - skb->head;
skb->csum_offset = offsetof(struct tcphdr, check);
diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
index e6b6be549581..24971346df00 100644
--- a/drivers/net/ethernet/sfc/ef100_tx.c
+++ b/drivers/net/ethernet/sfc/ef100_tx.c
@@ -190,6 +190,7 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
bool gso_partial = skb_shinfo(skb)->gso_type & SKB_GSO_PARTIAL;
unsigned int len, ip_offset, tcp_offset, payload_segs;
u32 mangleid = ESE_GZ_TX_DESC_IP4_ID_INC_MOD16;
+ u32 mangleid_outer = ESE_GZ_TX_DESC_IP4_ID_INC_MOD16;
unsigned int outer_ip_offset, outer_l4_offset;
u16 vlan_tci = skb_vlan_tag_get(skb);
u32 mss = skb_shinfo(skb)->gso_size;
@@ -200,8 +201,17 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
bool outer_csum;
u32 paylen;
- if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID)
- mangleid = ESE_GZ_TX_DESC_IP4_ID_NO_OP;
+ if (encap) {
+ if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID_INNER)
+ mangleid = ESE_GZ_TX_DESC_IP4_ID_NO_OP;
+ if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID)
+ mangleid_outer = ESE_GZ_TX_DESC_IP4_ID_NO_OP;
+ } else {
+ if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID)
+ mangleid = ESE_GZ_TX_DESC_IP4_ID_NO_OP;
+ mangleid_outer = ESE_GZ_TX_DESC_IP4_ID_NO_OP;
+ }
+
if (efx->net_dev->features & NETIF_F_HW_VLAN_CTAG_TX)
vlan_enable = skb_vlan_tag_present(skb);
@@ -245,8 +255,7 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
ESF_GZ_TX_TSO_OUTER_L4_OFF_W, outer_l4_offset >> 1,
ESF_GZ_TX_TSO_ED_OUTER_UDP_LEN, udp_encap && !gso_partial,
ESF_GZ_TX_TSO_ED_OUTER_IP_LEN, encap && !gso_partial,
- ESF_GZ_TX_TSO_ED_OUTER_IP4_ID, encap ? mangleid :
- ESE_GZ_TX_DESC_IP4_ID_NO_OP,
+ ESF_GZ_TX_TSO_ED_OUTER_IP4_ID, mangleid_outer,
ESF_GZ_TX_TSO_VLAN_INSERT_EN, vlan_enable,
ESF_GZ_TX_TSO_VLAN_INSERT_TCI, vlan_tci
);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f3a3b761abfb..3d19c888b839 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -5290,13 +5290,18 @@ void skb_warn_bad_offload(const struct sk_buff *skb);
static inline bool net_gso_ok(netdev_features_t features, int gso_type)
{
- netdev_features_t feature = (netdev_features_t)gso_type << NETIF_F_GSO_SHIFT;
+ netdev_features_t feature;
+
+ if (gso_type & (SKB_GSO_TCP_FIXEDID | SKB_GSO_TCP_FIXEDID_INNER))
+ gso_type |= __SKB_GSO_TCP_FIXEDID;
+
+ feature = ((netdev_features_t)gso_type << NETIF_F_GSO_SHIFT) & NETIF_F_GSO_MASK;
/* check flags correspondence */
BUILD_BUG_ON(SKB_GSO_TCPV4 != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
BUILD_BUG_ON(SKB_GSO_DODGY != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
- BUILD_BUG_ON(SKB_GSO_TCP_FIXEDID != (NETIF_F_TSO_MANGLEID >> NETIF_F_GSO_SHIFT));
+ BUILD_BUG_ON(__SKB_GSO_TCP_FIXEDID != (NETIF_F_TSO_MANGLEID >> NETIF_F_GSO_SHIFT));
BUILD_BUG_ON(SKB_GSO_TCPV6 != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
BUILD_BUG_ON(SKB_GSO_FCOE != (NETIF_F_FSO >> NETIF_F_GSO_SHIFT));
BUILD_BUG_ON(SKB_GSO_GRE != (NETIF_F_GSO_GRE >> NETIF_F_GSO_SHIFT));
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ca8be45dd8be..cf95b325f9b4 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -674,7 +674,7 @@ enum {
/* This indicates the tcp segment has CWR set. */
SKB_GSO_TCP_ECN = 1 << 2,
- SKB_GSO_TCP_FIXEDID = 1 << 3,
+ __SKB_GSO_TCP_FIXEDID = 1 << 3,
SKB_GSO_TCPV6 = 1 << 4,
@@ -707,6 +707,10 @@ enum {
SKB_GSO_FRAGLIST = 1 << 18,
SKB_GSO_TCP_ACCECN = 1 << 19,
+
+ /* These don't correspond with netdev features. */
+ SKB_GSO_TCP_FIXEDID = 1 << 30,
+ SKB_GSO_TCP_FIXEDID_INNER = 1 << 31,
};
#if BITS_PER_LONG > 32
diff --git a/net/core/dev.c b/net/core/dev.c
index 93a25d87b86b..f57c8dbf307f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3769,7 +3769,9 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
features &= ~dev->gso_partial_features;
/* Make sure to clear the IPv4 ID mangling feature if the
- * IPv4 header has the potential to be fragmented.
+ * IPv4 header has the potential to be fragmented. For
+ * encapsulated packets, the outer headers are guaranteed to
+ * have incrementing IDs if DF is not set.
*/
if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4) {
struct iphdr *iph = skb->encapsulation ?
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 76e38092cd8a..fc7a6955fa0a 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1393,14 +1393,13 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
segs = ERR_PTR(-EPROTONOSUPPORT);
- if (!skb->encapsulation || encap) {
- udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
- fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
+ /* fixed ID is invalid if DF bit is not set */
+ fixedid = !!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCP_FIXEDID << encap));
+ if (fixedid && !(ip_hdr(skb)->frag_off & htons(IP_DF)))
+ goto out;
- /* fixed ID is invalid if DF bit is not set */
- if (fixedid && !(ip_hdr(skb)->frag_off & htons(IP_DF)))
- goto out;
- }
+ if (!skb->encapsulation || encap)
+ udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
ops = rcu_dereference(inet_offloads[proto]);
if (likely(ops && ops->callbacks.gso_segment)) {
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 1949eede9ec9..e6612bd84d09 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -471,7 +471,6 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
const u16 offset = NAPI_GRO_CB(skb)->network_offsets[skb->encapsulation];
const struct iphdr *iph = (struct iphdr *)(skb->data + offset);
struct tcphdr *th = tcp_hdr(skb);
- bool is_fixedid;
if (unlikely(NAPI_GRO_CB(skb)->is_flist)) {
skb_shinfo(skb)->gso_type |= SKB_GSO_FRAGLIST | SKB_GSO_TCPV4;
@@ -485,10 +484,8 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr,
iph->daddr, 0);
- is_fixedid = (NAPI_GRO_CB(skb)->ip_fixedid >> skb->encapsulation) & 1;
-
skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV4 |
- (is_fixedid * SKB_GSO_TCP_FIXEDID);
+ (NAPI_GRO_CB(skb)->ip_fixedid * SKB_GSO_TCP_FIXEDID);
tcp_gro_complete(skb);
return 0;
--
2.36.1Richard Gobert wrote:
> Currently, NETIF_F_TSO_MANGLEID indicates that the inner-most ID can
> be mangled. Outer IDs can always be mangled.
>
> Make GSO preserve outer IDs by default, with NETIF_F_TSO_MANGLEID allowing
> both inner and outer IDs to be mangled.
>
> This commit also modifies a few drivers that use SKB_GSO_FIXEDID directly.
>
> Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
> ---
> .../networking/segmentation-offloads.rst | 9 ++++-----
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 8 ++++++--
> drivers/net/ethernet/sfc/ef100_tx.c | 17 +++++++++++++----
> include/linux/netdevice.h | 9 +++++++--
> include/linux/skbuff.h | 6 +++++-
> net/core/dev.c | 4 +++-
> net/ipv4/af_inet.c | 13 ++++++-------
> net/ipv4/tcp_offload.c | 5 +----
> 8 files changed, 45 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/networking/segmentation-offloads.rst b/Documentation/networking/segmentation-offloads.rst
> index 085e8fab03fd..d5dccfc6b82b 100644
> --- a/Documentation/networking/segmentation-offloads.rst
> +++ b/Documentation/networking/segmentation-offloads.rst
> @@ -46,7 +46,9 @@ GSO type SKB_GSO_TCP_FIXEDID is specified then we will not increment the IP
> ID and all segments will use the same IP ID. If a device has
> NETIF_F_TSO_MANGLEID set then the IP ID can be ignored when performing TSO
> and we will either increment the IP ID for all frames, or leave it at a
> -static value based on driver preference.
> +static value based on driver preference. For outer headers of encapsulated
> +packets, the device drivers must guarantee that the IPv4 ID field is
> +incremented in the case that a given header does not have the DF bit set.
Please split this into three paragraphs on FIXEDID, FIXED_INNER and
MANGLEID.
Specifically the use of FIXEDID to mean uncapped or outer should be
explicitly mentioned (as discussed previously).
Also, I understood that MANGLEID now means that both inner and outer
IP ID can be mangled. But this comment appears to say otherwise.
Maybe it helps to be more explicit also about behavior without DF.
> Partial Generic Segmentation Offload
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index b8c609d91d11..505c4ce7cef8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
> index e6b6be549581..24971346df00 100644
> --- a/drivers/net/ethernet/sfc/ef100_tx.c
> +++ b/drivers/net/ethernet/sfc/ef100_tx.c
Not sure whether these driver changes need to be separate patches.
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index f3a3b761abfb..3d19c888b839 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -5290,13 +5290,18 @@ void skb_warn_bad_offload(const struct sk_buff *skb);
>
> static inline bool net_gso_ok(netdev_features_t features, int gso_type)
> {
> - netdev_features_t feature = (netdev_features_t)gso_type << NETIF_F_GSO_SHIFT;
> + netdev_features_t feature;
> +
> + if (gso_type & (SKB_GSO_TCP_FIXEDID | SKB_GSO_TCP_FIXEDID_INNER))
> + gso_type |= __SKB_GSO_TCP_FIXEDID;
> +
> + feature = ((netdev_features_t)gso_type << NETIF_F_GSO_SHIFT) & NETIF_F_GSO_MASK;
>
> /* check flags correspondence */
> BUILD_BUG_ON(SKB_GSO_TCPV4 != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
> BUILD_BUG_ON(SKB_GSO_DODGY != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
> BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
> - BUILD_BUG_ON(SKB_GSO_TCP_FIXEDID != (NETIF_F_TSO_MANGLEID >> NETIF_F_GSO_SHIFT));
> + BUILD_BUG_ON(__SKB_GSO_TCP_FIXEDID != (NETIF_F_TSO_MANGLEID >> NETIF_F_GSO_SHIFT));
> BUILD_BUG_ON(SKB_GSO_TCPV6 != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
> BUILD_BUG_ON(SKB_GSO_FCOE != (NETIF_F_FSO >> NETIF_F_GSO_SHIFT));
> BUILD_BUG_ON(SKB_GSO_GRE != (NETIF_F_GSO_GRE >> NETIF_F_GSO_SHIFT));
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index ca8be45dd8be..cf95b325f9b4 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -674,7 +674,7 @@ enum {
> /* This indicates the tcp segment has CWR set. */
> SKB_GSO_TCP_ECN = 1 << 2,
>
> - SKB_GSO_TCP_FIXEDID = 1 << 3,
> + __SKB_GSO_TCP_FIXEDID = 1 << 3,
>
> SKB_GSO_TCPV6 = 1 << 4,
>
> @@ -707,6 +707,10 @@ enum {
> SKB_GSO_FRAGLIST = 1 << 18,
>
> SKB_GSO_TCP_ACCECN = 1 << 19,
> +
> + /* These don't correspond with netdev features. */
Can use clarification. Something like
/* These indirectly together map onto the same netdev feature:
* If NETIF_F_TSO_MANGLE is set it may mangle both inner and outer.
*/
> + SKB_GSO_TCP_FIXEDID = 1 << 30,
> + SKB_GSO_TCP_FIXEDID_INNER = 1 << 31,
> };
>
> #if BITS_PER_LONG > 32
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 93a25d87b86b..f57c8dbf307f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3769,7 +3769,9 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
> features &= ~dev->gso_partial_features;
>
> /* Make sure to clear the IPv4 ID mangling feature if the
> - * IPv4 header has the potential to be fragmented.
> + * IPv4 header has the potential to be fragmented. For
> + * encapsulated packets, the outer headers are guaranteed to
> + * have incrementing IDs if DF is not set.
This is saying that if !DF then both inner and outer must be
incrementing?
Maybe the outer headers are [also] garuanteed to have incrementing IDs.
> */
> if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4) {
> struct iphdr *iph = skb->encapsulation ?
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 76e38092cd8a..fc7a6955fa0a 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1393,14 +1393,13 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
>
> segs = ERR_PTR(-EPROTONOSUPPORT);
>
> - if (!skb->encapsulation || encap) {
> - udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
> - fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
> + /* fixed ID is invalid if DF bit is not set */
> + fixedid = !!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCP_FIXEDID << encap));
> + if (fixedid && !(ip_hdr(skb)->frag_off & htons(IP_DF)))
> + goto out;
>
> - /* fixed ID is invalid if DF bit is not set */
> - if (fixedid && !(ip_hdr(skb)->frag_off & htons(IP_DF)))
> - goto out;
> - }
> + if (!skb->encapsulation || encap)
> + udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
>
> ops = rcu_dereference(inet_offloads[proto]);
> if (likely(ops && ops->callbacks.gso_segment)) {
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index 1949eede9ec9..e6612bd84d09 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -471,7 +471,6 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
> const u16 offset = NAPI_GRO_CB(skb)->network_offsets[skb->encapsulation];
> const struct iphdr *iph = (struct iphdr *)(skb->data + offset);
> struct tcphdr *th = tcp_hdr(skb);
> - bool is_fixedid;
>
> if (unlikely(NAPI_GRO_CB(skb)->is_flist)) {
> skb_shinfo(skb)->gso_type |= SKB_GSO_FRAGLIST | SKB_GSO_TCPV4;
> @@ -485,10 +484,8 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
> th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr,
> iph->daddr, 0);
>
> - is_fixedid = (NAPI_GRO_CB(skb)->ip_fixedid >> skb->encapsulation) & 1;
> -
> skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV4 |
> - (is_fixedid * SKB_GSO_TCP_FIXEDID);
> + (NAPI_GRO_CB(skb)->ip_fixedid * SKB_GSO_TCP_FIXEDID);
This was only just introduced. And is still needed?
Willem de Bruijn wrote:
> Richard Gobert wrote:
>> Currently, NETIF_F_TSO_MANGLEID indicates that the inner-most ID can
>> be mangled. Outer IDs can always be mangled.
>>
>> Make GSO preserve outer IDs by default, with NETIF_F_TSO_MANGLEID allowing
>> both inner and outer IDs to be mangled.
>>
>> This commit also modifies a few drivers that use SKB_GSO_FIXEDID directly.
>>
>> Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
>> ---
>> .../networking/segmentation-offloads.rst | 9 ++++-----
>> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 8 ++++++--
>> drivers/net/ethernet/sfc/ef100_tx.c | 17 +++++++++++++----
>> include/linux/netdevice.h | 9 +++++++--
>> include/linux/skbuff.h | 6 +++++-
>> net/core/dev.c | 4 +++-
>> net/ipv4/af_inet.c | 13 ++++++-------
>> net/ipv4/tcp_offload.c | 5 +----
>> 8 files changed, 45 insertions(+), 26 deletions(-)
>>
>> diff --git a/Documentation/networking/segmentation-offloads.rst b/Documentation/networking/segmentation-offloads.rst
>> index 085e8fab03fd..d5dccfc6b82b 100644
>> --- a/Documentation/networking/segmentation-offloads.rst
>> +++ b/Documentation/networking/segmentation-offloads.rst
>> @@ -46,7 +46,9 @@ GSO type SKB_GSO_TCP_FIXEDID is specified then we will not increment the IP
>> ID and all segments will use the same IP ID. If a device has
>> NETIF_F_TSO_MANGLEID set then the IP ID can be ignored when performing TSO
>> and we will either increment the IP ID for all frames, or leave it at a
>> -static value based on driver preference.
>> +static value based on driver preference. For outer headers of encapsulated
>> +packets, the device drivers must guarantee that the IPv4 ID field is
>> +incremented in the case that a given header does not have the DF bit set.
>
> Please split this into three paragraphs on FIXEDID, FIXED_INNER and
> MANGLEID.
>
> Specifically the use of FIXEDID to mean uncapped or outer should be
> explicitly mentioned (as discussed previously).
>
> Also, I understood that MANGLEID now means that both inner and outer
> IP ID can be mangled. But this comment appears to say otherwise.
> Maybe it helps to be more explicit also about behavior without DF.
>
Sure, I'll elaborate more on these features.
>> Partial Generic Segmentation Offload
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>> index b8c609d91d11..505c4ce7cef8 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>
>> diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
>> index e6b6be549581..24971346df00 100644
>> --- a/drivers/net/ethernet/sfc/ef100_tx.c
>> +++ b/drivers/net/ethernet/sfc/ef100_tx.c
>
> Not sure whether these driver changes need to be separate patches.
>
I updated the drivers in the same patch to keep the kernel in a
stable state after this patch.
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index f3a3b761abfb..3d19c888b839 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -5290,13 +5290,18 @@ void skb_warn_bad_offload(const struct sk_buff *skb);
>>
>> static inline bool net_gso_ok(netdev_features_t features, int gso_type)
>> {
>> - netdev_features_t feature = (netdev_features_t)gso_type << NETIF_F_GSO_SHIFT;
>> + netdev_features_t feature;
>> +
>> + if (gso_type & (SKB_GSO_TCP_FIXEDID | SKB_GSO_TCP_FIXEDID_INNER))
>> + gso_type |= __SKB_GSO_TCP_FIXEDID;
>> +
>> + feature = ((netdev_features_t)gso_type << NETIF_F_GSO_SHIFT) & NETIF_F_GSO_MASK;
>>
>> /* check flags correspondence */
>> BUILD_BUG_ON(SKB_GSO_TCPV4 != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
>> BUILD_BUG_ON(SKB_GSO_DODGY != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
>> BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
>> - BUILD_BUG_ON(SKB_GSO_TCP_FIXEDID != (NETIF_F_TSO_MANGLEID >> NETIF_F_GSO_SHIFT));
>> + BUILD_BUG_ON(__SKB_GSO_TCP_FIXEDID != (NETIF_F_TSO_MANGLEID >> NETIF_F_GSO_SHIFT));
>> BUILD_BUG_ON(SKB_GSO_TCPV6 != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
>> BUILD_BUG_ON(SKB_GSO_FCOE != (NETIF_F_FSO >> NETIF_F_GSO_SHIFT));
>> BUILD_BUG_ON(SKB_GSO_GRE != (NETIF_F_GSO_GRE >> NETIF_F_GSO_SHIFT));
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index ca8be45dd8be..cf95b325f9b4 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -674,7 +674,7 @@ enum {
>> /* This indicates the tcp segment has CWR set. */
>> SKB_GSO_TCP_ECN = 1 << 2,
>>
>> - SKB_GSO_TCP_FIXEDID = 1 << 3,
>> + __SKB_GSO_TCP_FIXEDID = 1 << 3,
>>
>> SKB_GSO_TCPV6 = 1 << 4,
>>
>> @@ -707,6 +707,10 @@ enum {
>> SKB_GSO_FRAGLIST = 1 << 18,
>>
>> SKB_GSO_TCP_ACCECN = 1 << 19,
>> +
>> + /* These don't correspond with netdev features. */
>
> Can use clarification. Something like
>
> /* These indirectly together map onto the same netdev feature:
> * If NETIF_F_TSO_MANGLE is set it may mangle both inner and outer.
> */
NP. I'll use something like the comment you suggested.
>> + SKB_GSO_TCP_FIXEDID = 1 << 30,
>> + SKB_GSO_TCP_FIXEDID_INNER = 1 << 31,
>> };
>>
>> #if BITS_PER_LONG > 32
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 93a25d87b86b..f57c8dbf307f 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3769,7 +3769,9 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
>> features &= ~dev->gso_partial_features;
>>
>> /* Make sure to clear the IPv4 ID mangling feature if the
>> - * IPv4 header has the potential to be fragmented.
>> + * IPv4 header has the potential to be fragmented. For
>> + * encapsulated packets, the outer headers are guaranteed to
>> + * have incrementing IDs if DF is not set.
>
> This is saying that if !DF then both inner and outer must be
> incrementing?
>
> Maybe the outer headers are [also] garuanteed to have incrementing IDs.
>
You mean the inner headers? What I'm saying is that there is no need to clear
the MANGLEID feature if the outer header doesn't have the DF-bit set, since the
driver is guaranteed to generate incrementing IDs for the outer header in that case.
I also stated this in the documentation. See my previous discussion with Paolo.
I'll change this comment so that it is a bit clearer.
Discussion with Paolo: https://lore.kernel.org/netdev/a88ee88c-707f-4266-b514-d0390166dedb@gmail.com/
>> */
>> if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4) {
>> struct iphdr *iph = skb->encapsulation ?
>> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
>> index 76e38092cd8a..fc7a6955fa0a 100644
>> --- a/net/ipv4/af_inet.c
>> +++ b/net/ipv4/af_inet.c
>> @@ -1393,14 +1393,13 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
>>
>> segs = ERR_PTR(-EPROTONOSUPPORT);
>>
>> - if (!skb->encapsulation || encap) {
>> - udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
>> - fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
>> + /* fixed ID is invalid if DF bit is not set */
>> + fixedid = !!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCP_FIXEDID << encap));
>> + if (fixedid && !(ip_hdr(skb)->frag_off & htons(IP_DF)))
>> + goto out;
>>
>> - /* fixed ID is invalid if DF bit is not set */
>> - if (fixedid && !(ip_hdr(skb)->frag_off & htons(IP_DF)))
>> - goto out;
>> - }
>> + if (!skb->encapsulation || encap)
>> + udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
>>
>> ops = rcu_dereference(inet_offloads[proto]);
>> if (likely(ops && ops->callbacks.gso_segment)) {
>> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
>> index 1949eede9ec9..e6612bd84d09 100644
>> --- a/net/ipv4/tcp_offload.c
>> +++ b/net/ipv4/tcp_offload.c
>> @@ -471,7 +471,6 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
>> const u16 offset = NAPI_GRO_CB(skb)->network_offsets[skb->encapsulation];
>> const struct iphdr *iph = (struct iphdr *)(skb->data + offset);
>> struct tcphdr *th = tcp_hdr(skb);
>> - bool is_fixedid;
>>
>> if (unlikely(NAPI_GRO_CB(skb)->is_flist)) {
>> skb_shinfo(skb)->gso_type |= SKB_GSO_FRAGLIST | SKB_GSO_TCPV4;
>> @@ -485,10 +484,8 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
>> th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr,
>> iph->daddr, 0);
>>
>> - is_fixedid = (NAPI_GRO_CB(skb)->ip_fixedid >> skb->encapsulation) & 1;
>> -
>> skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV4 |
>> - (is_fixedid * SKB_GSO_TCP_FIXEDID);
>> + (NAPI_GRO_CB(skb)->ip_fixedid * SKB_GSO_TCP_FIXEDID);
>
> This was only just introduced. And is still needed?
>
This was only introduced so that the previous patch doesn't affect GSO, to make each
patch more independent. Now that GSO is fixed, it is not needed.
On 01/09/2025 12:38, Richard Gobert wrote: > Currently, NETIF_F_TSO_MANGLEID indicates that the inner-most ID can > be mangled. Outer IDs can always be mangled. > > Make GSO preserve outer IDs by default, with NETIF_F_TSO_MANGLEID allowing > both inner and outer IDs to be mangled. > > This commit also modifies a few drivers that use SKB_GSO_FIXEDID directly. > > Signed-off-by: Richard Gobert <richardbgobert@gmail.com> > --- > diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c > index e6b6be549581..24971346df00 100644 > --- a/drivers/net/ethernet/sfc/ef100_tx.c > +++ b/drivers/net/ethernet/sfc/ef100_tx.c > @@ -190,6 +190,7 @@ static void ef100_make_tso_desc(struct efx_nic *efx, > bool gso_partial = skb_shinfo(skb)->gso_type & SKB_GSO_PARTIAL; > unsigned int len, ip_offset, tcp_offset, payload_segs; > u32 mangleid = ESE_GZ_TX_DESC_IP4_ID_INC_MOD16; > + u32 mangleid_outer = ESE_GZ_TX_DESC_IP4_ID_INC_MOD16; > unsigned int outer_ip_offset, outer_l4_offset; > u16 vlan_tci = skb_vlan_tag_get(skb); > u32 mss = skb_shinfo(skb)->gso_size; Reverse xmas tree. With that fixed you can add my Reviewed-by: Edward Cree <ecree.xilinx@gmail.com> # for sfc
© 2016 - 2025 Red Hat, Inc.