Re: [PATCH net-next v5 3/5] net: gso: restore ids of outer ip headers correctly

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Richard Gobert wrote:
> 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>
> >> Reviewed-by: Edward Cree <ecree.xilinx@gmail.com> # for sfc
> >> ---
> >>  .../networking/segmentation-offloads.rst      | 22 ++++++++++++-------
> >>  .../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                        |  9 +++++++-
> >>  net/core/dev.c                                |  5 ++++-
> >>  net/ipv4/af_inet.c                            | 13 +++++------
> >>  net/ipv4/tcp_offload.c                        |  5 +----
> >>  8 files changed, 59 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/Documentation/networking/segmentation-offloads.rst b/Documentation/networking/segmentation-offloads.rst
> >> index 085e8fab03fd..72f69b22b28c 100644
> >> --- a/Documentation/networking/segmentation-offloads.rst
> >> +++ b/Documentation/networking/segmentation-offloads.rst
> >> @@ -43,10 +43,19 @@ also point to the TCP header of the packet.
> >>  For IPv4 segmentation we support one of two types in terms of the IP ID.
> >>  The default behavior is to increment the IP ID with every segment.  If the
> >>  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.
> >> +ID and all segments will use the same IP ID.
> >> +
> >> +For encapsulated packets, SKB_GSO_TCP_FIXEDID refers only to the outer header.
> >> +SKB_GSO_TCP_FIXEDID_INNER can be used to specify the same for the inner header.
> >> +Any combination of these two GSO types is allowed.
> >> +
> >> +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.  For encapsulated packets,
> >> +NETIF_F_TSO_MANGLEID is relevant for both outer and inner headers, unless the
> >> +DF bit is not set on the outer header, in which case the device driver must
> >> +guarantee that the IP ID field is incremented in the outer header with every
> >> +segment.
> > 
> > Is this introducing a new device requirement for advertising
> > NETIF_F_TSO_MANGLEID that existing devices may not meet?
> >   
> 
> No. As I discussed previously with Paolo, existing devices already increment outer
> IDs. It is even a requirement for GSO partial, as already stated in the documentation.

Where is this documented?

> This preserves the current behavior. It just makes it explicit. Actually, we are
> now even explicitly allowing the mangling of outer IDs if the DF-bit is set.
> 

> >>  #if BITS_PER_LONG > 32
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 93a25d87b86b..17cb399cdc2a 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -3769,7 +3769,10 @@ 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 so there is no need
> >> +	 * to clear the IPv4 ID mangling feature.
> > 
> > Why is this true. Or, why is it not also true for non-encapsulated or
> > inner headers?
> > 
> > The same preconditions (incl on DF) are now tested in inet_gro_flush
> > 
> 
> This comment is about the IDs that TSO generates, not the IDs that GRO accepts.
> I'll rewrite the comment to make it clearer.

Yes, this exact statement would convert the assertion into an
explanation. Really helps a casual reader.

> 
> This statement is true because of the requirement specified above, that device
> drivers must increment the outer IDs if the DF-bit is not set. This means that
> MANGLEID cannot turn incrementing IDs into fixed IDs in the outer header if the
> DF-bit is not set (unless the IDs were already fixed, after the next patch) so
> there is no need to clear NETIF_F_TSO_MANGLEID.
> 
> This isn't true for non-encapsulated or inner headers because nothing guarantees
> that MANGLEID won't mangle incrementing IDs into fixed IDs.
> 

> >> @@ -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);
> > 
> > ip_fixedid can now be 0, 1 (outer), 2 (inner) or 3 (inner and outer).
> > 
> > Not all generate a valid SKB_GSO type.
> 
> Since SKB_GSO_TCP_FIXEDID_INNER == SKB_GSO_TCP_FIXEDID << 1, all the values
> listed above generate the correct set of SKB_GSO types. This fact is also
> used in inet_gso_segment (SKB_GSO_TCP_FIXEDID << encap).

Oh right. Neat.

That dependency can use a BUILD_BUG_ON.