Hi Aaron I totally missed this response - sorry!
On Thu, Feb 22, 2018 at 10:24 AM, Aaron Conole <[email protected]> wrote: > Darrell Ball <[email protected]> writes: > > > On Fri, Feb 9, 2018 at 1:35 PM, Aaron Conole <[email protected]> wrote: > > > > Hi Darrell, > > > > Darrell Ball <[email protected]> writes: > > > > > Fragmentation support for userspace datapath conntrack is added; both > > > v4 and v6 are supported. See the patches for additional details. > > > > Very pumped about this! > > > > I went to start reviewing this, but found out that 04/ didn't apply > > correctly. No problem, I'll proceed - just figured I'd let you know > > that: > > > > The merge conflict in NEWS was due to 2 subsequent applies on Feb 6, > while > > the series was posted on Feb 4. > > Yep. I've worked around it. > > > 1. I'm looking at it :) > > > > Thanks Aaron > > I have a question on patches 5-7: > > Do you think it's worthwhile having these as configuration options in > the database? > > I can see where it could be considered that they aren't appropriate > (after all, the linux fragmentation support is enabled/disabled not via > the netlink interface, but by the system administrator outside of the > ovs configuration). On the other hand, from a user perspective it's > probably more friendly to have that configuration knob be persistent. > I think there is value in having the configuration for ipfrag be stored > in a database so that each 'restart' doesn't require an extra set of > commands be run. I envision a situation where users have their system > configured, restart, and get confused because they have to manually > re-enable the configuration. There are a few ways this could be done > (for instance, a post-start script that runs after ovs-ctl), but I think > the database might be a useful way of accomplishing that goal - it > exists and we use it for dpdk configurations, ex. For global and interface configuration, yes. For flow specific information, the database has very limited usage; notably, there is flow eviction policy an ip prefix settings. I did not opt for using the database here for a few reasons: 1/ Scripts for starts and restarts will be the same and hence the possibility to forget for restart is pretty low; database commands themselves are also run from scripts, for example for interfaces in general and various other stuff. 2/ Using the database, introduces more complexity and dependencies for limited gain for something that is usually set once at startup and never changes after that. I was/am simply leaning towards not using the database here for reasons mentioned above, although I did not think it was a clear question one way or the other and I expected the question to be mentioned in review. > > Something to think on. > I'm planning on running performance tests this coming week, and will > give my patch 3/11 comments then. > Thanks, right now I have not optimized this fully although I identified some enhancements. Fragmentation usage in itself is highly suboptimal and preferred usage is either none or a very limited percentage of total pps. > Thanks for working on this, Darrell! > > > Darrell > > > > > > 2. it'll need at least one more spin (but no need to rush that since > > I'll move myself past the error) > > > > Thanks, > > -Aaron > > > > > v4->v5: Added a sub-feature to optionally dump fragmentation lists. > > > This is useful for DOS forensics and debugging. > > > > > > The testing coverage was also extended including checking > > > more counters and frag list occupancies. > > > > > > Fixed a few bugs: > > > 1/ Handle dpdk mempool source restrictions for a batch of > > > packets from multiple sources; this also brings in a purge > > > frag list function to handle pathological cases of stuck > frags. > > > 2/ ipf_destroy was missing packet frees for frag lists. > > > 3/ A setting of CS_INVALID was missing for expired packets - > > > I mentioned this earlier for version 4. > > > > > > Some enhancements and coding standards changes for Patch 3. > > > > > > v3->v4: Add V6 support to the patches. > > > Fix possible race cleanup bug when the user disables > > > fragmentation and there are list occupancies, not cleaned > up > > > yet. > > > Add missed orig tuple fields for copy from reassembled packet > > > to fragments. > > > Fix an fragment list increment check - shoiuld have been "> 0" > > > rather then "!= 0". > > > Fix max frags calculation in case of theoretical corner case. > > > Add proper lock annotations. > > > Made some other improvements while adding V6 support. > > > > > > v2->v3: Patch 2 was updated: > > > Remove "XXX" todo items by implementing the ones needed, > > > including realloc frag_list contexts to save memory. > > > Fix related bug with max_frag_list_size when min_frag_size is > > > reconfigured. > > > > > > Tighten ip_tot_len sanity check for reassembled packets which > > > was more loose than intended. > > > > > > Add another sanity check for fragment ip_tot_len; even though > > > it be redundant, add for completeness. > > > > > > v1->v2: Few fixes, improvements and cleanups. > > > > > > Darrell Ball (11): > > > dp-packet: Add const qualifiers for checksum apis. > > > flow: Enhance parse_ipv6_ext_hdrs. > > > Userspace datapath: Add fragmentation handling. > > > conntrack: Support fragmentation. > > > ipf: Add command to enable fragmentation handling. > > > ipf: Add set minimum fragment size command. > > > ipf: Add set maximum fragments supported command. > > > ipf: Add command to get fragmentation handling status. > > > ipf: Enhance ipf_get_status. > > > tests: Add missed local stack checks. > > > tests: Enable fragmentation for userspace datapath. > > > > > > NEWS | 10 + > > > include/sparse/netinet/ip6.h | 1 + > > > lib/automake.mk | 2 + > > > lib/conntrack.c | 10 +- > > > lib/ct-dpif.c | 69 ++ > > > lib/ct-dpif.h | 13 + > > > lib/dp-packet.h | 4 +- > > > lib/dpctl.c | 216 ++++++ > > > lib/dpctl.man | 32 + > > > lib/dpif-netdev.c | 83 +++ > > > lib/dpif-netlink.c | 7 + > > > lib/dpif-provider.h | 18 + > > > lib/flow.c | 23 +- > > > lib/flow.h | 3 +- > > > lib/ipf.c | 1390 > ++++++++++++++++++++++++++++++++++++++ > > > lib/ipf.h | 86 +++ > > > tests/system-kmod-macros.at | 30 +- > > > tests/system-traffic.at | 45 +- > > > tests/system-userspace-macros.at | 125 +++- > > > 19 files changed, 2129 insertions(+), 38 deletions(-) > > > create mode 100644 lib/ipf.c > > > create mode 100644 lib/ipf.h > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
