Wow, amazing, that makes sense. I also ran manual tests locally and everything seemed fine.
On Tue, Sep 19, 2017 at 6:22 PM, Russell Bryant <[email protected]> wrote: > On Tue, Sep 19, 2017 at 9:38 AM, Russell Bryant <[email protected]> wrote: > > On Mon, Sep 18, 2017 at 7:31 PM, Han Zhou <[email protected]> wrote: > >> Thanks Russell for the quick work! > >> > >> On Mon, Sep 18, 2017 at 8:24 AM, Russell Bryant <[email protected]> > wrote: > >> > >>> @@ -301,6 +305,22 @@ consider_logical_flow(struct controller_ctx *ctx, > >>> if (m->match.wc.masks.conj_id) { > >>> m->match.flow.conj_id += *conj_id_ofs; > >>> } > >>> + if (is_switch(ldp)) { > >>> + unsigned int reg_index > >>> + = (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) - > >>> MFF_REG0; > >>> + int64_t port_id = m->match.flow.regs[reg_index]; > >>> + if (port_id) { > >>> + int64_t dp_id = lflow->logical_datapath->tunnel_key; > >>> + char buf[16]; > >>> + snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id, > >>> port_id); > >>> + if (!sset_contains(local_lport_ids, buf)) { > >>> + //VLOG_INFO("Matching on port id %"PRId64" dp > >>> %"PRId64", is NOT local", port_id, dp_id); > >>> + continue; > >>> + } else { > >>> + //VLOG_INFO("Matching on port id %"PRId64" dp > >>> %"PRId64", is local", port_id, dp_id); > >>> + } > >>> + } > >>> + } > >>> if (!m->n) { > >>> ofctrl_add_flow(flow_table, ptable, lflow->priority, > >>> lflow->header_.uuid.parts[0], &m->match, > >>> &ofpacts); > >> > >> I remember the expr_parse_string() is one of the biggest cost in > >> ovn-controller, so I wonder would it be better to move the check for > >> local_lport_ids before the parse happens, i.e. check against logical > flows > >> instead of ovs flows? > > > > Yes, that may be better. This was just a quick fix for the memory > > consumption issue. There is also a small improvement to CPU usage in > > my basic testing. I created 1000 ports with ACLs that used a 1000 > > member address set. Only one port was local. It finished about 10% > > faster with the patch. > > > > Moving this to before the logical flow to OpenFlow translation would > > obviously be better, but it's not quite straight forward. It's easy > > if you make assumptions about what the expression looks like, but > > doing it in a general way seems just as complicated as the existing > > expression parser. > > > >> Acked-by: Han Zhou <[email protected]> > > > > Thanks! > > > > I think I'll do a bit more testing before applying this. I'm also not > > sure how far this should be backported. The memory usage is bad > > enough with the default OpenStack security groups on large networks > > that I tend to think this should go back to 2.8 and 2.7 as well. > > I ran this patch through OpenStack CI and it works there too. I > applied this to master, branch-2.8, and branch-2.7. > > I think the next piece that makes sense is looking at improving use of > conjunctive flows. > > -- > Russell Bryant > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
