hi, > On Thu, Apr 14, 2011 at 07:03:49AM +0000, YAMAMOTO Takashi wrote: >> please just weaken the assertion and clear the flag, >> rather than complicating the code. > > I'm not quite sure I see exactly what you would like the code to look like. > What we have now: > > /* > * We may not use checksums on loopback interfaces > */ > if (__predict_false(ifp == NULL) || > IN_NEED_CHECKSUM(ifp, M_CSUM_IPv4)) { > if (sw_csum & M_CSUM_IPv4) { > ip->ip_sum = in_cksum(m, hlen); > m->m_pkthdr.csum_flags &= ~M_CSUM_IPv4; > } else { > KASSERT(m->m_pkthdr.csum_flags & M_CSUM_IPv4); > KASSERT(M_CSUM_DATA_IPv4_IPHL(m->m_pkthdr.csum_data) > >= > sizeof(struct ip)); > } > } > > This could be refactored into: > > if (sw_csum & M_CSUM_IPv4) { > ip->ip_sum = in_cksum(m, hlen); > m->m_pkthdr.csum_flags &= ~M_CSUM_IPv4; > } else if (__predict_false(ifp == NULL) > || IN_NEED_CHECKSUM(ifp, M_CSUM_IPv4)) { > KASSERT(m->m_pkthdr.csum_flags & M_CSUM_IPv4); > KASSERT(M_CSUM_DATA_IPv4_IPHL(m->m_pkthdr.csum_data) > >= > sizeof(struct ip)); > } > } > > or a variant that embeds the else if condition into both KASSERTs, which seems > pretty ugly to me. > > Can you please clarify?
somthing like the following. YAMAMOTO Takashi Index: ip_output.c =================================================================== RCS file: /cvsroot/src/sys/netinet/ip_output.c,v retrieving revision 1.208 diff -u -p -r1.208 ip_output.c --- ip_output.c 14 Apr 2011 15:53:36 -0000 1.208 +++ ip_output.c 25 Apr 2011 22:41:43 -0000 @@ -1010,12 +1010,19 @@ ip_fragment(struct mbuf *m, struct ifnet m->m_pkthdr.len = mhlen + len; m->m_pkthdr.rcvif = (struct ifnet *)0; mhip->ip_sum = 0; + KASSERT((m->m_pkthdr.csum_flags & M_CSUM_IPv4) == 0); if (sw_csum & M_CSUM_IPv4) { mhip->ip_sum = in_cksum(m, mhlen); - KASSERT((m->m_pkthdr.csum_flags & M_CSUM_IPv4) == 0); } else { - m->m_pkthdr.csum_flags |= M_CSUM_IPv4; + /* + * checksum is hw-offloaded or not necessary. + */ + m->m_pkthdr.csum_flags |= + m0->m_pkthdr.csum_flags & M_CSUM_IPv4; m->m_pkthdr.csum_data |= mhlen << 16; + KASSERT(!(ifp != NULL && + IN_NEED_CHECKSUM(ifp, M_CSUM_IPv4)) + || (m->m_pkthdr.csum_flags & M_CSUM_IPv4) != 0); } IP_STATINC(IP_STAT_OFRAGMENTS); fragments++; @@ -1030,19 +1037,17 @@ ip_fragment(struct mbuf *m, struct ifnet ip->ip_len = htons((u_int16_t)m->m_pkthdr.len); ip->ip_off |= htons(IP_MF); ip->ip_sum = 0; - /* - * We may not use checksums on loopback interfaces - */ - if (__predict_false(ifp == NULL) || - IN_NEED_CHECKSUM(ifp, M_CSUM_IPv4)) { - if (sw_csum & M_CSUM_IPv4) { - ip->ip_sum = in_cksum(m, hlen); - m->m_pkthdr.csum_flags &= ~M_CSUM_IPv4; - } else { - KASSERT(m->m_pkthdr.csum_flags & M_CSUM_IPv4); - KASSERT(M_CSUM_DATA_IPv4_IPHL(m->m_pkthdr.csum_data) >= - sizeof(struct ip)); - } + if (sw_csum & M_CSUM_IPv4) { + ip->ip_sum = in_cksum(m, hlen); + m->m_pkthdr.csum_flags &= ~M_CSUM_IPv4; + } else { + /* + * checksum is hw-offloaded or not necessary. + */ + KASSERT(!(ifp != NULL && IN_NEED_CHECKSUM(ifp, M_CSUM_IPv4)) + || (m->m_pkthdr.csum_flags & M_CSUM_IPv4) != 0); + KASSERT(M_CSUM_DATA_IPv4_IPHL(m->m_pkthdr.csum_data) >= + sizeof(struct ip)); } sendorfree: /*