On Fri, Oct 18, 2013 at 03:27:09PM -0400, Lawrence Teo wrote:
> Back in August I sent a diff to fix ICMP checksum calculation in
> in_proto_cksum_out() and in_delayed_cksum() in cases where the ICMP
> checksum field is not in the first mbuf of an mbuf chain (original post
> at http://marc.info/?l=openbsd-tech&m=137571298511653&w=2 ).
>
> bluhm@ replied on tech@ with the following feedback:
>
> On Fri, Aug 09, 2013 at 02:21:29AM +0200, Alexander Bluhm wrote:
> > On Mon, Aug 05, 2013 at 10:28:57AM -0400, Lawrence Teo wrote:
> > > Index: ip_output.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/netinet/ip_output.c,v
> > > retrieving revision 1.244
> > > diff -U5 -p -r1.244 ip_output.c
> > > --- ip_output.c 31 Jul 2013 15:41:52 -0000 1.244
> > > +++ ip_output.c 5 Aug 2013 02:44:20 -0000
> > > @@ -2058,25 +2058,35 @@ ip_mloopback(struct ifnet *ifp, struct m
> > > */
> > > void
> > > in_delayed_cksum(struct mbuf *m)
> > > {
> > > struct ip *ip;
> > > - u_int16_t csum, offset;
> > > + u_int16_t csum = 0, offset;
> > >
> > > ip = mtod(m, struct ip *);
> > > offset = ip->ip_hl << 2;
> > > +
> > > + if (ip->ip_p == IPPROTO_ICMP)
> > > + if (m_copyback(m, offset + offsetof(struct icmp, icmp_cksum),
> > > + sizeof(csum), &csum, M_NOWAIT))
> > > + return;
> >
> > The code at the end of this function tries to avoid the m_copyback()
> > in the common case unless (offset + sizeof(u_int16_t)) > m->m_len).
> > Do we want this optimization here?
> >
> > bluhm
>
> Here's my revised diff that preserves that optimization so that existing
> behavior won't change in the common case where the protocol checksum
> field is in the first mbuf.
>
> The new diff also implements similar logic in in6_proto_cksum_out().
>
> Comments/feedback appreciated. :)
Is there any good reason why you pass u_int8_t proto, u_int16_t
p_off to the in_delayed_cksum() now? What was wrong with the switch
in your previous diff? I think splitting the offset calculation
between in_delayed_cksum() and in_proto_cksum_out() doesn't make
it better.
The optimisation is fine now.
bluhm
>
> Thanks,
> Lawrence
>
>
> Index: netinet/in.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet/in.h,v
> retrieving revision 1.97
> diff -u -p -u -p -r1.97 in.h
> --- netinet/in.h 9 Oct 2013 09:33:43 -0000 1.97
> +++ netinet/in.h 16 Oct 2013 15:14:39 -0000
> @@ -835,7 +835,7 @@ int in_broadcast(struct in_addr, stru
> int in_canforward(struct in_addr);
> int in_cksum(struct mbuf *, int);
> int in4_cksum(struct mbuf *, u_int8_t, int, int);
> -void in_delayed_cksum(struct mbuf *);
> +void in_delayed_cksum(struct mbuf *, u_int8_t, u_int16_t);
> int in_localaddr(struct in_addr, u_int);
> void in_socktrim(struct sockaddr_in *);
> char *inet_ntoa(struct in_addr);
> Index: netinet/ip_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> retrieving revision 1.247
> diff -u -p -u -p -r1.247 ip_output.c
> --- netinet/ip_output.c 18 Oct 2013 09:04:03 -0000 1.247
> +++ netinet/ip_output.c 18 Oct 2013 15:14:04 -0000
> @@ -2050,34 +2050,35 @@ ip_mloopback(struct ifnet *ifp, struct m
> * Process a delayed payload checksum calculation.
> */
> void
> -in_delayed_cksum(struct mbuf *m)
> +in_delayed_cksum(struct mbuf *m, u_int8_t proto, u_int16_t p_off)
> {
> struct ip *ip;
> - u_int16_t csum, offset;
> + u_int16_t csum = 0, hlen, *p = NULL;
>
> ip = mtod(m, struct ip *);
> - offset = ip->ip_hl << 2;
> - csum = in4_cksum(m, 0, offset, m->m_pkthdr.len - offset);
> - if (csum == 0 && ip->ip_p == IPPROTO_UDP)
> - csum = 0xffff;
> -
> - switch (ip->ip_p) {
> - case IPPROTO_TCP:
> - offset += offsetof(struct tcphdr, th_sum);
> - break;
> -
> - case IPPROTO_UDP:
> - offset += offsetof(struct udphdr, uh_sum);
> - break;
> -
> - default:
> + if (ip->ip_p != proto)
> return;
> +
> + hlen = ip->ip_hl << 2;
> + p_off += hlen;
> + if ((p_off + sizeof(u_int16_t)) <= m->m_len)
> + p = (u_int16_t *)(mtod(m, caddr_t) + p_off);
> +
> + if (proto == IPPROTO_ICMP) {
> + if (p)
> + *p = 0;
> + else if (m_copyback(m, p_off, sizeof(csum), &csum, M_NOWAIT))
> + return;
> }
>
> - if ((offset + sizeof(u_int16_t)) > m->m_len)
> - m_copyback(m, offset, sizeof(csum), &csum, M_NOWAIT);
> + csum = in4_cksum(m, 0, hlen, m->m_pkthdr.len - hlen);
> + if (csum == 0 && proto == IPPROTO_UDP)
> + csum = 0xffff;
> +
> + if (p)
> + *p = csum;
> else
> - *(u_int16_t *)(mtod(m, caddr_t) + offset) = csum;
> + m_copyback(m, p_off, sizeof(csum), &csum, M_NOWAIT);
> }
>
> void
> @@ -2086,25 +2087,20 @@ in_proto_cksum_out(struct mbuf *m, struc
> if (m->m_pkthdr.csum_flags & M_TCP_CSUM_OUT) {
> if (!ifp || !(ifp->if_capabilities & IFCAP_CSUM_TCPv4) ||
> ifp->if_bridgeport != NULL) {
> - in_delayed_cksum(m);
> + in_delayed_cksum(m, IPPROTO_TCP,
> + offsetof(struct tcphdr, th_sum));
> m->m_pkthdr.csum_flags &= ~M_TCP_CSUM_OUT; /* Clear */
> }
> } else if (m->m_pkthdr.csum_flags & M_UDP_CSUM_OUT) {
> if (!ifp || !(ifp->if_capabilities & IFCAP_CSUM_UDPv4) ||
> ifp->if_bridgeport != NULL) {
> - in_delayed_cksum(m);
> + in_delayed_cksum(m, IPPROTO_UDP,
> + offsetof(struct udphdr, uh_sum));
> m->m_pkthdr.csum_flags &= ~M_UDP_CSUM_OUT; /* Clear */
> }
> } else if (m->m_pkthdr.csum_flags & M_ICMP_CSUM_OUT) {
> - struct ip *ip = mtod(m, struct ip *);
> - int hlen;
> - struct icmp *icp;
> -
> - hlen = ip->ip_hl << 2;
> - icp = (struct icmp *)(mtod(m, caddr_t) + hlen);
> - icp->icmp_cksum = 0;
> - icp->icmp_cksum = in4_cksum(m, 0, hlen,
> - ntohs(ip->ip_len) - hlen);
> + in_delayed_cksum(m, IPPROTO_ICMP,
> + offsetof(struct icmp, icmp_cksum));
> m->m_pkthdr.csum_flags &= ~M_ICMP_CSUM_OUT; /* Clear */
> }
> }
> Index: netinet6/ip6_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_output.c,v
> retrieving revision 1.144
> diff -u -p -u -p -r1.144 ip6_output.c
> --- netinet6/ip6_output.c 17 Oct 2013 16:27:46 -0000 1.144
> +++ netinet6/ip6_output.c 18 Oct 2013 15:06:23 -0000
> @@ -126,7 +126,7 @@ int ip6_splithdr(struct mbuf *, struct i
> int ip6_getpmtu(struct route_in6 *, struct route_in6 *,
> struct ifnet *, struct in6_addr *, u_long *, int *);
> int copypktopts(struct ip6_pktopts *, struct ip6_pktopts *, int);
> -void in6_delayed_cksum(struct mbuf *, u_int8_t);
> +void in6_delayed_cksum(struct mbuf *, u_int8_t, u_int16_t);
> void in6_proto_cksum_out(struct mbuf *, struct ifnet *);
>
> /* Context for non-repeating IDs */
> @@ -3173,44 +3173,35 @@ ip6_randomid_init(void)
> * Process a delayed payload checksum calculation.
> */
> void
> -in6_delayed_cksum(struct mbuf *m, u_int8_t nxt)
> +in6_delayed_cksum(struct mbuf *m, u_int8_t nxt, u_int16_t p_off)
> {
> int nxtp, offset;
> - u_int16_t csum;
> + u_int16_t csum = 0, *p = NULL;
>
> offset = ip6_lasthdr(m, 0, IPPROTO_IPV6, &nxtp);
> if (offset <= 0 || nxtp != nxt)
> /* If the desired next protocol isn't found, punt. */
> return;
>
> + p_off += offset;
> + if ((p_off + sizeof(u_int16_t)) <= m->m_len)
> + p = (u_int16_t *)(mtod(m, caddr_t) + p_off);
> +
> if (nxt == IPPROTO_ICMPV6) {
> - struct icmp6_hdr *icmp6;
> - icmp6 = (struct icmp6_hdr *)(mtod(m, caddr_t) + offset);
> - icmp6->icmp6_cksum = 0;
> + if (p)
> + *p = 0;
> + else if (m_copyback(m, p_off, sizeof(csum), &csum, M_NOWAIT))
> + return;
> }
>
> csum = (u_int16_t)(in6_cksum(m, nxt, offset, m->m_pkthdr.len - offset));
> + if (csum == 0 && nxt == IPPROTO_UDP)
> + csum = 0xffff;
>
> - switch (nxt) {
> - case IPPROTO_TCP:
> - offset += offsetof(struct tcphdr, th_sum);
> - break;
> -
> - case IPPROTO_UDP:
> - offset += offsetof(struct udphdr, uh_sum);
> - if (csum == 0)
> - csum = 0xffff;
> - break;
> -
> - case IPPROTO_ICMPV6:
> - offset += offsetof(struct icmp6_hdr, icmp6_cksum);
> - break;
> - }
> -
> - if ((offset + sizeof(u_int16_t)) > m->m_len)
> - m_copyback(m, offset, sizeof(csum), &csum, M_NOWAIT);
> + if (p)
> + *p = csum;
> else
> - *(u_int16_t *)(mtod(m, caddr_t) + offset) = csum;
> + m_copyback(m, p_off, sizeof(csum), &csum, M_NOWAIT);
> }
>
> void
> @@ -3219,17 +3210,20 @@ in6_proto_cksum_out(struct mbuf *m, stru
> if (m->m_pkthdr.csum_flags & M_TCP_CSUM_OUT) {
> if (!ifp || !(ifp->if_capabilities & IFCAP_CSUM_TCPv6) ||
> ifp->if_bridgeport != NULL) {
> - in6_delayed_cksum(m, IPPROTO_TCP);
> + in6_delayed_cksum(m, IPPROTO_TCP,
> + offsetof(struct tcphdr, th_sum));
> m->m_pkthdr.csum_flags &= ~M_TCP_CSUM_OUT; /* Clear */
> }
> } else if (m->m_pkthdr.csum_flags & M_UDP_CSUM_OUT) {
> if (!ifp || !(ifp->if_capabilities & IFCAP_CSUM_UDPv6) ||
> ifp->if_bridgeport != NULL) {
> - in6_delayed_cksum(m, IPPROTO_UDP);
> + in6_delayed_cksum(m, IPPROTO_UDP,
> + offsetof(struct udphdr, uh_sum));
> m->m_pkthdr.csum_flags &= ~M_UDP_CSUM_OUT; /* Clear */
> }
> } else if (m->m_pkthdr.csum_flags & M_ICMP_CSUM_OUT) {
> - in6_delayed_cksum(m, IPPROTO_ICMPV6);
> + in6_delayed_cksum(m, IPPROTO_ICMPV6,
> + offsetof(struct icmp6_hdr, icmp6_cksum));
> m->m_pkthdr.csum_flags &= ~M_ICMP_CSUM_OUT; /* Clear */
> }
> }
