It's a good idea.
There's one case, in collect_rules_strict(), where it's nice to avoid
the repeated overhead of converting the same match to a cls_rule
repeatedly, so I just added a wrapper classifier_find_match_exactly()
and used that in the other cases.
Thanks,
Ben.
On Mon, Aug 06, 2012 at 03:14:19PM -0700, Ethan Jackson wrote:
> Some of this would be a bit simpler if classifier_find_rule_exactly()
> took a match and a priority instead of a cls_rule. May make sense to
> insert that change into a new patch preceding this one.
>
> Looks good, thanks.
> Ethan
>
> On Fri, Jul 20, 2012 at 4:25 PM, Ben Pfaff <b...@nicira.com> wrote:
> > Until now, "struct cls_rule" didn't own any data outside its own memory
> > block. An upcoming commit will make "struct cls_rule" sometimes own blocks
> > of memory, so it needs "destroy" and to a lesser extent "clone" functions.
> > This commit adds these in advance, even though they are mostly no-ops, to
> > make it possible to separately review the memory management.
> >
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > lib/classifier.c | 17 +++++++++++++++++
> > lib/classifier.h | 2 ++
> > ofproto/ofproto.c | 22 +++++++++++++++++++---
> > tests/test-classifier.c | 46
> > ++++++++++++++++++++++++++++++++++------------
> > utilities/ovs-ofctl.c | 2 ++
> > 5 files changed, 74 insertions(+), 15 deletions(-)
> >
> > diff --git a/lib/classifier.c b/lib/classifier.c
> > index cb66295..a482666 100644
> > --- a/lib/classifier.c
> > +++ b/lib/classifier.c
> > @@ -69,6 +69,23 @@ cls_rule_init(struct cls_rule *rule,
> > rule->priority = priority;
> > }
> >
> > +/* Initializes 'dst' as a copy of 'src'. */
> > +void
> > +cls_rule_clone(struct cls_rule *dst, const struct cls_rule *src)
> > +{
> > + *dst = *src;
> > +}
> > +
> > +/* Frees memory referenced by 'rule'. Doesn't free 'rule' itself (it's
> > + * normally embedded into a larger structure).
> > + *
> > + * ('rule' must not currently be in a classifier.) */
> > +void
> > +cls_rule_destroy(struct cls_rule *rule OVS_UNUSED)
> > +{
> > + /* Nothing to do yet. */
> > +}
> > +
> > /* Returns true if 'a' and 'b' match the same packets at the same priority,
> > * false if they differ in some way. */
> > bool
> > diff --git a/lib/classifier.h b/lib/classifier.h
> > index 8a11f05..74f9211 100644
> > --- a/lib/classifier.h
> > +++ b/lib/classifier.h
> > @@ -70,6 +70,8 @@ struct cls_rule {
> >
> > void cls_rule_init(struct cls_rule *,
> > const struct match *, unsigned int priority);
> > +void cls_rule_clone(struct cls_rule *, const struct cls_rule *);
> > +void cls_rule_destroy(struct cls_rule *);
> >
> > bool cls_rule_equal(const struct cls_rule *, const struct cls_rule *);
> > uint32_t cls_rule_hash(const struct cls_rule *, uint32_t basis);
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index e55e89f..4cb56e2 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -1428,6 +1428,8 @@ ofproto_add_flow(struct ofproto *ofproto, const
> > struct match *match,
> > cls_rule_init(&cr, match, priority);
> > rule = rule_from_cls_rule(classifier_find_rule_exactly(
> > &ofproto->tables[0].cls, &cr));
> > + cls_rule_destroy(&cr);
> > +
> > if (!rule || !ofpacts_equal(rule->ofpacts, rule->ofpacts_len,
> > ofpacts, ofpacts_len)) {
> > struct ofputil_flow_mod fm;
> > @@ -1468,6 +1470,8 @@ ofproto_delete_flow(struct ofproto *ofproto,
> > cls_rule_init(&cr, target, priority);
> > rule = rule_from_cls_rule(classifier_find_rule_exactly(
> > &ofproto->tables[0].cls, &cr));
> > + cls_rule_destroy(&cr);
> > +
> > if (!rule) {
> > /* No such rule -> success. */
> > return true;
> > @@ -1904,6 +1908,7 @@ static void
> > ofproto_rule_destroy__(struct rule *rule)
> > {
> > if (rule) {
> > + cls_rule_destroy(&rule->cr);
> > free(rule->ofpacts);
> > rule->ofproto->ofproto_class->rule_dealloc(rule);
> > }
> > @@ -2456,7 +2461,8 @@ collect_rules_loose(struct ofproto *ofproto, uint8_t
> > table_id,
> > cls_cursor_init(&cursor, &table->cls, &cr);
> > CLS_CURSOR_FOR_EACH (rule, cr, &cursor) {
> > if (rule->pending) {
> > - return OFPROTO_POSTPONE;
> > + error = OFPROTO_POSTPONE;
> > + goto exit;
> > }
> > if (!ofproto_rule_is_hidden(rule)
> > && ofproto_rule_has_out_port(rule, out_port)
> > @@ -2465,7 +2471,10 @@ collect_rules_loose(struct ofproto *ofproto, uint8_t
> > table_id,
> > }
> > }
> > }
> > - return 0;
> > +
> > +exit:
> > + cls_rule_destroy(&cr);
> > + return error;
> > }
> >
> > /* Searches 'ofproto' for rules in table 'table_id' (or in all tables, if
> > @@ -2503,7 +2512,8 @@ collect_rules_strict(struct ofproto *ofproto, uint8_t
> > table_id,
> > &cr));
> > if (rule) {
> > if (rule->pending) {
> > - return OFPROTO_POSTPONE;
> > + error = OFPROTO_POSTPONE;
> > + goto exit;
> > }
> > if (!ofproto_rule_is_hidden(rule)
> > && ofproto_rule_has_out_port(rule, out_port)
> > @@ -2512,6 +2522,9 @@ collect_rules_strict(struct ofproto *ofproto, uint8_t
> > table_id,
> > }
> > }
> > }
> > +
> > +exit:
> > + cls_rule_destroy(&cr);
> > return 0;
> > }
> >
> > @@ -2907,6 +2920,7 @@ add_flow(struct ofproto *ofproto, struct ofconn
> > *ofconn,
> >
> > /* Serialize against pending deletion. */
> > if (is_flow_deletion_pending(ofproto, &cr, table - ofproto->tables)) {
> > + cls_rule_destroy(&rule->cr);
> > ofproto->ofproto_class->rule_dealloc(rule);
> > return OFPROTO_POSTPONE;
> > }
> > @@ -2914,6 +2928,7 @@ add_flow(struct ofproto *ofproto, struct ofconn
> > *ofconn,
> > /* Check for overlap, if requested. */
> > if (fm->flags & OFPFF_CHECK_OVERLAP
> > && classifier_rule_overlaps(&table->cls, &rule->cr)) {
> > + cls_rule_destroy(&rule->cr);
> > ofproto->ofproto_class->rule_dealloc(rule);
> > return OFPERR_OFPFMFC_OVERLAP;
> > }
> > @@ -3629,6 +3644,7 @@ ofproto_collect_ofmonitor_refresh_rules(const struct
> > ofmonitor *m,
> > ofproto_collect_ofmonitor_refresh_rule(m, rule, seqno, rules);
> > }
> > }
> > + cls_rule_destroy(&target);
> > }
> >
> > static void
> > diff --git a/tests/test-classifier.c b/tests/test-classifier.c
> > index f279bda..5e24dd2 100644
> > --- a/tests/test-classifier.c
> > +++ b/tests/test-classifier.c
> > @@ -95,6 +95,11 @@ test_rule_from_cls_rule(const struct cls_rule *rule)
> > return rule ? CONTAINER_OF(rule, struct test_rule, cls_rule) : NULL;
> > }
> >
> > +static struct test_rule *make_rule(int wc_fields, unsigned int priority,
> > + int value_pat);
> > +static void free_rule(struct test_rule *);
> > +static struct test_rule *clone_rule(const struct test_rule *);
> > +
> > /* Trivial (linear) classifier. */
> > struct tcls {
> > size_t n_rules;
> > @@ -138,8 +143,8 @@ tcls_insert(struct tcls *tcls, const struct test_rule
> > *rule)
> > const struct cls_rule *pos = &tcls->rules[i]->cls_rule;
> > if (cls_rule_equal(pos, &rule->cls_rule)) {
> > /* Exact match. */
> > - free(tcls->rules[i]);
> > - tcls->rules[i] = xmemdup(rule, sizeof *rule);
> > + free_rule(tcls->rules[i]);
> > + tcls->rules[i] = clone_rule(rule);
> > return tcls->rules[i];
> > } else if (pos->priority < rule->cls_rule.priority) {
> > break;
> > @@ -154,7 +159,7 @@ tcls_insert(struct tcls *tcls, const struct test_rule
> > *rule)
> > memmove(&tcls->rules[i + 1], &tcls->rules[i],
> > sizeof *tcls->rules * (tcls->n_rules - i));
> > }
> > - tcls->rules[i] = xmemdup(rule, sizeof *rule);
> > + tcls->rules[i] = clone_rule(rule);
> > tcls->n_rules++;
> > return tcls->rules[i];
> > }
> > @@ -422,7 +427,7 @@ destroy_classifier(struct classifier *cls)
> > cls_cursor_init(&cursor, cls, NULL);
> > CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cls_rule, &cursor) {
> > classifier_remove(cls, &rule->cls_rule);
> > - free(rule);
> > + free_rule(rule);
> > }
> > classifier_destroy(cls);
> > }
> > @@ -522,6 +527,24 @@ make_rule(int wc_fields, unsigned int priority, int
> > value_pat)
> > return rule;
> > }
> >
> > +static struct test_rule *
> > +clone_rule(const struct test_rule *src)
> > +{
> > + struct test_rule *dst;
> > +
> > + dst = xmalloc(sizeof *dst);
> > + dst->aux = src->aux;
> > + cls_rule_clone(&dst->cls_rule, &src->cls_rule);
> > + return dst;
> > +}
> > +
> > +static void
> > +free_rule(struct test_rule *rule)
> > +{
> > + cls_rule_destroy(&rule->cls_rule);
> > + free(rule);
> > +}
> > +
> > static void
> > shuffle(unsigned int *p, size_t n)
> > {
> > @@ -584,7 +607,7 @@ test_single_rule(int argc OVS_UNUSED, char *argv[]
> > OVS_UNUSED)
> > assert(tcls_is_empty(&tcls));
> > compare_classifiers(&cls, &tcls);
> >
> > - free(rule);
> > + free_rule(rule);
> > classifier_destroy(&cls);
> > tcls_destroy(&tcls);
> > }
> > @@ -619,7 +642,7 @@ test_rule_replacement(int argc OVS_UNUSED, char *argv[]
> > OVS_UNUSED)
> > tcls_insert(&tcls, rule2);
> > assert(test_rule_from_cls_rule(
> > classifier_replace(&cls, &rule2->cls_rule)) == rule1);
> > - free(rule1);
> > + free_rule(rule1);
> > check_tables(&cls, 1, 1, 0);
> > compare_classifiers(&cls, &tcls);
> > tcls_destroy(&tcls);
> > @@ -760,7 +783,7 @@ test_many_rules_in_one_list (int argc OVS_UNUSED, char
> > *argv[] OVS_UNUSED)
> > tcls_destroy(&tcls);
> >
> > for (i = 0; i < N_RULES; i++) {
> > - free(rules[i]);
> > + free_rule(rules[i]);
> > }
> > } while (next_permutation(ops, ARRAY_SIZE(ops)));
> > assert(n_permutations == (factorial(N_RULES * 2) >> N_RULES));
> > @@ -838,7 +861,7 @@ test_many_rules_in_one_table(int argc OVS_UNUSED, char
> > *argv[] OVS_UNUSED)
> > for (i = 0; i < N_RULES; i++) {
> > tcls_remove(&tcls, tcls_rules[i]);
> > classifier_remove(&cls, &rules[i]->cls_rule);
> > - free(rules[i]);
> > + free_rule(rules[i]);
> >
> > check_tables(&cls, i < N_RULES - 1, N_RULES - (i + 1), 0);
> > compare_classifiers(&cls, &tcls);
> > @@ -897,18 +920,17 @@ test_many_rules_in_n_tables(int n_tables)
> > struct test_rule *target;
> > struct cls_cursor cursor;
> >
> > - target = xmemdup(tcls.rules[rand() % tcls.n_rules],
> > - sizeof(struct test_rule));
> > + target = clone_rule(tcls.rules[rand() % tcls.n_rules]);
> >
> > cls_cursor_init(&cursor, &cls, &target->cls_rule);
> > CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cls_rule, &cursor) {
> > classifier_remove(&cls, &rule->cls_rule);
> > - free(rule);
> > + free_rule(rule);
> > }
> > tcls_delete_matches(&tcls, &target->cls_rule);
> > compare_classifiers(&cls, &tcls);
> > check_tables(&cls, -1, -1, -1);
> > - free(target);
> > + free_rule(target);
> > }
> >
> > destroy_classifier(&cls);
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > index 7295cef..93cd7ac 100644
> > --- a/utilities/ovs-ofctl.c
> > +++ b/utilities/ovs-ofctl.c
> > @@ -1778,6 +1778,7 @@ fte_free(struct fte *fte)
> > if (fte) {
> > fte_version_free(fte->versions[0]);
> > fte_version_free(fte->versions[1]);
> > + cls_rule_destroy(&fte->rule);
> > free(fte);
> > }
> > }
> > @@ -1816,6 +1817,7 @@ fte_insert(struct classifier *cls, const struct match
> > *match,
> > if (old) {
> > fte_version_free(old->versions[index]);
> > fte->versions[!index] = old->versions[!index];
> > + cls_rule_destroy(&old->rule);
> > free(old);
> > }
> > }
> > --
> > 1.7.2.5
> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
