With notes below:
Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
On Nov 6, 2014, at 10:03 AM, Ben Pfaff <b...@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 <jrajaha...@nicira.com>
> Signed-off-by: Ben Pfaff <b...@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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
