Han Zhou <zhou...@gmail.com> wrote on 02/18/2016 12:10:09 PM:
> From: Han Zhou <zhou...@gmail.com>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: "dev@openvswitch.org" <dev@openvswitch.org>
> Date: 02/18/2016 12:10 PM
> Subject: Re: [ovs-dev] [PATCH v6 2/3] [ovn-controller] Make flow
> table persistent in ovn controller
>
>
>
> On Thu, Feb 18, 2016 at 6:52 AM, Ryan Moats <rmo...@us.ibm.com> wrote:
> >
> > 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
> Ryan, thanks for explain. What I meant is to rely on the delta
> reported by OVSDB_IDL_CHANGE_DELETE to do the job of removing old
> flows. But I need to study more on the "track" feature of ovsdb,
> maybe it is not capable of doing this yet?
Oh - without that clause, I was seeing errors in trying to access
the memory pointed to by the deleted row, and that could cause nasty
things to happen...
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
