> On May 29, 2015, at 4:28 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> On Mon, May 18, 2015 at 04:10:13PM -0700, Jarno Rajahalme wrote:
>> This makes it possible to tentatively add flows to the classifier
>> without the datapath seeing them.
>>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> This is nifty.
>
> Here are some suggestions as an incremental diff.
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 0cf42f6..d5ba6d7 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -318,11 +318,12 @@ cls_rule_is_catchall(const struct cls_rule *rule)
>
> /* Rules inserted during classifier_defer() need to be made visible before
> * calling classifier_publish().
> + *
> + * 'rule' must be in a classifier.
> */
> -void cls_rule_make_visible(const struct cls_rule *rule)
> +void
> +cls_rule_make_visible(const struct cls_rule *rule)
> {
> - ovs_assert(rule->cls_match); /* Must be in a classifier. */
> -
> rule->cls_match->visible = true;
> }
>
Done.
> diff --git a/lib/classifier.h b/lib/classifier.h
> index 77c4458..092ee46 100644
> --- a/lib/classifier.h
> +++ b/lib/classifier.h
> @@ -359,8 +359,34 @@ void cls_cursor_advance(struct cls_cursor *);
> #ifdef __cplusplus
> }
> #endif
> -
> +/* Deferred publication of new rules.
> + *
> + * When a new rule is added to a classifier, it can optionally be
> "invisible".
> + * That means that lookups won't find the rule, although iterations through
> + * the classifier will still see it.
> + *
> + * There are two reasons for new rules to be invisible:
> + *
> + * 1. Performance: Adding (or deleting) a rule can, in pathological
> cases,
> + * have a cost proportional to the number of rules already in the
> + * classifier. When multiple rules are being added (or deleted) in
> one
> + * go, though, this cost can be paid just once, not once per addition
> + * (or deletion), as long as it is OK for the new rules to be
> invisible
> + * until the batch change is complete.
> + *
> + * 2. Tentative additions: Invisibility allows a rule to be added
> + * tentatively, to possibly be modified or removed before it becomes
> + * visible.
> + *
> + * Classifier deletions are always visible to lookup and iteration
> immediately,
> + * but the performance advantages mentioned above apply to deletions too.
> + *
> + * To use deferred publication, first call classifier_defer(). Then, modify
> + * the classifier via additions and deletion. Call cls_rule_make_visible()
> on
> + * each new rule at an appropriate time. Finally, call classifier_publish().
> + */
> +
This is what I had in mind, but in the end (patch 17/19), it was cleaner to not
mix the versioning with the existing “deferring”.
I’ll add explanatory comments to the patch 17 instead.
> static inline void
> classifier_defer(struct classifier *cls)
> {
> @@ -373,4 +399,5 @@ classifier_publish(struct classifier *cls)
> cls->publish = true;
> pvector_publish(&cls->subtables);
> }
> +
> #endif /* classifier.h */
Do you want to see a v2 for these changes?
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
