[ovs-dev] [PATCH ovn] ofctrl.c: Update installed OVS flow cookie when lflow is changed.

Mark Michelson mmichels at redhat.com
Mon Jan 13 20:45:09 UTC 2020


On 1/12/20 5:48 PM, Han Zhou wrote:
> On Sun, Jan 12, 2020 at 2:10 PM Han Zhou <hzhou at ovn.org> wrote:
>>
>> When an old lflow is replaced by a new lflow, if the OVS flows
>> translated by the old and new lflows have same match, ofctrl will
>> update existing OVS flow instead of deleting and adding a new one.
>>
>> However, when updating the existing flows, the cookie was not updated
>> by current implementation, which results in discrepency between lflows
>> and OVS flows, making debugging difficult and confuses tools such as
>> ovn-trace. This patch fixes it.
>>
>> Note: since command OFPFC_MODIFY_STRICT in FLOW_MOD message doesn't
>> support updating flow cookie after OpenFlow 1.1, this patch changes
>> to use OFPFC_ADD command, which effectively modifies existing flow
>> if a match is found.
>>
>> Signed-off-by: Han Zhou <hzhou at ovn.org>
>> ---
>>   controller/ofctrl.c | 12 ++++++++++--
>>   tests/ovn.at        | 36 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 46 insertions(+), 2 deletions(-)
>>
>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> index 10edd84..6f2c530 100644
>> --- a/controller/ofctrl.c
>> +++ b/controller/ofctrl.c
>> @@ -828,6 +828,7 @@ ovn_flow_to_string(const struct ovn_flow *f)
>>   {
>>       struct ds s = DS_EMPTY_INITIALIZER;
>>       ds_put_format(&s, "sb_uuid="UUID_FMT", ", UUID_ARGS(&f->sb_uuid));
>> +    ds_put_format(&s, "cookie=%"PRIx64", ", f->cookie);
>>       ds_put_format(&s, "table_id=%"PRIu8", ", f->table_id);
>>       ds_put_format(&s, "priority=%"PRIu16", ", f->priority);
>>       minimatch_format(&f->match, NULL, NULL, &s, OFP_DEFAULT_PRIORITY);
>> @@ -1176,7 +1177,8 @@ ofctrl_put(struct ovn_desired_flow_table
> *flow_table,
>>                   i->sb_uuid = d->sb_uuid;
>>               }
>>               if (!ofpacts_equal(i->ofpacts, i->ofpacts_len,
>> -                               d->ofpacts, d->ofpacts_len)) {
>> +                               d->ofpacts, d->ofpacts_len) ||
>> +                i->cookie != d->cookie) {
>>                   /* Update actions in installed flow. */
>>                   struct ofputil_flow_mod fm = {
>>                       .match = i->match,
>> @@ -1184,8 +1186,14 @@ ofctrl_put(struct ovn_desired_flow_table
> *flow_table,
>>                       .table_id = i->table_id,
>>                       .ofpacts = d->ofpacts,
>>                       .ofpacts_len = d->ofpacts_len,
>> -                    .command = OFPFC_MODIFY_STRICT,
>> +                    .command = OFPFC_ADD,
>>                   };
>> +                /* Update cookie if it is changed. */
>> +                if (i->cookie != d->cookie) {
>> +                    fm.modify_cookie = true;
>> +                    fm.new_cookie = htonll(d->cookie);
>> +                    i->cookie = d->cookie;
>> +                }
>>                   add_flow_mod(&fm, &msgs);
>>                   ovn_flow_log(i, "updating installed");
>>
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 411b768..adb677c 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -17338,3 +17338,39 @@ OVS_WAIT_UNTIL([
>>
>>   OVN_CLEANUP([hv1])
>>   AT_CLEANUP
>> +
>> +AT_SETUP([ovn -- trace when flow cookie updated])
>> +AT_KEYWORDS([cookie])
>> +ovn_start
>> +
>> +net_add n1
>> +sim_add hv1
>> +as hv1
>> +ovs-vsctl add-br br-phys
>> +ovn_attach n1 br-phys 192.168.0.1
>> +ovs-vsctl add-port br-int vif1 -- \
>> +    set interface vif1 external-ids:iface-id=lp1 ofport-request=1
>> +
>> +ovn-nbctl ls-add lsw0
>> +ovn-nbctl lsp-add lsw0 lp1
>> +ovn-nbctl lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.1"
>> +ovn-nbctl acl-add lsw0 from-lport 1000 'eth.type == 0x1234' drop
>> +
>> +ovn-nbctl --wait=hv --timeout=3 sync
>> +
>> +# Trace with --ovs should see ovs flow related to the ACL
>> +AT_CHECK([ovn-trace --ovs lsw0 'inport == "lp1" && eth.type == 0x1234' |
> grep "dl_type=0x1234" | grep "cookie"], [0], [ignore])
>> +
>> +# Replace the ACL with same match but different action.
>> +ovn-nbctl acl-del lsw0 -- \
>> +    acl-add lsw0 from-lport 1000 'eth.type == 0x1234' allow
>> +
>> +ovn-nbctl --wait=hv --timeout=3 sync
>> +
>> +# Trace with --ovs should still see the ovs flow related to the ACL,
> which
>> +# means the OVS flow is updated with new cookie corresponding to the new
> lflow.
>> +AT_CHECK([ovn-trace --ovs lsw0 'inport == "lp1" && eth.type == 0x1234' |
> grep "dl_type=0x1234 actions="], [0], [ignore])
>> +
>> +OVN_CLEANUP([hv1])
>> +
>> +AT_CLEANUP
>> --
>> 2.1.0
>>
> 
> CC Ben
> 
> Hi Ben,
> 
> I need your help to confirm that it is ok to replace OFPFC_MODIFY_STRICT
> with OFPFC_ADD for flow update, so that flow cookies can be updated. It
> seems there is no other way to update cookie after OF1.1.
> Although I see this in Documentation/topics/design.rst, which mentioned
> that NXM supports updating cookie with OFPFC_MODIFY_STRICT when cookie is
> not 'UNIT64_MAX'.
> ---------------
> The following table shows the handling of different protocols when receiving
> ``OFPFC_MODIFY`` and ``OFPFC_MODIFY_STRICT`` messages.  A mask of 0
> indicates
> either an explicit mask of zero or an implicit one by not specifying the
> ``NXM_NX_COOKIE(_W)`` field.
> 
> ==============  ======  ======  =============  =============
>                  Match   Update   Add on miss    Add on miss
>                  cookie  cookie     mask!=0        mask==0
> ==============  ======  ======  =============  =============
> OpenFlow 1.0      no     yes    (add on miss)  (add on miss)
> OpenFlow 1.1     yes      no         no             yes
> OpenFlow 1.2     yes      no         no             no
> NXM              yes     yes\*       no             yes
> ==============  ======  ======  =============  =============
> 
> \* Updates the flow's cookie unless the ``cookie`` field is ``UINT64_MAX``.
> ---------------
> However, it didn't work when I try using OFPFC_MODIFY_STRICT, even if I set
> fm.cookie = <new cookie> and fm.cookie_mask = UINT64_MAX. So I had to
> change it to OFPFC_ADD to make it work.

Hi Han, I decided to look into this, and according to the OpenFlow 
protocol version 1.1.0 up through 1.5.0, the following language exists 
in some section:

"For modify requests (OFPFC_MODIFY or OFPFC_MODIFY_STRICT), if a 
matching entry exists in the table, the instructions field of this entry 
is updated with the value from the request, whereas its
cookie, idle_timeout, hard_timeout, flags, counters and duration fields 
are left unchanged."

In OpenFlow 1.1.0, the section ends with:

"For modify requests, if no flow currently residing in the requested 
table matches the request, and if the cookie_mask field contains 0, the 
modify acts like an add, and the new flow entry must be inserted with 
zeroed counters"

However, in OpenFlow 1.2.0 onwards, the section ends with:

"For modify requests, if no flow entry currently residing in the 
requested table matches the request, no error is recorded, and no flow 
table modification occurs."

So it appears that with 1.1.0, modification with a zeroed cookie mask 
resulted in the equivalent OFPFC_ADD. In 1.2.0 onwards this is not the case.

> 
> I am not familiar with OF standard and its implementation in OVS, but the
> change worked well with my tests. Could you help explain what's the side
> effect of using OFPFC_ADD instead of OFPFC_MODIFY_STRICT?

 From an OpenFlow point of view, this will reset statistics on the flow 
(idle time, packet count, etc.) but otherwise has no negative effects. 
 From an OVS point of view, I'm not sure how it is different from flow 
modification. I would suspect that both might result in invalidation of 
cached megaflows though.

> 
> Thanks,
> Han
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list