[ovs-dev] [PATCH] ovs-ofctl: Fix memory exhaustion failures in corner cases.

[Jarno Rajahalme]

With notes below:

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

On Nov 6, 2014, at 10:03 AM, Ben Pfaff <blp at nicira.com> wrote:

> Inserting or removing a sequence of flows with different wildcard patterns
> causes temporary use of O(n**2) memory due to pvector modifications inside
> the classifier.  This commit fixes the problem in two easy cases.  There
> is at least one more difficult case inside ovs-vswitchd that this does not
> fix.
> 

I’m curious about the case inside ovs-vswitchd...

> VMware-BZ: #1322017
> CC: Jarno Rajahalme <jrajahalme at nicira.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> tests/ovs-ofctl.at    | 13 +++++++++++++
> utilities/ovs-ofctl.c |  3 +++
> 2 files changed, 16 insertions(+)
> 
> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> index 446d8f5..7290ef1 100644
> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -2650,6 +2650,19 @@ AT_CHECK([ovs-ofctl diff-flows flows.txt br0], [2], [dnl
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> 
> +AT_SETUP([ovs-ofctl diff-flows - N**2 memory use])
> +# A bug in prior versions made this use on the order of 50000**2 * 8
> +# bytes (about 20 GB) of RAM on 64-bit machines.  The fixed version
> +# should use much less than 1 GB.  We try to set a virtual memory
> +# limit of 1 GB, to force a failure in the presence of the bug even on
> +# a machine with a ridiculous amount of RAM (and to prevent swap
> +# death).  We don't bother to check for failure of the ulimit command
> +# since it's really just a precaution.
> +ulimit -v 1048576
> +for i in `seq 1 50000`; do echo "reg0=$i/$i actions=drop"; done > flows
> +AT_CHECK([ovs-ofctl diff-flows flows flows])
> +AT_CLEANUP
> +

Just a note: This test is pretty slow on my VM, I’ll look if we can make this run faster.

> AT_SETUP([ovs-ofctl -F and -O interaction])
> AT_CHECK([ovs-ofctl -F oxm -O openflow10], [1], [],
>   [ovs-ofctl: None of the enabled OpenFlow versions (OpenFlow10) supports any of the enabled flow formats (OXM).  (Use -O to enable additional OpenFlow versions or -F to enable additional flow formats.)
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 211c276..3d1d589 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -50,6 +50,7 @@
> #include "ofproto/ofproto.h"
> #include "openflow/nicira-ext.h"
> #include "openflow/openflow.h"
> +#include "ovs-rcu.h"
> #include "packets.h"
> #include "pcap-file.h"
> #include "poll-loop.h"
> @@ -2349,6 +2350,7 @@ fte_free_all(struct classifier *cls)
>     CLS_FOR_EACH_SAFE (fte, rule, cls) {
>         classifier_remove(cls, &fte->rule);
>         fte_free(fte);
> +        ovsrcu_quiesce();

I have an outstanding patch that changed the rte_free(fte) call to ovsrcu_postpone(rte_free, rte), which is technically required by the classifier API, but in this case, when there is only one thread using the classifier, that is not so critical. However, how about calling rte_free() after ovsrcu_quiesce(), as in the single-threaded case ovsrcu_quiesce() would call all the postponed functions before returning?

>     }
>     classifier_destroy(cls);
> }
> @@ -2375,6 +2377,7 @@ fte_insert(struct classifier *cls, const struct match *match,
>         cls_rule_destroy(&old->rule);
>         free(old);
>     }
> +    ovsrcu_quiesce();

Same here, how about calling free(old) only after ovsrcu_quiesce()?

> }
> 
> /* Reads the flows in 'filename' as flow table entries in 'cls' for the version
> -- 
> 2.1.1
>