Han Zhou <zhou...@gmail.com> wrote on 02/18/2016 01:34:03 AM:
[snipped for BW]
> > + /* loop through all the flows to see if there is an old flow to be
> > + * removed - do so if the old flow has the same priority,
> table, and match
> > + * but a different action or if the old flow has the same
> priority, table
> > + * and action, but the new match is either a superset or subset of
the
> > + * old match */
> > +
> > + struct ovn_flow *d, *next;
> > + HMAP_FOR_EACH_SAFE (d, next, hmap_node, desired_flows) {
> > + if (f->table_id == d->table_id && f->priority == d->priority)
{
> > + if ((match_equal(&f->match, &d->match)
> > + && !ofpacts_equal(f->ofpacts, f->ofpacts_len,
> > + d->ofpacts, d->ofpacts_len))
> > + || (ofpacts_equal(f->ofpacts, f->ofpacts_len,
> > + d->ofpacts, d->ofpacts_len)
> > + && ((flow_wildcards_has_extra
> (&f->match.wc,&d->match.wc)
> > + && flow_equal_except(&f->match.flow,
> &d->match.flow,
> > + &f->match.wc))
> > + || (flow_wildcards_has_extra
> (&d->match.wc,&f->match.wc)
> > + && flow_equal_except(&d->match.flow,
> > + &f->match.flow,
> > + &d->match.wc))))) {
> > + static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(5, 5);
> > + if (!VLOG_DROP_INFO(&rl)) {
> > + char *s = ovn_flow_to_string(d);
> > + VLOG_INFO("removing superceded flow: %s", s);
> > + free(s);
> > + }
> >
> > - ovn_flow_destroy(f);
> > - return;
> > + hmap_remove(desired_flows, &d->hmap_node);
> > + ovn_flow_destroy(d);
> > + }
> > +
> > + /* if this is a complete duplicate, remove the new flow */
> > + if (match_equal(&f->match, &d->match)
> > + && ofpacts_equal(f->ofpacts, f->ofpacts_len,
> > + d->ofpacts, d->ofpacts_len)) {
> > + static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(5, 5);
> > + if (!VLOG_DROP_INFO(&rl)) {
> > + char *s = ovn_flow_to_string(f);
> > + VLOG_INFO("dropping duplicate flow: %s", s);
> > + free(s);
> > + }
> > +
> > + ovn_flow_destroy(f);
> > + return;
> > + }
> > + }
> > }
> >
> Ryan, sorry that I don't quite understand the loop here. Could you
> explain why we need to examine existing flows one by one to identify
> the old flows to be removed? Shouldn't the ovsdb track deleted
> entries and then we can just delete whatever is informed by ovsdb?
> Otherwise, I wonder if we go with this approach, it maybe less
> efficient in circumstances when there are big changes in SB lflow
> table, e.g. during some fault-recovery when SB db are rebuilt.
>
> Maybe I missed some point here, correct me if I am wrong :)
Here's the deal - when we initially find a port, a desired OF flow
gets created without any port security (match regex is
/reg6=.,metadata=0x1/ IIRC). After we set port security via
ovn-nbctl, that OF flow gets created with a match pattern that
includes the mac address (match regex is
/reg6=.,metadata=0x1,dl_src=<mac address>/ IIRC). If the desired
OF flow table gets rebuilt each time, the first OF flow gets purged
from the switch after this change because it doesn't exist in the
rebuilt desired OF flow table. To persist the desired OF flow table
across cycles, we have to add the ability to detect and remove that
old desired flow by hand, otherwise both flows end up in the switch
and we lose port security.
Now, I absolutely admit that this may be less efficient in cases where
there are big changes, but I *believe* that this is far more efficient
in the "sunny day" case once we layer the part 3 patch of incremental
logical flow processing on to this (I say believe because I'm running
performance tests this morning to try and verify it).
Lastly, this wasn't my initial choice of how to do things, but I still
haven't thought of a way to add incremental logical flow processing
that doesn't require some memory of what OF flows have been asked for
during previous cycles and still works correctly.
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
