[ovs-dev] [PATCH v2 2/2] ovs-ofctl: Support multiple tables in replace-flows and diff-flows.

[Ben Pfaff]

On Wed, Nov 25, 2015 at 04:19:40PM -0800, Jarno Rajahalme wrote:
> 
> > On Nov 24, 2015, at 4:22 PM, Ben Pfaff <blp at ovn.org> wrote:
> > 
> > On Tue, Nov 24, 2015 at 10:21:41AM -0800, Jarno Rajahalme wrote:
> >> 
> >>> On Nov 24, 2015, at 10:15 AM, Jarno Rajahalme <jarno at ovn.org> wrote:
> >>> 
> >>> 
> >>>> On Nov 24, 2015, at 9:40 AM, Jarno Rajahalme <jarno at ovn.org> wrote:
> >>>> 
> >>>> 
> >>>>> On Nov 24, 2015, at 9:25 AM, Ben Pfaff <blp at ovn.org> wrote:
> >>>>> 
> >>>>> On Thu, Nov 19, 2015 at 01:33:18PM -0800, Jarno Rajahalme wrote:
> >>>>>> Currently ovs-ofctl replace-flows and diff-flows commands only support
> >>>>>> flows in table 0.  Extend this to cover all possible tables.
> >>>>>> 
> >>>>>> Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
> >>>>> 
> >>>>> There's one oddity that may deserve consideration.  It depends on how
> >>>>> careful we want to be.
> >>>>> 
> >>>>> OpenFlow 1.0 does not define a way to add a flow to a particular table.
> >>>>> The switch is responsible for deciding which table is most appropriate
> >>>>> for a given flow.  For example, a switch might have one table that
> >>>>> supports wildcards and another one that is exact-match (this is in fact
> >>>>> specifically envisioned by OF1.0 through its insistence that exact-match
> >>>>> flows have the highest priority).
> >>>>> 
> >>>>> This means that when talking to an OF1.0 switch, "ovs-ofctl
> >>>>> replace-flows" (and friends) should ignore the table number.  If
> >>>>> a flow on the switch is in table 1, but the input file says it is in
> >>>>> table 0 (probably because it doesn't specify a table at all), ovs-ofctl
> >>>>> should do nothing, because that's the desired state.
> >>>>> 
> >>>> 
> >>>> So for an OF1.0 switch without the Table ID extension we should ignore table numbers both ways, when reading from the file and when reading from the switch, essentially pretend that there is only one table?
> >>>> 
> >>>>> However, for practically forever, OVS has had special extensions to
> >>>>> allow control over the table in which a flow lives.  This means that if
> >>>>> ovs-ofctl is talking to OVS, even in OpenFlow 1.0, it should place flows
> >>>>> where the user requested and should not ignore the table numbers.
> >>>>> 
> >>>>> This distinction is reflected through ofputil_protocol values.  If a
> >>>>> switch supports OFPUTIL_P_OF10_STD_TID or OFPUTIL_P_OF10_NXM_TID, then
> >>>>> ovs-ofctl can place flows arbitrarily; if it only supports
> >>>>> OFPUTIL_P_OF10_STD (or, theoretically, only OFPUTIL_P_OF10_NXM), then it
> >>>>> is just a plain OF1.0 switch and all of the tables should be treated
> >>>>> alike.
> >>>>> 
> >>>>> OF1.1+ all support placing flows where the user requests.
> >>>>> 
> >>>>> It's probably not too hard to support this, and possibly it is
> >>>>> worthwhile.
> >>>>> 
> >>> 
> >>> IMO this could be cleaner if the choice of protocol is driven by the input file. If the file has any flow with a non-zero or non-all table number, then we restrict the choice of protocols to ones that support multiple tables. Sounds reasonable?
> >>> 
> >> 
> >> parse_ofp_str() already does this:
> >> 
> >>            if (!strcmp(name, "table")) {
> >>                error = str_to_u8(value, "table", &fm->table_id);
> >>                if (fm->table_id != 0xff) {
> >>                    *usable_protocols &= OFPUTIL_P_TID;
> >>                }
> >>            }
> >> 
> >> Here even “table=0” restricts vanilla OF1.0 out, which I think is the right thing to do.
> >> 
> >> So it turns out OF1.0 without table extension is already taken care of by restricting the choice of protocol.
> > 
> > Hmm.  Well, OK, we're no more wrong than we were before then.
> 
> Sounds like the dump-flows printing out table=0 for OF1.0 should be
> fixed in a separate patch? If so, you think this patch is ready to go?

I'm not sure that's the correct resolution but I do think it's
reasonable to resolve it separately.

Acked-by: Ben Pfaff <blp at ovn.org>