On Fri, Jun 06, 2014 at 10:34:50AM +0100, Thomas Graf wrote:
> On 06/05/14 at 10:02pm, Ben Pfaff wrote:
> > Each of these functions had only a single caller, and breaking them out as
> > separate functions didn't seem to many the code clearer.
> >
> > I added a new function meter_insert_rule(): oftable_insert_rule() had used
> > members of struct meter directly, which worked because it was after the
> > definition of struct meter. So there was a choice to either move the
> > definition of struct meter earlier or add a helper function; I chose the
> > latter.
> >
> > Signed-off-by: Ben Pfaff <[email protected]>
> [...]
> > -
> > -static void
> > -do_add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
> > - const struct ofp_header *request, uint32_t buffer_id,
> > - struct rule *rule)
> > - OVS_REQUIRES(ofproto_mutex)
> > -{
> > - struct ofopgroup *group;
> > + if (fm->hard_timeout || fm->idle_timeout) {
> > + list_insert(&ofproto->expirable, &rule->expirable);
> > + }
> > + cookies_insert(ofproto, rule);
> > + eviction_group_add_rule(rule);
> > + if (actions->provider_meter_id != UINT32_MAX) {
> > + meter_insert_rule(rule);
> > + }
> > - oftable_insert_rule(rule);
> > + fat_rwlock_wrlock(&table->cls.rwlock);
> > + classifier_insert(&table->cls, CONST_CAST(struct cls_rule *,
> > &rule->cr));
> > + fat_rwlock_unlock(&table->cls.rwlock);
>
> I noticed the change in order of exposing the rule to the
> eviction group and inserting the meter rule plus inserting the
> classifier. Is that a problem?
I only changed it for cosmetic reasons, because I thought that the
code "looked better" that way, so that's a good question.
I do not think that the order matters because both of these are used
only with ofproto_mutex held:
* The list of meter rules is mainly used to determine which
rules must be deleted when a meter is deleted, which only
happens when ofproto_mutex is held.
(I see that handle_meter_request() breaks this rule. We
shall have to fix this. Also we ought to annotate 'rules'
with OVS_GUARDED_BY.)
* The eviction groups are only used during flow_mods, which
hold ofproto_mutex anyway.
(We could use better annotations here too.)
> > +/* Finds the meter invoked by 'rule''s actions and adds 'rule' to the
> > meter's
> > + * list of rules. */
> > +static void
> > +meter_insert_rule(struct rule *rule)
> > +{
>
> Would it make sense to pass the rule_actions into the function
> directly?
I thought about that but decided to give the function a simpler
interface.
Thanks for the review!
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
