On Wed, Aug 1, 2018 at 2:36 AM, Justin Pettit <[email protected]> wrote:
> > > On Jul 16, 2018, at 7:39 PM, Darrell Ball <[email protected]> wrote: > > > > +static void > > +ipf_expiry_list_add(struct ipf_list *ipf_list, long long now) > > + OVS_REQUIRES(ipf_lock) > > +{ > > + enum { > > + IPF_FRAG_LIST_TIMEOUT_DEFAULT = 15000, > > + }; > > + > > + ipf_list->expiration = now + IPF_FRAG_LIST_TIMEOUT_DEFAULT; > > + ovs_list_push_back(&frag_exp_list, &ipf_list->list_node); > > +} > > This was brought up in the previous review, and I think we should drop > "_DEFAULT", since it's not configurable. > Thats right; I was in the process of updating it along with some other changes and it slipped back in. > > > +static bool > > +ipf_return_invalid(struct dp_packet *pkt) > > +{ > > + pkt->md.ct_state = CS_INVALID; > > + return false; > > +} > > This function looks kind of odd to me. I think the same thing could be > accomplished more clearly with a goto in the functions that need it. I've > implemented it in my incremental, below. > I realize OVS does not use these types of error functions and I have used 'goto' syntax myself repeatedly in OVS and it is perfectly fine. I'll update to use 'goto' > > > + > > +static bool > > +ipf_is_valid_v4_frag(struct dp_packet *pkt) > > +{ > > + if (OVS_UNLIKELY(dp_packet_ip_checksum_bad(pkt))) { > > + return ipf_return_invalid(pkt); > > + } > > Do you think it would make sense to provide a counter when a packet gets > marked as invalid? > It is a separate series across conntrack and ipf. > > > +static void > > +ipf_extract_frags_from_batch(struct dp_packet_batch *pb, ovs_be16 > dl_type, > > + uint16_t zone, long long now, uint32_t > hash_basis) > > +{ > > + const size_t pb_cnt = dp_packet_batch_size(pb); > > + int pb_idx; /* Index in a packet batch. */ > > + struct dp_packet *pkt; > > + > > + DP_PACKET_BATCH_REFILL_FOR_EACH (pb_idx, pb_cnt, pkt, pb) { > > + > > + if (OVS_UNLIKELY((dl_type == htons(ETH_TYPE_IP) && > > + ipf_is_valid_v4_frag(pkt)) || > > + (dl_type == htons(ETH_TYPE_IPV6) && > > + ipf_is_valid_v6_frag(pkt)))) { > > Is this case unlikely? > Yes, fragments are unlikely overall. > > > diff --git a/lib/ipf.h b/lib/ipf.h > > new file mode 100644 > > index 0000000..212d1b3 > > --- /dev/null > > +++ b/lib/ipf.h > > ... > > +struct ipf_status { > > + bool ifp_v4_enabled; > > + unsigned int min_v4_frag_size; > > + unsigned int nfrag_max; > > + unsigned int nfrag; > > + unsigned int n4frag_accepted; > > + unsigned int n4frag_completed_sent; > > + unsigned int n4frag_expired_sent; > > + unsigned int n4frag_too_small; > > + unsigned int n4frag_overlap; > > + bool ifp_v6_enabled; > > + unsigned int min_v6_frag_size; > > + unsigned int n6frag_accepted; > > + unsigned int n6frag_completed_sent; > > + unsigned int n6frag_expired_sent; > > + unsigned int n6frag_too_small; > > + unsigned int n6frag_overlap; > > +}; > > This isn't used until a later patch. Let's wait to introduce it until > then. > For sure. > > > +/* Collects and reassembles fragments which are to be sent through > > + * conntrack, if fragment processing is enabled or fragments are > > + * in flight. */ > > +void ipf_preprocess_conntrack(struct dp_packet_batch *pb, long long > now, > > + ovs_be16 dl_type, uint16_t zone, > > + uint32_t hash_basis); > > + > > +/* Updates the state of fragments associated with reassembled packets > and > > + * sends out fragments that are either associated with completed > > + * packets or expired, if fragment processing is enabled or fragments > are > > + * in flight. */ > > +void ipf_postprocess_conntrack(struct dp_packet_batch *pb, long long > now, > > + ovs_be16 dl_type); > > These functions are also described in "ipf.c". Let's just use the ones > there so that we don't have to worry about them drifting apart. > thats fine with me > > I'd like to commit it with the incremental at the bottom of this message. > I will fold in. Thank you for all those fixups. I would like to defer one - the update for 'thru.' -> 'through' in system-traffic.at for now to avoid the long line thingy. > > --Justin > > > -=-=-=-=-=-=-=-=-=-=- > > diff --git a/lib/ipf.c b/lib/ipf.c > index 1169a8a5e60c..bb2e2d692036 100644 > --- a/lib/ipf.c > +++ b/lib/ipf.c > @@ -106,12 +106,12 @@ struct ipf_list { > struct ovs_list list_node; > struct ipf_frag *frag_list; > struct ipf_list_key key; > - struct dp_packet *reass_execute_ctx; /* reassembled packet. */ > - long long expiration; > - int last_sent_idx; /* last sent fragment idx. */ > - int last_inuse_idx; /* last inuse fragment idx. */ > - int size; /* fragment list size. */ > - uint8_t state; /* frag list state; see ipf_list_state. */ > + struct dp_packet *reass_execute_ctx; /* Reassembled packet. */ > + long long expiration; /* In milliseconds. */ > + int last_sent_idx; /* Last sent fragment idx. */ > + int last_inuse_idx; /* Last inuse fragment idx. */ > + int size; /* Fragment list size. */ > + uint8_t state; /* Frag list state; see > ipf_list_state. */ > }; > > struct reassembled_pkt { > @@ -266,10 +266,10 @@ ipf_expiry_list_add(struct ipf_list *ipf_list, long > long now) > OVS_REQUIRES(ipf_lock) > { > enum { > - IPF_FRAG_LIST_TIMEOUT_DEFAULT = 15000, > + IPF_FRAG_LIST_TIMEOUT = 15000, > }; > > - ipf_list->expiration = now + IPF_FRAG_LIST_TIMEOUT_DEFAULT; > + ipf_list->expiration = now + IPF_FRAG_LIST_TIMEOUT; > ovs_list_push_back(&frag_exp_list, &ipf_list->list_node); > } > > @@ -447,7 +447,7 @@ ipf_reassemble_v4_frags(struct ipf_list *ipf_list) > len += add_len; > if (len > IPV4_PACKET_MAX_SIZE) { > ipf_print_reass_packet( > - "Unsupported big reassmbled v4 packet; v4 hdr:", l3); > + "Unsupported big reassembled v4 packet; v4 hdr:", l3); > dp_packet_delete(pkt); > return NULL; > } > @@ -488,7 +488,7 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list) > pl += add_len; > if (pl > IPV6_PACKET_MAX_DATA) { > ipf_print_reass_packet( > - "Unsupported big reassmbled v6 packet; v6 hdr:", l3); > + "Unsupported big reassembled v6 packet; v6 hdr:", l3); > dp_packet_delete(pkt); > return NULL; > } > @@ -510,7 +510,7 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list) > if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, > &frag_hdr) > || !nw_frag || !frag_hdr) { > > - ipf_print_reass_packet("Unparsed reassmbled v6 packet; v6 hdr:", > l3); > + ipf_print_reass_packet("Unparsed reassembled v6 packet; v6 > hdr:", l3); > dp_packet_delete(pkt); > return NULL; > } > @@ -594,32 +594,25 @@ ipf_list_state_transition(struct ipf_list > *ipf_list, bool ff, bool lf, > ipf_list->state = next_state; > } > > -static bool > -ipf_return_invalid(struct dp_packet *pkt) > -{ > - pkt->md.ct_state = CS_INVALID; > - return false; > -} > - > static bool > ipf_is_valid_v4_frag(struct dp_packet *pkt) > { > if (OVS_UNLIKELY(dp_packet_ip_checksum_bad(pkt))) { > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > > const struct eth_header *l2 = dp_packet_eth(pkt); > const struct ip_header *l3 = dp_packet_l3(pkt); > > if (OVS_UNLIKELY(!l2 || !l3)) { > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > > const char *tail = dp_packet_tail(pkt); > uint8_t pad = dp_packet_l2_pad_size(pkt); > size_t size = tail - (char *)l3 - pad; > if (OVS_UNLIKELY(size < IP_HEADER_LEN)) { > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > > if (!(IP_IS_FRAGMENT(l3->ip_frag_off))) { > @@ -628,20 +621,20 @@ ipf_is_valid_v4_frag(struct dp_packet *pkt) > > uint16_t ip_tot_len = ntohs(l3->ip_tot_len); > if (OVS_UNLIKELY(ip_tot_len != size)) { > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > > size_t ip_hdr_len = IP_IHL(l3->ip_ihl_ver) * 4; > if (OVS_UNLIKELY(ip_hdr_len < IP_HEADER_LEN)) { > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > if (OVS_UNLIKELY(size < ip_hdr_len)) { > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > > if (OVS_UNLIKELY(!dp_packet_ip_checksum_valid(pkt) > && csum(l3, ip_hdr_len) != 0)) { > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > > uint32_t min_v4_frag_size_; > @@ -649,9 +642,13 @@ ipf_is_valid_v4_frag(struct dp_packet *pkt) > bool lf = ipf_is_last_v4_frag(pkt); > if (OVS_UNLIKELY(!lf && dp_packet_size(pkt) < min_v4_frag_size_)) { > ipf_count(false, IPF_COUNTER_NFRAGS_TOO_SMALL); > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > return true; > + > +invalid_pkt: > + pkt->md.ct_state = CS_INVALID; > + return false; > } > > static bool > @@ -686,7 +683,7 @@ ipf_is_valid_v6_frag(struct dp_packet *pkt OVS_UNUSED) > const char *l4 = dp_packet_l4(pkt); > > if (OVS_UNLIKELY(!l2 || !l3 || !l4)) { > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > > const char *tail = dp_packet_tail(pkt); > @@ -695,7 +692,7 @@ ipf_is_valid_v6_frag(struct dp_packet *pkt OVS_UNUSED) > size_t l3_hdr_size = sizeof *l3; > > if (OVS_UNLIKELY(l3_size < l3_hdr_size)) { > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > > uint8_t nw_frag = 0; > @@ -710,7 +707,7 @@ ipf_is_valid_v6_frag(struct dp_packet *pkt OVS_UNUSED) > > int pl = ntohs(l3->ip6_plen); > if (OVS_UNLIKELY(pl + l3_hdr_size != l3_size)) { > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > > ovs_be16 ip6f_offlg = frag_hdr->ip6f_offlg; > @@ -724,10 +721,14 @@ ipf_is_valid_v6_frag(struct dp_packet *pkt > OVS_UNUSED) > > if (OVS_UNLIKELY(!lf && dp_packet_size(pkt) < min_v6_frag_size_)) { > ipf_count(true, IPF_COUNTER_NFRAGS_TOO_SMALL); > - return ipf_return_invalid(pkt); > + goto invalid_pkt; > } > > return true; > + > +invalid_pkt: > + pkt->md.ct_state = CS_INVALID; > + return false; > } > > static void > @@ -983,6 +984,7 @@ ipf_dp_packet_batch_add(struct dp_packet_batch *pb , > struct dp_packet *pkt, > * reason known right now is a mempool source check, which exists due to > DPDK > * support, where packets are no longer being received on any port with a > * source matching the fragment. > + * > * Returns true if the list was purged. */ > static bool > ipf_purge_list_check(struct ipf_list *ipf_list, long long now) > @@ -1201,7 +1203,7 @@ ipf_post_execute_reass_pkts(struct dp_packet_batch > *pb, bool v6) > > /* Extracts any fragments from the batch and reassembles them when a > * complete packet is received. Completed packets are attempted to > - * be added to the batch to be sent thru. conntrack. */ > + * be added to the batch to be sent through conntrack. */ > void > ipf_preprocess_conntrack(struct dp_packet_batch *pb, long long now, > ovs_be16 dl_type, uint16_t zone, uint32_t > hash_basis) > @@ -1216,7 +1218,7 @@ ipf_preprocess_conntrack(struct dp_packet_batch > *pb, long long now, > } > > /* Updates fragments based on the processing of the reassembled packet > sent > - * thru. conntrack and adds these fragments to any batches seen. Expired > + * through conntrack and adds these fragments to any batches seen. > Expired > * fragments are marked as invalid and also added to the batches seen > * with low priority. Reassembled packets are freed. */ > void > diff --git a/lib/ipf.h b/lib/ipf.h > index 212d1b3458f9..040031f0190c 100644 > --- a/lib/ipf.h > +++ b/lib/ipf.h > @@ -20,41 +20,14 @@ > #include "dp-packet.h" > #include "openvswitch/types.h" > > -struct ipf_status { > - bool ifp_v4_enabled; > - unsigned int min_v4_frag_size; > - unsigned int nfrag_max; > - unsigned int nfrag; > - unsigned int n4frag_accepted; > - unsigned int n4frag_completed_sent; > - unsigned int n4frag_expired_sent; > - unsigned int n4frag_too_small; > - unsigned int n4frag_overlap; > - bool ifp_v6_enabled; > - unsigned int min_v6_frag_size; > - unsigned int n6frag_accepted; > - unsigned int n6frag_completed_sent; > - unsigned int n6frag_expired_sent; > - unsigned int n6frag_too_small; > - unsigned int n6frag_overlap; > -}; > - > -/* Collects and reassembles fragments which are to be sent through > - * conntrack, if fragment processing is enabled or fragments are > - * in flight. */ > void ipf_preprocess_conntrack(struct dp_packet_batch *pb, long long now, > ovs_be16 dl_type, uint16_t zone, > uint32_t hash_basis); > > -/* Updates the state of fragments associated with reassembled packets and > - * sends out fragments that are either associated with completed > - * packets or expired, if fragment processing is enabled or fragments are > - * in flight. */ > void ipf_postprocess_conntrack(struct dp_packet_batch *pb, long long now, > ovs_be16 dl_type); > > void ipf_init(void); > - > void ipf_destroy(void); > > #endif /* ipf.h */ > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 556a6a0db2d0..822122093944 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -2140,7 +2140,7 @@ packet-out in_port=1, packet= > 50540000000a505400000009080045000030000100310011a48 > ]) > > AT_CHECK([ovs-ofctl bundle br0 bundle.txt]) > -# There is one byte of overlap, hence the no packet gets thru. conntrack. > +# There is one byte of overlap, hence the no packet gets through > conntrack. > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], > [dnl > ]) > > @@ -2164,7 +2164,7 @@ packet-out in_port=1, packet= > 50540000000a5054000000090800450001a4000120000011834 > ]) > > AT_CHECK([ovs-ofctl bundle br0 bundle.txt]) > -# There is one byte of overlap, hence the no packet gets thru. conntrack. > +# There is one byte of overlap, hence the no packet gets through > conntrack. > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], > [dnl > ]) > > I would like to defer the update to "thru." in system-traffic.at to avoid the long line thingy. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
