| Message ID | 20200511094613.915169-1-numans@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Incremental processing improvements. | expand |
On 5/11/20 11:46 AM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > If ofctrl_check_and_add_flow(F') is called where flow F' has match-actions (M, A2) > and if there already exists a flow F with match-actions (M, A1) in the desired flow > table, then this function doesn't update the existing flow F with new actions > actions A2. > > This patch is required for the upcoming patch in this series which > adds incremental processing for OVS interface in the flow output stage. > Since we will be not be clearing the flow output data if these changes > are handled incrementally, some of the existing flows will be updated > with new actions. One such example would be flows in physical > table OFTABLE_LOCAL_OUTPUT (table 33). And this patch is required to > update such flows. Otherwise we will have incomplete actions installed. > > Signed-off-by: Numan Siddique <numans@ovn.org> Hi Numan, I think the title of the commit should be "ofctrl: Replace the actions of an existing flow if actions have changed." > --- > controller/ofctrl.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 4b51cd86e..8f2f13767 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -667,14 +667,23 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table, > > ovn_flow_log(f, "ofctrl_add_flow"); > > - if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) { > - if (log_duplicate_flow) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > - if (!VLOG_DROP_DBG(&rl)) { > - char *s = ovn_flow_to_string(f); > - VLOG_DBG("dropping duplicate flow: %s", s); > - free(s); > + struct ovn_flow *existing_f = > + ovn_flow_lookup(&flow_table->match_flow_table, f, true); > + if (existing_f) { > + if (ofpacts_equal(f->ofpacts, f->ofpacts_len, > + existing_f->ofpacts, existing_f->ofpacts_len)) { > + if (log_duplicate_flow) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > + if (!VLOG_DROP_DBG(&rl)) { > + char *s = ovn_flow_to_string(f); > + VLOG_DBG("dropping duplicate flow: %s", s); > + free(s); > + } > } > + } else { > + free(existing_f->ofpacts); > + existing_f->ofpacts = xmemdup(f->ofpacts, f->ofpacts_len); > + existing_f->ofpacts_len = f->ofpacts_len; We could avoid the free(existing_f->ofpacts) followed by xmemdup by swapping f->ofpacts with existing_f->ofpacts (same for ofpacts_len). We'd probably need a function for that and we'd have to call it in ofctrl_add_or_append_flow() too. I'm not sure if that makes the code harder to read though. What do you think? Thanks, Dumitru > } > ovn_flow_destroy(f); > return; >
On 5/11/20 5:46 AM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > If ofctrl_check_and_add_flow(F') is called where flow F' has match-actions (M, A2) > and if there already exists a flow F with match-actions (M, A1) in the desired flow > table, then this function doesn't update the existing flow F with new actions > actions A2. > > This patch is required for the upcoming patch in this series which > adds incremental processing for OVS interface in the flow output stage. > Since we will be not be clearing the flow output data if these changes > are handled incrementally, some of the existing flows will be updated > with new actions. One such example would be flows in physical > table OFTABLE_LOCAL_OUTPUT (table 33). And this patch is required to > update such flows. Otherwise we will have incomplete actions installed. I understand the explanation for this patch, but I'm wondering if this now makes it possible to do silly things like define ACLs with duplicate matches but different verdicts. Previously, if you did this, the first ACL would get installed, and you'd see a debug message about dropping the duplicate flow. With this change, whichever ACL is processed last wins and there's no debug message. Maybe we could store a value in the ovn_flow that indicates when the flow was added to the desired flow table. Perhaps each time the incremental engine runs, an int is incremented. This way, you could have logic like: existing_f = ovn_flow_lookup(); if (existing_f) { if (existing_f->age == f->age || ofpacts_equal(existing_f, f)) { // This is a duplicate flow } else { // replace existing_f's actions with f's actions } } > > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/ofctrl.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 4b51cd86e..8f2f13767 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -667,14 +667,23 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table, > > ovn_flow_log(f, "ofctrl_add_flow"); > > - if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) { > - if (log_duplicate_flow) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > - if (!VLOG_DROP_DBG(&rl)) { > - char *s = ovn_flow_to_string(f); > - VLOG_DBG("dropping duplicate flow: %s", s); > - free(s); > + struct ovn_flow *existing_f = > + ovn_flow_lookup(&flow_table->match_flow_table, f, true); > + if (existing_f) { > + if (ofpacts_equal(f->ofpacts, f->ofpacts_len, > + existing_f->ofpacts, existing_f->ofpacts_len)) { > + if (log_duplicate_flow) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > + if (!VLOG_DROP_DBG(&rl)) { > + char *s = ovn_flow_to_string(f); > + VLOG_DBG("dropping duplicate flow: %s", s); > + free(s); > + } > } > + } else { > + free(existing_f->ofpacts); > + existing_f->ofpacts = xmemdup(f->ofpacts, f->ofpacts_len); > + existing_f->ofpacts_len = f->ofpacts_len; > } > ovn_flow_destroy(f); > return; >
On Fri, May 15, 2020 at 6:50 AM Mark Michelson <mmichels@redhat.com> wrote: > On 5/11/20 5:46 AM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > If ofctrl_check_and_add_flow(F') is called where flow F' has > match-actions (M, A2) > > and if there already exists a flow F with match-actions (M, A1) in the > desired flow > > table, then this function doesn't update the existing flow F with new > actions > > actions A2. > > > > This patch is required for the upcoming patch in this series which > > adds incremental processing for OVS interface in the flow output stage. > > Since we will be not be clearing the flow output data if these changes > > are handled incrementally, some of the existing flows will be updated > > with new actions. One such example would be flows in physical > > table OFTABLE_LOCAL_OUTPUT (table 33). And this patch is required to > > update such flows. Otherwise we will have incomplete actions installed. > > I understand the explanation for this patch, but I'm wondering if this > now makes it possible to do silly things like define ACLs with duplicate > matches but different verdicts. Previously, if you did this, the first > ACL would get installed, and you'd see a debug message about dropping > the duplicate flow. With this change, whichever ACL is processed last > wins and there's no debug message. > > Maybe we could store a value in the ovn_flow that indicates when the > flow was added to the desired flow table. Perhaps each time the > incremental engine runs, an int is incremented. This way, you could have > logic like: > > existing_f = ovn_flow_lookup(); > if (existing_f) { > if (existing_f->age == f->age || ofpacts_equal(existing_f, f)) { > // This is a duplicate flow > } else { > // replace existing_f's actions with f's actions > } > } > > I don't think we really need to maintain an age param for this. I did some testing with the present master.. I added the below ACLs to a logical switch ovn-nbctl acl-add sw0 from-lport 1002 "ip4.src == 10.0.0.4" "drop" ovn-nbctl acl-add sw0 from-lport 1003 "ip4.src == 10.0.0.4" "allow" And the corresponding OF flows are: ** cookie=0xd23a2018, duration=6.057s, table=14, n_packets=0, n_bytes=0, priority=2002,ip,metadata=0x1,nw_src=10.0.0.4 actions=drop cookie=0x9bb3ead1, duration=3.218s, table=14, n_packets=0, n_bytes=0, priority=2003,ip,metadata=0x1,nw_src=10.0.0.4 actions=resubmit(,15) *** And then changed the second ACL with priority 1003 to 1002 (ovn-nbctl actl-add doesn't allow to add duplicate ACLs) ovn-nbctl set acl $acl_id priority=1002 After this I see the below flows: cookie=0x8515a20e, duration=15.873s, table=14, n_packets=0, n_bytes=0, priority=2002,ip,metadata=0x1,nw_src=10.0.0.4 actions=resubmit(,15) I was expecting to see a debug warning message in ovn-controller. But It doesn't show up. This happens because right now ovn_flow_lookup() is called here [1] with cmp_sb_uuid set to true. Since both the above ACLs will have different sb_uuid, ovn_flow_lookup() fails. I think we should pass 'cmp_sb_uuid' to false. I think we should consider a flow as duplicate (with the same match) and ignore it if the sb_uuid is different (as in the case of above ACLs) And if sb_uuid matches, then we should replace the existing actions and this is what this patch does. Examples of such flows are flows with MC_Flood, MC_Unknown. Thanks Numan > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > controller/ofctrl.c | 23 ++++++++++++++++------- > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > index 4b51cd86e..8f2f13767 100644 > > --- a/controller/ofctrl.c > > +++ b/controller/ofctrl.c > > @@ -667,14 +667,23 @@ ofctrl_check_and_add_flow(struct > ovn_desired_flow_table *flow_table, > > > > ovn_flow_log(f, "ofctrl_add_flow"); > > > > - if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) { > > - if (log_duplicate_flow) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, > 5); > > - if (!VLOG_DROP_DBG(&rl)) { > > - char *s = ovn_flow_to_string(f); > > - VLOG_DBG("dropping duplicate flow: %s", s); > > - free(s); > > + struct ovn_flow *existing_f = > > + ovn_flow_lookup(&flow_table->match_flow_table, f, true); > > + if (existing_f) { > > + if (ofpacts_equal(f->ofpacts, f->ofpacts_len, > > + existing_f->ofpacts, > existing_f->ofpacts_len)) { > > + if (log_duplicate_flow) { > > + static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 5); > > + if (!VLOG_DROP_DBG(&rl)) { > > + char *s = ovn_flow_to_string(f); > > + VLOG_DBG("dropping duplicate flow: %s", s); > > + free(s); > > + } > > } > > + } else { > > + free(existing_f->ofpacts); > > + existing_f->ofpacts = xmemdup(f->ofpacts, f->ofpacts_len); > > + existing_f->ofpacts_len = f->ofpacts_len; > > } > > ovn_flow_destroy(f); > > return; > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Wed, May 13, 2020 at 7:24 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 5/11/20 11:46 AM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > If ofctrl_check_and_add_flow(F') is called where flow F' has > match-actions (M, A2) > > and if there already exists a flow F with match-actions (M, A1) in the > desired flow > > table, then this function doesn't update the existing flow F with new > actions > > actions A2. > > > > This patch is required for the upcoming patch in this series which > > adds incremental processing for OVS interface in the flow output stage. > > Since we will be not be clearing the flow output data if these changes > > are handled incrementally, some of the existing flows will be updated > > with new actions. One such example would be flows in physical > > table OFTABLE_LOCAL_OUTPUT (table 33). And this patch is required to > > update such flows. Otherwise we will have incomplete actions installed. > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > Hi Numan, > > I think the title of the commit should be "ofctrl: Replace the actions > of an existing flow if actions have changed." > > > --- > > controller/ofctrl.c | 23 ++++++++++++++++------- > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > index 4b51cd86e..8f2f13767 100644 > > --- a/controller/ofctrl.c > > +++ b/controller/ofctrl.c > > @@ -667,14 +667,23 @@ ofctrl_check_and_add_flow(struct > ovn_desired_flow_table *flow_table, > > > > ovn_flow_log(f, "ofctrl_add_flow"); > > > > - if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) { > > - if (log_duplicate_flow) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, > 5); > > - if (!VLOG_DROP_DBG(&rl)) { > > - char *s = ovn_flow_to_string(f); > > - VLOG_DBG("dropping duplicate flow: %s", s); > > - free(s); > > + struct ovn_flow *existing_f = > > + ovn_flow_lookup(&flow_table->match_flow_table, f, true); > > + if (existing_f) { > > + if (ofpacts_equal(f->ofpacts, f->ofpacts_len, > > + existing_f->ofpacts, > existing_f->ofpacts_len)) { > > + if (log_duplicate_flow) { > > + static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 5); > > + if (!VLOG_DROP_DBG(&rl)) { > > + char *s = ovn_flow_to_string(f); > > + VLOG_DBG("dropping duplicate flow: %s", s); > > + free(s); > > + } > > } > > + } else { > > + free(existing_f->ofpacts); > > + existing_f->ofpacts = xmemdup(f->ofpacts, f->ofpacts_len); > > + existing_f->ofpacts_len = f->ofpacts_len; > > We could avoid the free(existing_f->ofpacts) followed by xmemdup by > swapping f->ofpacts with existing_f->ofpacts (same for ofpacts_len). > We'd probably need a function for that and we'd have to call it in > ofctrl_add_or_append_flow() too. > > Good idea. I'll do as suggested. But I don't think we can do the same for ofctrl_add_or_append_flow() as it appends the actions. Thanks Numan > I'm not sure if that makes the code harder to read though. What do you > think? > > Thanks, > Dumitru > > > } > > ovn_flow_destroy(f); > > return; > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 5/15/20 2:23 PM, Numan Siddique wrote: > > > On Wed, May 13, 2020 at 7:24 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: > > On 5/11/20 11:46 AM, numans@ovn.org <mailto:numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > If ofctrl_check_and_add_flow(F') is called where flow F' has > match-actions (M, A2) > > and if there already exists a flow F with match-actions (M, A1) in > the desired flow > > table, then this function doesn't update the existing flow F with > new actions > > actions A2. > > > > This patch is required for the upcoming patch in this series which > > adds incremental processing for OVS interface in the flow output > stage. > > Since we will be not be clearing the flow output data if these changes > > are handled incrementally, some of the existing flows will be updated > > with new actions. One such example would be flows in physical > > table OFTABLE_LOCAL_OUTPUT (table 33). And this patch is required to > > update such flows. Otherwise we will have incomplete actions > installed. > > > > Signed-off-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > Hi Numan, > > I think the title of the commit should be "ofctrl: Replace the actions > of an existing flow if actions have changed." > > > --- > > controller/ofctrl.c | 23 ++++++++++++++++------- > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > index 4b51cd86e..8f2f13767 100644 > > --- a/controller/ofctrl.c > > +++ b/controller/ofctrl.c > > @@ -667,14 +667,23 @@ ofctrl_check_and_add_flow(struct > ovn_desired_flow_table *flow_table, > > > > ovn_flow_log(f, "ofctrl_add_flow"); > > > > - if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) { > > - if (log_duplicate_flow) { > > - static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 5); > > - if (!VLOG_DROP_DBG(&rl)) { > > - char *s = ovn_flow_to_string(f); > > - VLOG_DBG("dropping duplicate flow: %s", s); > > - free(s); > > + struct ovn_flow *existing_f = > > + ovn_flow_lookup(&flow_table->match_flow_table, f, true); > > + if (existing_f) { > > + if (ofpacts_equal(f->ofpacts, f->ofpacts_len, > > + existing_f->ofpacts, > existing_f->ofpacts_len)) { > > + if (log_duplicate_flow) { > > + static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 5); > > + if (!VLOG_DROP_DBG(&rl)) { > > + char *s = ovn_flow_to_string(f); > > + VLOG_DBG("dropping duplicate flow: %s", s); > > + free(s); > > + } > > } > > + } else { > > + free(existing_f->ofpacts); > > + existing_f->ofpacts = xmemdup(f->ofpacts, > f->ofpacts_len); > > + existing_f->ofpacts_len = f->ofpacts_len; > > We could avoid the free(existing_f->ofpacts) followed by xmemdup by > swapping f->ofpacts with existing_f->ofpacts (same for ofpacts_len). > We'd probably need a function for that and we'd have to call it in > ofctrl_add_or_append_flow() too. > > > Good idea. I'll do as suggested. But I don't think we can do the same for > ofctrl_add_or_append_flow() as it appends the actions. > You're right, we can't do it in ofctrl_add_or_append_flow(), thanks for double checking. Regards, Dumitru
diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 4b51cd86e..8f2f13767 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -667,14 +667,23 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table, ovn_flow_log(f, "ofctrl_add_flow"); - if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) { - if (log_duplicate_flow) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); - if (!VLOG_DROP_DBG(&rl)) { - char *s = ovn_flow_to_string(f); - VLOG_DBG("dropping duplicate flow: %s", s); - free(s); + struct ovn_flow *existing_f = + ovn_flow_lookup(&flow_table->match_flow_table, f, true); + if (existing_f) { + if (ofpacts_equal(f->ofpacts, f->ofpacts_len, + existing_f->ofpacts, existing_f->ofpacts_len)) { + if (log_duplicate_flow) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); + if (!VLOG_DROP_DBG(&rl)) { + char *s = ovn_flow_to_string(f); + VLOG_DBG("dropping duplicate flow: %s", s); + free(s); + } } + } else { + free(existing_f->ofpacts); + existing_f->ofpacts = xmemdup(f->ofpacts, f->ofpacts_len); + existing_f->ofpacts_len = f->ofpacts_len; } ovn_flow_destroy(f); return;