[ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

[Russell Bryant]

On Mon, Sep 18, 2017 at 7:31 PM, Han Zhou <zhouhan at gmail.com> wrote:
> Thanks Russell for the quick work!
>
> On Mon, Sep 18, 2017 at 8:24 AM, Russell Bryant <russell at ovn.org> 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 <zhouhan at gmail.com>

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.

-- 
Russell Bryant