Re: [PATCH net-next v2 3/4] net: core: resegment oversized TCP GSO skbs

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

Wang Zhan wrote:
> A GSO skb which exceeds an egress device limit loses its GSO feature mask
> and is segmented into individual packets. This is unnecessarily expensive
> when the device can still offload smaller TCP GSO skbs, which is easy to
> hit once one hop of a BIG TCP path raises gso_max_size and the next one
> does not.
> 
> For an unencapsulated TCP GSO skb which exceeds gso_max_size or
> gso_max_segs, work out how many MSS segments each output skb may carry and
> resegment the skb with that bound instead. Keep the features computed
> without the GSO limit checks, which say whether the device offloads the
> GSO type at all. Encapsulated and frag-list skbs, GSO types the device
> cannot offload, and bounds below two segments keep the existing full
> segmentation path. The result obeys the GSO feature and limit contract the
> device already advertises, so apply it automatically, without extra device
> state or a userspace control.
> 
> The check runs on the skb which is handed to the driver, after
> validate_xmit_vlan() and sk_validate_xmit_skb(), and costs one extra
> ndo_features_check() on the oversized path, against segmenting the skb
> into individual packets. That position is also why the limit follows the
> L3 protocol rather than skb->protocol: validate_xmit_vlan() replaces the
> latter with the VLAN ethertype when it pushes the tag inside the skb.
> 
> Measured on a veth -> bridge -> TAP -> guest virtio-net path, with BIG TCP
> enabled on the veth endpoints and left off in the guest, so the skbs which
> the veth hop accepts have to be segmented before the TAP device. A single
> iperf3 TCP flow, six alternating runs per state (`-t 15 -O 5`, fixed CPU
> affinity and port tuple). The middle column is the same tree with the
> resegmentation disabled:
> 
>   protocol  no BIG TCP   mixed, no reseg  mixed, resegmented
>   TCP/IPv4  51.550 Gbps  15.850 Gbps      52.617 Gbps
>   TCP/IPv6  52.050 Gbps  15.783 Gbps      51.933 Gbps
> 
> Coefficient of variation for the two mixed columns was 0.48% and 0.82%
> for IPv4 and 0.44% and 0.44% for IPv6. A BIG TCP hop which feeds a 64 KiB
> hop loses 69% of the throughput of a path which never enables BIG TCP at
> all; bounded resegmentation recovers it, 3.3x over the existing
> segmentation path and within noise of the no BIG TCP baseline.
> 
> Assisted-by: LLM
> Signed-off-by: Wang Zhan <wang.zhan@smartx.com>
> ---
>  net/core/dev.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 106 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 16685888b2812..548db4d4e874c 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3834,18 +3834,24 @@ static bool skb_gso_has_extension_hdr(const struct sk_buff *skb)
>  			 skb_inner_network_header_len(skb) != sizeof(struct ipv6hdr)));
>  }
>  
> +/*
> + * Does @skb fit the GSO limits of @dev?  The size limit depends on the L3
> + * protocol, which validate_xmit_vlan() replaces with the VLAN ethertype when
> + * it pushes the tag inside the skb, so look behind the tag.
> + */
>  static bool gso_within_device_limits(const struct sk_buff *skb,
>  				     const struct net_device *dev)
>  {
>  	return skb_shinfo(skb)->gso_segs <= READ_ONCE(dev->gso_max_segs) &&
> -	       skb->len < netif_get_gso_max_size(dev, skb->protocol);
> +	       skb->len < netif_get_gso_max_size(dev, vlan_get_protocol(skb));

If this change is needed, it is not new for this feature and should be
a separate commit.

> +static bool skb_can_gso_resegment(struct sk_buff *skb,
> +				  netdev_features_t features)
> +{
> +	__be16 protocol;
> +
> +	if (!net_gso_ok(features | NETIF_F_GSO_ROBUST,
> +			skb_shinfo(skb)->gso_type))
> +		return false;
> +
> +	if (!(features & NETIF_F_SG))
> +		return false;
> +
> +	protocol = skb_network_protocol(skb, NULL);
> +	if (!protocol || !can_checksum_protocol(features, protocol))
> +		return false;
> +
> +	/*
> +	 * The TCP frag-list path does not carry the bounded segment
> +	 * limit through skb_segment_list(). Keep bounded resegmentation
> +	 * on the regular skb path until that support is added.
> +	 */
> +	if (skb_has_frag_list(skb))
> +		return false;
> +
> +	return true;
> +}
> +
> +static unsigned int
> +skb_gso_resegment_max_segs(struct sk_buff *skb, struct net_device *dev,
> +			   netdev_features_t features)
> +{
> +	unsigned int mss = skb_shinfo(skb)->gso_size;
> +	unsigned int hdr_len, max_segs;
> +	unsigned int gso_max_size;
> +	struct tcphdr _tcph, *th;
> +
> +	gso_max_size = netif_get_gso_max_size(dev, vlan_get_protocol(skb));
> +
> +	if (!skb_is_gso(skb) || !skb_is_gso_tcp(skb) ||
> +	    skb->encapsulation || mss == GSO_BY_FRAGS ||
> +	    !skb_mac_header_was_set(skb) ||
> +	    !skb_transport_header_was_set(skb) ||
> +	    !skb_can_gso_resegment(skb, features))
> +		return 0;

Is this duplicating/extending skb_can_gso_resegment

> +
> +	th = skb_header_pointer(skb, skb_transport_offset(skb), sizeof(_tcph),
> +				&_tcph);
> +	if (!th || th->doff < sizeof(*th) / 4)
> +		return 0;
> +
> +	hdr_len = skb_transport_header(skb) - skb_mac_header(skb) +
> +		  th->doff * 4;
> +	if (gso_max_size <= hdr_len + mss)
> +		return 0;
> +
> +	/*
> +	 * gso_within_device_limits() accepts gso_segs == gso_max_segs but
> +	 * rejects skb->len >= gso_max_size, so only the size bound needs - 1.
> +	 */
> +	max_segs = (gso_max_size - hdr_len - 1) / mss;
> +	max_segs = min_t(unsigned int, max_segs,
> +			 READ_ONCE(dev->gso_max_segs));
> +
> +	return max_segs > 1 ? max_segs : 0;
> +}
> +
>  static int xmit_one(struct sk_buff *skb, struct net_device *dev,
>  		    struct netdev_queue *txq, bool more)
>  {
> @@ -4073,6 +4152,7 @@ static struct sk_buff *validate_xmit_unreadable_skb(struct sk_buff *skb,
>   */
>  static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev, bool *again)
>  {
> +	unsigned int resegment_max_segs = 0;
>  	netdev_features_t features;
>  
>  	skb = validate_xmit_unreadable_skb(skb, dev);
> @@ -4088,10 +4168,29 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
>  	if (unlikely(!skb))
>  		goto out_null;
>  
> -	if (netif_needs_gso(skb, features)) {
> +	/*
> +	 * An oversized skb loses its GSO feature bits and is segmented
> +	 * down to MSS sized skbs below.  A TCP skb can instead be split
> +	 * into GSO skbs which do fit the device, so keep the bits and
> +	 * bound the resegmentation.  The features computed without the
> +	 * limit checks say whether the device offloads the GSO type at
> +	 * all.
> +	 */
> +	if (skb_is_gso(skb) && skb_is_gso_tcp(skb) && !skb->encapsulation &&
> +	    !gso_within_device_limits(skb, dev)) {
> +		netdev_features_t offload = __netif_skb_features(skb, false);
> +
> +		resegment_max_segs =
> +			skb_gso_resegment_max_segs(skb, dev, offload);
> +		if (resegment_max_segs)
> +			features = offload;
> +	}
> +

This is a lot to put in the hot path for a rare use case. Consider how
to make this less expensive.

> +	if (resegment_max_segs || netif_needs_gso(skb, features)) {
>  		struct sk_buff *segs;
>  
> -		segs = skb_gso_segment(skb, features);
> +		segs = __skb_gso_segment(skb, features, true,
> +					 resegment_max_segs);
>  		if (IS_ERR(segs)) {
>  			goto out_kfree_skb;
>  		} else if (segs) {
> -- 
> 2.47.3
>