[ovs-dev] [ovs-dev, v9, 03/10] Make flow table persistent in ovn controller

Ryan Moats rmoats at us.ibm.com
Wed Mar 23 13:40:08 UTC 2016


Ben Pfaff <blp at ovn.org> wrote on 03/22/2016 04:54:16 PM:

> From: Ben Pfaff <blp at ovn.org>
> To: Ryan Moats/Omaha/IBM at IBMUS
> Cc: dev at openvswitch.org
> Date: 03/22/2016 04:54 PM
> Subject: Re: [ovs-dev,v9,03/10] Make flow table persistent in ovn
controller
>
> On Fri, Mar 11, 2016 at 03:06:18PM -0600, Ryan Moats wrote:
> > From: RYAN D. MOATS <rmoats at us.ibm.com>
> >
> > This is a prerequisite for incremental processing.
> >
> > Side effects:
> >
> > 1. Table rows are now tracked so that removed rows are correctly
> >    handled.
> > 2. Hash by table id+priority+action added to help detect superseded
> >    flows.
> > 3. Hash by insert seqno added to help find deleted flows.
> >
> > Signed-off-by: RYAN D. MOATS <rmoats at us.ibm.com>
>
> This had a number of patch rejects so I'm reviewing it on the basis of
> what I see, without trying to build or test it.  Therefore it's a pretty
> superficial review; I'll look at it more carefully when it is rebased.
>
> There's a number of instances of code similar to this, but I really
> haven't got a clue what it really does or where the number 4 comes from:
> > +        // this offset is to protect the hard coded rules in
physical.c
> > +        ins_seqno += 4;
>
> It looks like ofpacts_hash() could just be a wrapper around
> hash_bytes().
>
> It seems like the main change here is to make lflow_run() iterate over
> only the logical flows that have changed.  But I don't see how that can
> work as-is, because OpenFlow flows are not a pure function of the
> logical flows; rather, they have other inputs, such as the mapping from
> logical ports to OpenFlow port numbers.  I don't see anything here that
> makes sure that logical flows are revisited if this mapping changes.
> Maybe this is in a later patch in the series, but we ordinarily require
> each commit to be correct as a self-contained unit.

This is the first patch in the series that needs major rework as part of
the rebase and it was self-consistent when first submitted (in the sense
that it passed all of the unit tests) and it will be again before I send
an update.

As to the += 4 offset, this comes from the fact that when _TRACKED returns
a deleted row, you can't trust what is in the returned row as the memory
has already been freed). Therefore, all you have is the seqnos to go by.
Fortunately, the insert seqno is unique and constant, so we can use this
as a key for looking up the flow(s) that need to be removed - this is the
reason for all of the extra "seqno" hashmaps and related lookups.  However,
there are four OF flows added in physical.c that aren't driven by any ovnsb
rows (I refer to them as "hard-coded").  To avoid collisions, I assigned
these flows to insert seqno's 1, 2, 3, and 4 and then offset each row's
insert seqno received for a row by 4 to avoid collisions.

As to ofpacts_hash(), I wanted to use hash_bytes(), but I just couldn't
make it work reliably.  Since I'm rebasing this anyway, I'll give it
another pass.

Finally, this patch set doesn't iterate over only logical flows that
have changed - it still iterates over all of the logical flows during each
pass, so each flow is still revisited and updated as other inputs change.

Ryan (regXboi)



More information about the dev mailing list