I'll respond to the individual comments soon, although some comments are answered by subsequent patches. I did notice a problem that I will fix and make adjustments for, in that the ipf context should have been per datapath.
Thanks Darrell On Tue, Dec 11, 2018 at 8:15 AM Ben Pfaff <[email protected]> wrote: > On Mon, Nov 19, 2018 at 11:09:25AM -0800, Darrell Ball wrote: > > Fragmentation handling is added for supporting conntrack. > > Both v4 and v6 are supported. > > > > After discussion with several people, I decided to not store > > configuration state in the database to be more consistent with > > the kernel in future, similarity with other conntrack configuration > > which will not be in the database as well and overall simplicity. > > Accordingly, fragmentation handling is enabled by default. > > > > This patch enables fragmentation tests for the userspace datapath. > > > > Signed-off-by: Darrell Ball <[email protected]> > > Thanks for implementing this. > > This could use more comments, especially on the data structures and > file-level variables and at the level of an individual function. > > Please don't invent yet another mutex. > > ipf_print_reass_packet() seems like it could use ds_put_hex_dump() and > thereby be more readable (I don't really want to read 182 hex digits in > a row without any white space). > > Why 91 bytes in ipf_print_reass_packet()? > > All the counters seem to be write-only. > > struct ipf_addr seems odd to me. It has both aligned and unaligned > versions of addresses, which means that the overall struct needs to be > aligned, and it's a union nested in a struct instead of just a union. > ipf_addr_hash_add() implies that all the bytes in the struct need to be > initialized even if only some of them are used. > > ipf_list_key_hash() seems to be at risk of hashing trailing padding at > the end of struct ipf_list_key. > > Does ipf_list_complete() run one past the end of the array? Naively, it > seems like it might. > > I found ipf_sort() a little hard to read mostly due to the long variable > names. Here's an alternate form that you can accept or reject as you > like (I have not tested it): > > static void > ipf_sort(struct ipf_frag *frags, size_t last_idx) > OVS_REQUIRES(ipf_lock) > { > for (int i = 1; i <= last_idx; i++) { > struct ipf_frag ipf_frag = frags[i]; > int j = i - 1; > while (j >= 0 && frags[j].start_data_byte > > ipf_frag.start_data_byte) { > frags[j + 1] = frags[j]; > j--; > } > frags[j + 1] = ipf_frag; > } > } > > ipf_reassemble_v4_frags() and ipf_reassemble_v6_frags() know the final > length of the packet they're assembling, but it doesn't look to me like > they preallocate the space for it. > > ipf_reassemble_v6_frags() and ipf_reassemble_v6_frags() calculate the > length of the L3 header manually and skip past it. Couldn't they just > use the L4 header pointer? > > The ipf_list_key_cmp() interface is a little confusing because the > return value convention and the name is a little like a strcmp()ish > function, but it doesn't use a -1/+1 return value. I'd rename it to > ipf_list_key_equal()s, change the return type to "bool", and make true > mean equal, false mean unequal. > > Processing a fragment is O(n) in the number of existing fragments due to > the dup check. I don't know whether this is important. > > It looks like packet duplication can cause a fraglist to be marked > invalid. I wonder whether this is good behavior. > > It seems like we could estimate the number of fragments needed by > dividing the total size by the size of the first fragment received. > > Do we need xzalloc() for frag_list? It seems like we're going to > initialize each element as needed anyway. > > I think there's a race if either v4 or v6 is ever disabled, because the > code checks whether fragment reassembly is enabled twice, once at a high > level and once again in ipf_handle_frag(). The latter function > assert-fails if reassembly is disabled, which seems like it could be a > problem. > > At the same time, I don't see any way to ever disable fragment > reassembly. The code appears to enable it by default and not provide > any way to disable it. > > Is it common practice to enforce a minimum size for fragments? > > Maybe it would be a little easier to make the counters a two-dimensional > array: atomic_uint64_t ipf_counters[2][N_COUNTERS], where one dimension > is IPv4/IPv6 and the other is the particular counter. Then ipf_count() > would become trivial. Just a thought though. > > Thanks, and I'll look forward to the next version. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
