Shouldn't the version check come first and the code avoid further checks if it fails? Probably unlikely to fail though given execution has reached "ip4_input" :)
Thanks, Chris. > On Jul 21, 2020, at 7:12 AM, Dave Barach via lists.fd.io > <dbarach=cisco....@lists.fd.io> wrote: > > Branches hurt performance more than arithmetic and/or conditional moves. > > From: vpp-dev@lists.fd.io <mailto:vpp-dev@lists.fd.io> <vpp-dev@lists.fd.io > <mailto:vpp-dev@lists.fd.io>> On Behalf Of "?? > Sent: Monday, July 20, 2020 10:17 PM > To: vpp-dev <vpp-dev@lists.fd.io <mailto:vpp-dev@lists.fd.io>> > Subject: [vpp-dev] Why not break the check flow when the first error occurs > of ip4_inout()? > > Hi all, > > When I'm reading the ip4_input.c code, I'm confused about the following code. > > vnet/vnet/ip/ip4_input.c:ip4_input_inline() > { > ... > error0 = error1 = IP4_ERROR_NONE; > > /* Punt packets with options. */ > error0 = (ip0->ip_version_and_header_length & 0xf) != 5 ? > IP4_ERROR_OPTIONS : error0; > error1 = (ip1->ip_version_and_header_length & 0xf) != 5 ? > IP4_ERROR_OPTIONS : error1; > > /* Version != 4? Drop it. */ > error0 = (ip0->ip_version_and_header_length >> 4) != 4 ? > IP4_ERROR_VERSION : error0; > error1 = (ip1->ip_version_and_header_length >> 4) != 4 ? > IP4_ERROR_VERSION : error1; > > /* Verify header checksum. */ > .... > error0 = 0xffff != ip_csum_fold (sum0) ? IP4_ERROR_BAD_CHECKSUM : > error0; > error1 = 0xffff != ip_csum_fold (sum1) ? IP4_ERROR_BAD_CHECKSUM : > error1; > > /* Drop fragmentation offset 1 packets. */ > .... > error0 = ip4_get_fragment_offset (ip0) == 1 ? > IP4_ERROR_FRAGMENT_OFFSET_ONE : error0; > error1 = ip4_get_fragment_offset (ip1) == 1 ? > IP4_ERROR_FRAGMENT_OFFSET_ONE : error1; > > /* TTL < 1? Drop it. */ > error0 = (ip0->ttl < 1 && cast0 == VNET_UNICAST) ? > IP4_ERROR_TIME_EXPIRED : error0; > error1 = (ip1->ttl < 1 && cast1 == VNET_UNICAST) ? > IP4_ERROR_TIME_EXPIRED : error1; > > /* Verify lengths. */ > error0 = ip_len0 < sizeof (ip0[0]) ? IP4_ERROR_TOO_SHORT : error0; > error1 = ip_len1 < sizeof (ip1[0]) ? IP4_ERROR_TOO_SHORT : error1; > > p0->error = error_node->errors[error0]; > p1->error = error_node->errors[error1]; > > if (PREDICT_FALSE(error0 != IP4_ERROR_NONE)) > { > if (error0 == IP4_ERROR_TIME_EXPIRED) { > icmp4_error_set_vnet_buffer(p0, ICMP4_time_exceeded, > ICMP4_time_exceeded_ttl_exceeded_in_transit, 0); > next0 = IP4_INPUT_NEXT_ICMP_ERROR; > } else > next0 = error0 != IP4_ERROR_OPTIONS ? IP4_INPUT_NEXT_DROP > : IP4_INPUT_NEXT_PUNT; > } > if (PREDICT_FALSE(error1 != IP4_ERROR_NONE)) > { > if (error1 == IP4_ERROR_TIME_EXPIRED) { > icmp4_error_set_vnet_buffer(p1, ICMP4_time_exceeded, > ICMP4_time_exceeded_ttl_exceeded_in_transit, 0); > next1 = IP4_INPUT_NEXT_ICMP_ERROR; > } else > next1 = error1 != IP4_ERROR_OPTIONS ? IP4_INPUT_NEXT_DROP > : IP4_INPUT_NEXT_PUNT; > } > } > > ----------------------------------------------------------------------------------------------- > > 1. why not break the check flow when the first error occurs? for example: > error0 = (ip0->ip_version_and_header_length & 0xf) != 5 ? > IP4_ERROR_OPTIONS : error0; > if (error0 != IP4_ERROR_NONE) {goto error_handle;} > > 2. suppose all the results of checking are wrong, I think at the end, we > could only get the last error rather than the first error. > options error, version error, ...and length error, so the last error is > IP4_ERROR_TOO_SHORT > > Is there any optimization concept behind this？ Can I improe(hope it will > improve :-)） the code like the following, before we check the next error, > let's check wether the error has occured: > > /* init error to none */ > error0 = error1 = IP4_ERROR_NONE; > > /* check the first error */ > error0 = (ip0->ip_version_and_header_length & 0xf) != 5 ? IP4_ERROR_OPTIONS > : error0; > > error0 = error0 != IP4_ERROR_NONE? error0 : > ((ip0->ip_version_and_header_length >> 4) != 4 ? IP4_ERROR_VERSION : error0); > error0 = error0 != IP4_ERROR_NONE? error0 : (0xffff != ip_csum_fold (sum0) > ? IP4_ERROR_BAD_CHECKSUM : error0); > ... > > >
signature.asc
Description: Message signed with OpenPGP
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#17014): https://lists.fd.io/g/vpp-dev/message/17014 Mute This Topic: https://lists.fd.io/mt/75696670/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
