Re: CVS commit: src/sys/netinet

[yamt%mwd.biglobe.ne.jp@localhost (YAMAMOTO Takashi)]

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:
        /*