[ovs-dev] [patch v5 00/11] Userspace datapath: Add fragmentation support.

Aaron Conole aconole at redhat.com
Thu Feb 22 18:24:00 UTC 2018


Darrell Ball <dlu998 at gmail.com> writes:

> On Fri, Feb 9, 2018 at 1:35 PM, Aaron Conole <aconole at redhat.com> wrote:
>
>  Hi Darrell,
>
>  Darrell Ball <dlu998 at gmail.com> 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.

Something to think on.

I'm planning on running performance tests this coming week, and will
give my patch 3/11 comments then.

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


More information about the dev mailing list