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

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

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. In the future, we could add
> > NETIF_F_TSO_MANGLEID_{INNER,OUTER} to provide more granular control to
> > drivers.
> > 
> > This commit also modifies a few drivers that use SKB_GSO_FIXEDID directly.
> > 
> > Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
> > ---
> >  Documentation/networking/segmentation-offloads.rst |  4 ++--
> >  drivers/net/ethernet/hisilicon/hns3/hns3_enet.c    |  2 +-
> >  drivers/net/ethernet/mellanox/mlx5/core/en_rx.c    |  8 ++++++--
> >  drivers/net/ethernet/sfc/ef100_tx.c                | 14 ++++++++------
> >  include/linux/netdevice.h                          |  9 +++++++--
> >  include/linux/skbuff.h                             |  6 +++++-
> >  net/core/dev.c                                     |  7 +++----
> >  net/ipv4/af_inet.c                                 | 13 ++++++-------
> >  net/ipv4/tcp_offload.c                             |  4 +---
> >  9 files changed, 39 insertions(+), 28 deletions(-)
> > 
> > diff --git a/Documentation/networking/segmentation-offloads.rst b/Documentation/networking/segmentation-offloads.rst
> > index 085e8fab03fd..21c759b81f4e 100644
> > --- a/Documentation/networking/segmentation-offloads.rst
> > +++ b/Documentation/networking/segmentation-offloads.rst
> > @@ -42,8 +42,8 @@ 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
> > +GSO type SKB_GSO_TCP_FIXEDID_{OUTER,INNER} 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.
> > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> > index bfa5568baa92..b28f890b0af5 100644
> > --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> > @@ -3868,7 +3868,7 @@ static int hns3_gro_complete(struct sk_buff *skb, u32 l234info)
> >  		skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;
> >  
> >  	if (l234info & BIT(HNS3_RXD_GRO_FIXID_B))
> > -		skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_FIXEDID;
> > +		skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_FIXEDID_OUTER;
> >  
> >  	skb->csum_start = (unsigned char *)th - skb->head;
> >  	skb->csum_offset = offsetof(struct tcphdr, check);
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > index b8c609d91d11..78df60c62225 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_OUTER;
> > +	}
> >  
> >  	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..aab2425e62bb 100644
> > --- a/drivers/net/ethernet/sfc/ef100_tx.c
> > +++ b/drivers/net/ethernet/sfc/ef100_tx.c
> > @@ -189,7 +189,8 @@ 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;
> > +	u32 mangleid_inner = 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,10 @@ 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 (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID_OUTER)
> > +		mangleid_outer = ESE_GZ_TX_DESC_IP4_ID_NO_OP;
> > +	if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID_INNER)
> > +		mangleid_inner = 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);
> >  
> > @@ -239,14 +242,13 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
> >  			      ESF_GZ_TX_TSO_CSO_INNER_L4, 1,
> >  			      ESF_GZ_TX_TSO_INNER_L3_OFF_W, ip_offset >> 1,
> >  			      ESF_GZ_TX_TSO_INNER_L4_OFF_W, tcp_offset >> 1,
> > -			      ESF_GZ_TX_TSO_ED_INNER_IP4_ID, mangleid,
> > +			      ESF_GZ_TX_TSO_ED_INNER_IP4_ID, mangleid_inner,
> >  			      ESF_GZ_TX_TSO_ED_INNER_IP_LEN, 1,
> >  			      ESF_GZ_TX_TSO_OUTER_L3_OFF_W, outer_ip_offset >> 1,
> >  			      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 5e5de4b0a433..e55ba6918b0a 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -5287,13 +5287,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_OUTER | SKB_GSO_TCP_FIXEDID_INNER))
> > +		gso_type |= __SKB_GSO_TCP_FIXEDID;
> 
> This is quite peculiar.
> 
> Is there a real use case for specifying FIXEDID separately for outer
> and inner? Can the existing single bit govern both together instead?
> That would be a lot simpler.

I guess not, as with GRO this is under control of the sender, and
possibly a separate middlebox in control of encapsulation.

Still, possible to preserve existing FIXEDID for the unencapsulated
or inner, and add only one extra FIXEDID for outer? Or is there value
in having three bits?