On Mon, Jul 03, 2023 at 08:04:11PM +0300, Alexander Bluhm wrote:
> As final step before making LRO (Large Receive Offload) the default,
> we have to fix path MTU discovery when forwarding.
>
> The drivers, currently ix(4) and lo(4) only, record an upper bound
> of the size of the original packets in ph_mss. When sending we
> must chop the packets with TSO (TCP Segmentation Offload) to that
> size. That means we have to call tcp_if_output_tso() before
> ifp->if_output(). I have put that logic into if_output_tso() to
> avoid code duplication.
>
> ok?
I like the idea of this commit. Some comments below.
Thanks,
Jan
> Index: net/if.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
> retrieving revision 1.702
> diff -u -p -r1.702 if.c
> --- net/if.c 2 Jul 2023 19:59:15 -0000 1.702
> +++ net/if.c 3 Jul 2023 10:28:30 -0000
> @@ -109,6 +109,9 @@
> #include <netinet/tcp.h>
> #include <netinet/tcp_timer.h>
> #include <netinet/tcp_var.h>
I think is a merge bug, isn't it?
> +#include <netinet/tcp.h>
> +#include <netinet/tcp_timer.h>
> +#include <netinet/tcp_var.h>
> @@ -883,6 +886,57 @@ if_output_ml(struct ifnet *ifp, struct m
> ml_purge(ml);
>
> return error;
> +}
> +
> +int
> +if_output_tso(struct ifnet *ifp, struct mbuf **mp, struct sockaddr *dst,
> + struct rtentry *rt, u_int mtu)
> +{
> + uint32_t ifcap;
> + int error;
> +
> + switch (dst->sa_family) {
> + case AF_INET:
> + ifcap = IFCAP_TSOv4;
> + break;
> +#ifdef INET6
> + case AF_INET6:
> + ifcap = IFCAP_TSOv6;
> + break;
> +#endif
> + default:
> + unhandled_af(dst->sa_family);
> + }
> +
> + /*
> + * Try to send with TSO first. When forwarding LRO may set
> + * maximium segment size in mbuf header. Chop TCP segment
> + * even if it would fit interface MTU to preserve maximum
> + * path MTU.
> + */
> + error = tcp_if_output_tso(ifp, mp, dst, rt, ifcap, mtu);
> + if (error || *mp == NULL)
> + return error;
> +
> + if ((*mp)->m_pkthdr.len <= mtu) {
I may miss something but...
Couldn't you move the *_cksum_out() calls above the upper
tcp_if_output_tso() call? And than remove the *_cksum_out() calls
inside of tcp_if_output_tso()?
Thus, there is just one place where we call them.
> + switch (dst->sa_family) {
> + case AF_INET:
> + in_hdr_cksum_out(*mp, ifp);
> + in_proto_cksum_out(*mp, ifp);
> + break;
> +#ifdef INET6
> + case AF_INET6:
> + in6_proto_cksum_out(*mp, ifp);
> + break;
> +#endif
> + }
> + error = ifp->if_output(ifp, *mp, dst, rt);
> + *mp = NULL;
> + return error;
> + }
> +
> + /* mp still contains mbuf that has to be fragmented or dropped. */
> + return 0;
> }
