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

[Richard Gobert <richardbgobert@gmail.com>]

Willem de Bruijn 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. 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?

The ideal solution would be to split NETIF_F_TSO_MANGLEID into
NETIF_F_TSO_MANGLEID_OUTER and NETIF_F_TSO_MANGLEID_INNER.
 
As I noted, we can add this in the future, then each SKB_GSO_FIXEDID
bit will have a corresponding NETIF_F_TSO_MANGLEID bit. This would
be a much bigger change, as it requires modifying all of the
drivers that use NETIF_F_TSO_MANGLEID. My proposed solution is
somewhat of a middle-ground, as it preserves the current behavior of
NETIF_F_TSO_MANGLEID.
 
Preserving the existing FIXEDID for unencapsulated or inner headers
seems confusing to me, and just moves the ugly code elsewhere. We also
still need to compare both SKB_GSO_FIXEDID bits against NETIF_F_TSO_MANGLEID.