On Mon, Oct 28, 2019, 9:54 PM Numan Siddique <nusid...@redhat.com
<mailto:nusid...@redhat.com>> wrote:
On Mon, Oct 28, 2019, 9:29 PM Mark Michelson <mmich...@redhat.com
<mailto:mmich...@redhat.com>> wrote:
On 10/28/19 11:33 AM, Numan Siddique wrote:
> On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson
<mmich...@redhat.com <mailto:mmich...@redhat.com>> wrote:
>>
>> As stated in previous commits, conjunctive matches have an
issue where
>> it is possible to install multiple flows that have identical
matches.
>> This results in ambiguity, and can lead to features (such as
ACLs) not
>> functioning properly.
>>
>> This change fixes the problem by combining conjunctions with
identical
>> matches into a single flow. As an example, in the past we
may have had
>> something like:
>>
>> nw_dst=10.0.0.1 actions=conjunction(2,1/2)
>> nw_dst=10.0.0.1 actions=conjunction(3,1/2)
>>
>> This commit changes this into
>>
>> nw_dst=10.0.0.1 actions=conjunction(2,1/2),conjunction(3,1/2)
>>
>> This way, there is only a single flow with the proscribed
match, and
>> there is no ambiguity.
>>
>> Signed-off-by: Mark Michelson <mmich...@redhat.com
<mailto:mmich...@redhat.com>>
Acked-by: Numan Siddique <num...@ovn.org <mailto:num...@ovn.org>>
>> ---
>> controller/lflow.c | 5 ++--
>> controller/ofctrl.c | 73
+++++++++++++++++++++++++++++++++++++++++++++--------
>> controller/ofctrl.h | 6 +++++
>> tests/ovn.at <http://ovn.at> | 17 +++++--------
>> 4 files changed, 79 insertions(+), 22 deletions(-)
>>
>> diff --git a/controller/lflow.c b/controller/lflow.c
>> index e3ed20cd4..34b7c36a6 100644
>> --- a/controller/lflow.c
>> +++ b/controller/lflow.c
>> @@ -736,8 +736,9 @@ consider_logical_flow(
>> dst->clause = src->clause;
>> dst->n_clauses = src->n_clauses;
>> }
>> - ofctrl_add_flow(flow_table, ptable,
lflow->priority, 0, &m->match,
>> - &conj, &lflow->header_.uuid);
>> +
>> + ofctrl_add_or_append_flow(flow_table, ptable,
lflow->priority, 0,
>> + &m->match, &conj,
&lflow->header_.uuid);
>> ofpbuf_uninit(&conj);
>> }
>> }
>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> index 3131baff0..afb0036f1 100644
>> --- a/controller/ofctrl.c
>> +++ b/controller/ofctrl.c
>> @@ -69,6 +69,11 @@ struct ovn_flow {
>> uint64_t cookie;
>> };
>>
>> +static struct ovn_flow *ovn_flow_alloc(uint8_t table_id,
uint16_t priority,
>> + uint64_t cookie,
>> + const struct match
*match,
>> + const struct ofpbuf
*actions,
>> + const struct uuid
*sb_uuid);
>> static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
>> static struct ovn_flow *ovn_flow_lookup(struct hmap
*flow_table,
>> const struct
ovn_flow *target,
>> @@ -657,16 +662,8 @@ ofctrl_check_and_add_flow(struct
ovn_desired_flow_table *flow_table,
>> const struct uuid *sb_uuid,
>> bool log_duplicate_flow)
>> {
>> - struct ovn_flow *f = xmalloc(sizeof *f);
>> - f->table_id = table_id;
>> - f->priority = priority;
>> - minimatch_init(&f->match, match);
>> - f->ofpacts = xmemdup(actions->data, actions->size);
>> - f->ofpacts_len = actions->size;
>> - f->sb_uuid = *sb_uuid;
>> - f->match_hmap_node.hash = ovn_flow_match_hash(f);
>> - f->uuid_hindex_node.hash = uuid_hash(&f->sb_uuid);
>> - f->cookie = cookie;
>> + struct ovn_flow *f = ovn_flow_alloc(table_id, priority,
cookie, match,
>> + actions, sb_uuid);
>>
>> ovn_flow_log(f, "ofctrl_add_flow");
>>
>> @@ -721,9 +718,65 @@ ofctrl_add_flow(struct
ovn_desired_flow_table *desired_flows,
>> ofctrl_check_and_add_flow(desired_flows, table_id,
priority, cookie,
>> match, actions, sb_uuid, true);
>> }
>> +
>> +void
>> +ofctrl_add_or_append_flow(struct ovn_desired_flow_table
*desired_flows,
>> + uint8_t table_id, uint16_t
priority, uint64_t cookie,
>> + const struct match *match,
>> + const struct ofpbuf *actions,
>> + const struct uuid *sb_uuid)
>> +{
>> + struct ovn_flow *f = ovn_flow_alloc(table_id, priority,
cookie, match,
>> + actions, sb_uuid);
>> +
>> + ovn_flow_log(f, "ofctrl_add_or_append_flow");
>> +
>> + struct ovn_flow *existing;
>> + existing =
ovn_flow_lookup(&desired_flows->match_flow_table, f, false);
>> + if (existing) {
>> + /* There's already a flow with this particular
match. Append the
>> + * action to that flow rather than adding a new flow
>> + */
>> + uint64_t compound_stub[64 / 8];
>> + struct ofpbuf compound;
>> + ofpbuf_use_stub(&compound, compound_stub,
sizeof(compound_stub));
>> + ofpbuf_put(&compound, existing->ofpacts,
existing->ofpacts_len);
>> + ofpbuf_put(&compound, f->ofpacts, f->ofpacts_len);
>
> Instead of making use of a stub and copying the existing and new
> actions, can't we just
> copy the new actions to "existing->ofpacts" using ofpbuf_put() ?
You can't use ofpbuf_put() directly since existing->ofpacts is
of type
ofpact, not ofpbuf.
Oops. Please ignore my comment. I thought existing->ofpacts is of
type ofpbuf.
Sorry for the noise.
Numan
And I don't think you can cast existing->ofpacts to
an ofpbuf since existing->ofpacts was created from the 'data'
member of
an ofpbuf; we don't have the metadata, such as the method by
which the
data was allocated.
So maybe it's possible to create a new ofpbuf but have it use the
existing->ofpacts buffer? I was looking and here are the issues:
1) ofpbuf_clone_data() creates a copy of the data passed in
rather than
using it directly. So it still requires freeing
existing->ofpacts and
reassigning it.
2) ofpbuf_use_stub() would result in overwriting the data passed
into it
unless we adjust the ofpbuf's size so that it points past the
end of the
data we passed in. I can't find any example of this being done
in OVS.
If you have a good idea on how to re-use existing->ofpacts, then
I'll
happily do it.
>
> ofpbuf_put() will take care of reallocating the memory if
required.
>
> Other than that, this and the 1st patch of this series, looks
good to me.
>
> Thanks
> Numan
>
>> +
>> + free(existing->ofpacts);
>> + existing->ofpacts = xmemdup(compound.data,
compound.size);
>> + existing->ofpacts_len = compound.size;
>> +
>> + ovn_flow_destroy(f);
>> + } else {
>> + hmap_insert(&desired_flows->match_flow_table,
&f->match_hmap_node,
>> + f->match_hmap_node.hash);
>> + hindex_insert(&desired_flows->uuid_flow_table,
&f->uuid_hindex_node,
>> + f->uuid_hindex_node.hash);
>> + }
>> +}
>>
>> /* ovn_flow. */
>>
>> +static struct ovn_flow *
>> +ovn_flow_alloc(uint8_t table_id, uint16_t priority,
uint64_t cookie,
>> + const struct match *match, const struct
ofpbuf *actions,
>> + const struct uuid *sb_uuid)
>> +{
>> + struct ovn_flow *f = xmalloc(sizeof *f);
>> + f->table_id = table_id;
>> + f->priority = priority;
>> + minimatch_init(&f->match, match);
>> + f->ofpacts = xmemdup(actions->data, actions->size);
>> + f->ofpacts_len = actions->size;
>> + f->sb_uuid = *sb_uuid;
>> + f->match_hmap_node.hash = ovn_flow_match_hash(f);
>> + f->uuid_hindex_node.hash = uuid_hash(&f->sb_uuid);
>> + f->cookie = cookie;
>> +
>> + return f;
>> +}
>> +
>> /* Returns a hash of the match key in 'f'. */
>> static uint32_t
>> ovn_flow_match_hash(const struct ovn_flow *f)
>> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
>> index 1e9ac16b9..21d2ce648 100644
>> --- a/controller/ofctrl.h
>> +++ b/controller/ofctrl.h
>> @@ -70,6 +70,12 @@ void ofctrl_add_flow(struct
ovn_desired_flow_table *, uint8_t table_id,
>> const struct match *, const struct
ofpbuf *ofpacts,
>> const struct uuid *);
>>
>> +void ofctrl_add_or_append_flow(struct
ovn_desired_flow_table *desired_flows,
>> + uint8_t table_id, uint16_t
priority,
>> + uint64_t cookie, const
struct match *match,
>> + const struct ofpbuf *actions,
>> + const struct uuid *sb_uuid);
>> +
>> void ofctrl_remove_flows(struct ovn_desired_flow_table *,
const struct uuid *);
>>
>> void ovn_desired_flow_table_init(struct
ovn_desired_flow_table *);
>> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at
<http://ovn.at>
>> index 641a646fc..50d8efeec 100644
>> --- a/tests/ovn.at <http://ovn.at>
>> +++ b/tests/ovn.at <http://ovn.at>
>> @@ -12247,7 +12247,7 @@ ovn-nbctl create Address_Set name=set1 \
>> addresses=\"10.0.0.4\",\"10.0.0.5\",\"10.0.0.6\"
>> ovn-nbctl create Address_Set name=set2 \
>> addresses=\"10.0.0.7\",\"10.0.0.8\",\"10.0.0.9\"
>> -ovn-nbctl acl-add ls1 to-lport 1002 \
>> +ovn-nbctl acl-add ls1 to-lport 1001 \
>> 'ip4 && ip4.src == $set1 && ip4.dst == $set1' allow
>> ovn-nbctl acl-add ls1 to-lport 1001 \
>> 'ip4 && ip4.src == $set1 && ip4.dst == $set2' drop
>> @@ -12296,7 +12296,7 @@ cat 2.expected > expout
>> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in
<http://ovs-pcap.in>" hv1/vif2-tx.pcap > 2.packets
>> AT_CHECK([cat 2.packets], [0], [expout])
>>
>> -# There should be total of 12 flows present with
conjunction action and 2 flows
>> +# There should be total of 9 flows present with conjunction
action and 2 flows
>> # with conj match. Eg.
>> # table=44, priority=2002,conj_id=2,metadata=0x1
actions=resubmit(,45)
>> # table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
>> @@ -12306,14 +12306,11 @@ AT_CHECK([cat 2.packets], [0],
[expout])
>> # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7
actions=conjunction(3,2/2)
>> # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
actions=conjunction(3,2/2)
>> # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8
actions=conjunction(3,2/2)
>> -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.6
actions=conjunction(2,1/2)
>> -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.4
actions=conjunction(2,1/2)
>> -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.5
actions=conjunction(2,1/2)
>> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
actions=conjunction(3,1/2)
>> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4
actions=conjunction(3,1/2)
>> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5
actions=conjunction(3,1/2)
>> -
>> -OVS_WAIT_UNTIL([test 12 = `as hv1 ovs-ofctl dump-flows
br-int | \
>> +# priority=2002,ip,metadata=0x1,nw_src=10.0.0.6
actions=conjunction(2,1/2),conjunction(3,1/2)
>> +# priority=2002,ip,metadata=0x1,nw_src=10.0.0.4
actions=conjunction(2,1/2),conjunction(3,1/2)
>> +# priority=2002,ip,metadata=0x1,nw_src=10.0.0.5
actions=conjunction(2,1/2),conjunction(3,1/2)
>> +
>> +OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows
br-int | \
>> grep conjunction | wc -l`])
>> OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows
br-int | \
>> grep conj_id | wc -l`])
>> --
>> 2.14.5
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org <mailto:d...@openvswitch.org>
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
