Re: [ovs-dev] [PATCH ovn] Exclude inport and outport symbol tables from conjunction
On Sat, Sep 14, 2019 at 2:41 AM Daniel Alvarez Sanchez
wrote:
> Acked-by: Daniel Alvarez
>
>
> On Fri, Sep 13, 2019 at 11:02 PM Mark Michelson
> wrote:
> >
> > Acked-by: Mark Michelson
> >
> > It sucks that we lose the efficiency of the conjunctive match altogether
> > on port groups because of this error, but I understand this is a huge
> > bug and needs fixing.
> If I'm not mistaken, from OpenStack standpoint conjunction was *only*
> being used when using port groups and ACLs that matched on port ranges
> ( e.g tcp.dst >= X && tcp.dst <=Y) which was not working. Therefore
> we're not losing performance because it was already broken (given that
> there was more than one ACL like that).
>
> >
> > Perhaps it would be good to start up a discussion on this list about a
> > more longterm solution that would allow for conjunctive matches with no
> > ambiguity.
> Agreed! We already discussed some ideas on IRC but it'd be awesome to
> have a thread and brainstorm there.
>
>
Thanks for the reviews. I applied this to master.
Agree we can discuss it further and come up with ideas.
I know Dumitru has some idea to make use of conjunctions for port groups.
CC'ing Han if he has any comments on ideas.
Thanks
Numan
> Thanks a lot everyone!
> Daniel
> >
> > On 9/13/19 4:49 PM, nusid...@redhat.com wrote:
> > > From: Numan Siddique
> > >
> > > If there are multiple ACLs associated with a port group and they
> > > match on a range of some field, then ovn-controller doesn't install
> > > the flows properly and this results in broken ACL functionality.
> > >
> > > For example, if there is a port group - pg1 with logical ports - [p1,
> p2]
> > > and if there are below ACLs (only match condition is shown)
> > >
> > > 1 - outport == @pg1 && ip4 && tcp.dst >= 500 && tcp.dst <= 501
> > > 2 - outport == @pg1 && ip4 && tcp.dst >= 600 && tcp.dst <= 601
> > >
> > > The first ACL will result in the below OF flows
> > >
> > > 1. conj_id=1,tcp
> > > 2. tcp,reg15=0x11: conjunction(1, 1/2)
> > > 3. tcp,reg15=0x12: conjunction(1, 1/2)
> > > 5. tcp,tp_dst=500: conjunction(1, 2/2)
> > > 6. tcp,tp_dst=501: conjunction(1, 2/2)
> > >
> > > The second ACL will result in the below OF flows
> > > 7. conj_id=2,tcp
> > > 8. tcp,reg15=0x11: conjunction(2, 1/2)
> > > 9. tcp,reg15=0x12: conjunction(2, 1/2)
> > > 11. tcp,tp_dst=600: conjunction(2, 2/2)
> > > 12. tcp,tp_dst=601: conjunction(2, 3/2)
> > >
> > > The OF flows (2) and (8) have the exact match but with different
> action.
> > > This results in only one of the flows getting installed. The same goes
> > > for the flows (3) and (9). And this completely breaks the ACL
> functionality
> > > for such scenarios.
> > >
> > > In order to fix this issue, this patch excludes the 'inport' and
> 'outport' symbols
> > > from conjunction. With this patch we will have the below flows.
> > >
> > > tcp,reg15=0x11,tp_dst=500
> > > tcp,reg15=0x11,tp_dst=501
> > > tcp,reg15=0x12,tp_dst=500
> > > tcp,reg15=0x12,tp_dst=501
> > > tcp,reg15=0x13,tp_dst=500
> > > tcp,reg15=0x13,tp_dst=501
> > > tcp,reg15=0x11,tp_dst=600
> > > tcp,reg15=0x11,tp_dst=601
> > > tcp,reg15=0x12,tp_dst=600
> > > tcp,reg15=0x12,tp_dst=601
> > > tcp,reg15=0x13,tp_dst=600
> > > tcp,reg15=0x13,tp_dst=601
> > >
> > > Signed-off-by: Numan Siddique
> > > ---
> > > lib/expr.c | 2 +-
> > > tests/ovn.at | 26 ++
> > > 2 files changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/expr.c b/lib/expr.c
> > > index e4c650f7c..c0871e1e8 100644
> > > --- a/lib/expr.c
> > > +++ b/lib/expr.c
> > > @@ -1499,7 +1499,7 @@ expr_symtab_add_string(struct shash *symtab,
> const char *name,
> > > const struct mf_field *field = mf_from_id(id);
> > > struct expr_symbol *symbol;
> > >
> > > -symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL,
> false,
> > > +symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL,
> true,
> > > field->writable);
> > > symbol->field = field;
> > > return symbol;
> > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > index 2a35b4e15..14d9f59b0 100644
> > > --- a/tests/ovn.at
> > > +++ b/tests/ovn.at
> > > @@ -589,6 +589,24 @@ ip,reg14=0x6
> > > ipv6,reg14=0x5
> > > ipv6,reg14=0x6
> > > ])
> > > +AT_CHECK([expr_to_flow 'inport == {"eth0", "eth1", "eth2"} && ip4 &&
> tcp && tcp.dst == {500, 501}'], [0], [dnl
> > > +tcp,reg14=0x5,tp_dst=500
> > > +tcp,reg14=0x5,tp_dst=501
> > > +tcp,reg14=0x6,tp_dst=500
> > > +tcp,reg14=0x6,tp_dst=501
> > > +])
> > > +AT_CHECK([expr_to_flow 'outport == {"eth0", "eth1", "eth2"} && ip4 &&
> tcp && tcp.src == {400, 401} && tcp.dst == {500, 501}'], [0], [dnl
> > > +conj_id=1,tcp,reg15=0x5
> > > +conj_id=2,tcp,reg15=0x6
> > > +tcp,reg15=0x5,tp_dst=500: conjunction(1, 0/2)
> > > +tcp,reg15=0x5,tp_dst=501: conjunction(1, 0/2)
> > > +tcp,reg15=0x5,tp_src=400: conjunction(1, 1/2)
> > > +tcp,reg15=0x5,tp_src=401: conjunction(1, 1/2)
> > > +tcp,reg15=0x6,tp_dst=500: conjunction(2,
[ovs-dev] [branch 2.12] ovn: Exclude inport and outport symbol tables from conjunction
From: Numan Siddique
If there are multiple ACLs associated with a port group and they
match on a range of some field, then ovn-controller doesn't install
the flows properly and this results in broken ACL functionality.
For example, if there is a port group - pg1 with logical ports - [p1, p2]
and if there are below ACLs (only match condition is shown)
1 - outport == @pg1 && ip4 && tcp.dst >= 500 && tcp.dst <= 501
2 - outport == @pg1 && ip4 && tcp.dst >= 600 && tcp.dst <= 601
The first ACL will result in the below OF flows
1. conj_id=1,tcp
2. tcp,reg15=0x11: conjunction(1, 1/2)
3. tcp,reg15=0x12: conjunction(1, 1/2)
5. tcp,tp_dst=500: conjunction(1, 2/2)
6. tcp,tp_dst=501: conjunction(1, 2/2)
The second ACL will result in the below OF flows
7. conj_id=2,tcp
8. tcp,reg15=0x11: conjunction(2, 1/2)
9. tcp,reg15=0x12: conjunction(2, 1/2)
11. tcp,tp_dst=600: conjunction(2, 2/2)
12. tcp,tp_dst=601: conjunction(2, 3/2)
The OF flows (2) and (8) have the exact match but with different action.
This results in only one of the flows getting installed. The same goes
for the flows (3) and (9). And this completely breaks the ACL functionality
for such scenarios.
In order to fix this issue, this patch excludes the 'inport' and 'outport'
symbols
from conjunction. With this patch we will have the below flows.
tcp,reg15=0x11,tp_dst=500
tcp,reg15=0x11,tp_dst=501
tcp,reg15=0x12,tp_dst=500
tcp,reg15=0x12,tp_dst=501
tcp,reg15=0x13,tp_dst=500
tcp,reg15=0x13,tp_dst=501
tcp,reg15=0x11,tp_dst=600
tcp,reg15=0x11,tp_dst=601
tcp,reg15=0x12,tp_dst=600
tcp,reg15=0x12,tp_dst=601
tcp,reg15=0x13,tp_dst=600
tcp,reg15=0x13,tp_dst=601
Acked-by: Mark Michelson
Acked-by: Daniel Alvarez
Signed-off-by: Numan Siddique
(cherry-picked from ovn commit 298701dbc99645700be41680a43d049cb061847a)
---
ovn/lib/expr.c | 2 +-
tests/ovn.at | 26 ++
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index e4c650f7c..c0871e1e8 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -1499,7 +1499,7 @@ expr_symtab_add_string(struct shash *symtab, const char
*name,
const struct mf_field *field = mf_from_id(id);
struct expr_symbol *symbol;
-symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, false,
+symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, true,
field->writable);
symbol->field = field;
return symbol;
diff --git a/tests/ovn.at b/tests/ovn.at
index 2361524ff..54aa19bb2 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -573,6 +573,24 @@ ip,reg14=0x6
ipv6,reg14=0x5
ipv6,reg14=0x6
])
+AT_CHECK([expr_to_flow 'inport == {"eth0", "eth1", "eth2"} && ip4 && tcp &&
tcp.dst == {500, 501}'], [0], [dnl
+tcp,reg14=0x5,tp_dst=500
+tcp,reg14=0x5,tp_dst=501
+tcp,reg14=0x6,tp_dst=500
+tcp,reg14=0x6,tp_dst=501
+])
+AT_CHECK([expr_to_flow 'outport == {"eth0", "eth1", "eth2"} && ip4 && tcp &&
tcp.src == {400, 401} && tcp.dst == {500, 501}'], [0], [dnl
+conj_id=1,tcp,reg15=0x5
+conj_id=2,tcp,reg15=0x6
+tcp,reg15=0x5,tp_dst=500: conjunction(1, 0/2)
+tcp,reg15=0x5,tp_dst=501: conjunction(1, 0/2)
+tcp,reg15=0x5,tp_src=400: conjunction(1, 1/2)
+tcp,reg15=0x5,tp_src=401: conjunction(1, 1/2)
+tcp,reg15=0x6,tp_dst=500: conjunction(2, 0/2)
+tcp,reg15=0x6,tp_dst=501: conjunction(2, 0/2)
+tcp,reg15=0x6,tp_src=400: conjunction(2, 1/2)
+tcp,reg15=0x6,tp_src=401: conjunction(2, 1/2)
+])
AT_CHECK([expr_to_flow 'inport == "eth0" && inport == "eth1"'], [0], [dnl
(no flows)
])
@@ -677,6 +695,14 @@ reg15=0x11
reg15=0x12
reg15=0x13
])
+AT_CHECK([expr_to_flow 'outport == @pg1 && ip4.src == {10.0.0.4, 10.0.0.5}'],
[0], [dnl
+ip,reg15=0x11,nw_src=10.0.0.4
+ip,reg15=0x11,nw_src=10.0.0.5
+ip,reg15=0x12,nw_src=10.0.0.4
+ip,reg15=0x12,nw_src=10.0.0.5
+ip,reg15=0x13,nw_src=10.0.0.4
+ip,reg15=0x13,nw_src=10.0.0.5
+])
AT_CHECK([expr_to_flow 'outport == {@pg_empty}'], [0], [dnl
(no flows)
])
--
2.21.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [branch 2.12] ovn: Exclude inport and outport symbol tables from conjunction
Bleep bloop. Greetings Numan Siddique, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: Failed to merge in the changes. Patch failed at 0001 ovn: Exclude inport and outport symbol tables from conjunction The copy of the patch that failed is found in: /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch When you have resolved this problem, run "git am --resolved". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Re reply
Hello Dear, Please I want you to partner with me to establish a Charity Organization in your country on my behalf and also on behalf of my Late husband. Thanks, Mrs Justina Bana. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 0/1] Balance-tcp bond mode optimization
> > > > I confirm a decent performance improvement with DPDK and balance-tcp > bonding: > > > > lb-output-action=false > > > > rx: 740 Mbps 1446 kpps > > > > lb-output-action=true > > > > rx: 860 Mbps 1680 kpps > > > > I'm running a very simple test with a tweaked version of testpmd which > > generates 256 L4 flows, I guess that with much flows the improvement > > is way higher. > > > > Tested-by: Matteo Croce > > Thank you Matteo for testing this patch. > I found an issue with the patch. It's not 100% reproducible, but sometimes > the > option gets enabled regardless of the bonding type and the configuration. > This breaks the unit tests, as the bond/show output is wrong: > > # ovs-vsctl add-br br0 > [63129.589500] device ovs-system entered promiscuous mode [63129.591802] > device br0 entered promiscuous mode > > # ovs-vsctl add-bond br0 bond dummy0 dummy1 -- set Port bond lacp=active > bond-mode=active-backup [63150.700542] device dummy1 entered > promiscuous mode [63150.700892] device dummy0 entered promiscuous mode > Let me check this in my setup. I always used 'netdev' bridges for testing my patch. May be I need to be check for data path support in the display function as well. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/3] ovs-check-dead-ifs: python3 print format
On Fri, Sep 13, 2019 at 01:29:01PM -0400, Aaron Conole wrote:
> The print call changed in python3, so update it.
>
> Signed-off-by: Aaron Conole
LGTM
Acked-by: William Tu
> ---
> utilities/ovs-check-dead-ifs.in | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/utilities/ovs-check-dead-ifs.in b/utilities/ovs-check-dead-ifs.in
> index ac54f6c9c..f398a3401 100755
> --- a/utilities/ovs-check-dead-ifs.in
> +++ b/utilities/ovs-check-dead-ifs.in
> @@ -37,7 +37,7 @@ for ifname in os.listdir("/sys/class/net"):
> except IOError:
> pass
> except ValueError:
> -print "%s: unexpected format\n" % fn
> +print("%s: unexpected format\n" % fn)
>
> # Get inodes for all packet sockets whose ifindexes don't exist.
> invalid_inodes = set()
> @@ -95,8 +95,8 @@ for pid in os.listdir("/proc"):
> bad_pids.add(pid)
>
> if bad_pids:
> -print """
> +print("""
> The following processes are listening for packets to arrive on network
> devices
> -that no longer exist. You may want to restart them."""
> +that no longer exist. You may want to restart them.""")
> sys.stdout.flush()
> os.execvp("ps", ["ps"] + ["%s" % pid for pid in bad_pids])
> --
> 2.21.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] ovs-check-dead-ifs: unshadow pid variable
On Fri, Sep 13, 2019 at 01:29:02PM -0400, Aaron Conole wrote:
> The pid variable is being shadowed by the list comprehension in the
> os.execvp() call. This can generate flakes / warnings in some environments
> so fix it.
>
> Signed-off-by: Aaron Conole
LGTM, tested on my machine and solve the flake8 error.
Acked-by: William Tu
> ---
> utilities/ovs-check-dead-ifs.in | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/utilities/ovs-check-dead-ifs.in b/utilities/ovs-check-dead-ifs.in
> index f398a3401..73e4fd9e1 100755
> --- a/utilities/ovs-check-dead-ifs.in
> +++ b/utilities/ovs-check-dead-ifs.in
> @@ -99,4 +99,4 @@ if bad_pids:
> The following processes are listening for packets to arrive on network
> devices
> that no longer exist. You may want to restart them.""")
> sys.stdout.flush()
> -os.execvp("ps", ["ps"] + ["%s" % pid for pid in bad_pids])
> +os.execvp("ps", ["ps"] + ["%s" % pspid for pspid in bad_pids])
> --
> 2.21.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/3] flake8: also check the ovs-check-dead-ifs script
On Fri, Sep 13, 2019 at 01:29:03PM -0400, Aaron Conole wrote: > Signed-off-by: Aaron Conole LGTM Acked-by: William Tu > --- > utilities/automake.mk | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/utilities/automake.mk b/utilities/automake.mk > index c379596fd..58f743e04 100644 > --- a/utilities/automake.mk > +++ b/utilities/automake.mk > @@ -156,6 +156,7 @@ endif > > FLAKE8_PYFILES += utilities/ovs-pcap.in \ > utilities/checkpatch.py utilities/ovs-dev.py \ > + utilities/ovs-check-dead-ifs.in \ > utilities/ovs-tcpdump.in \ > utilities/ovs-pipegen.py > > -- > 2.21.0 > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Exclude inport and outport symbol tables from conjunction
On Sat, Sep 14, 2019 at 12:40 AM Numan Siddique wrote:
>
>
>
> On Sat, Sep 14, 2019 at 2:41 AM Daniel Alvarez Sanchez <
dalva...@redhat.com> wrote:
>>
>> Acked-by: Daniel Alvarez
>>
>>
>> On Fri, Sep 13, 2019 at 11:02 PM Mark Michelson
wrote:
>> >
>> > Acked-by: Mark Michelson
>> >
>> > It sucks that we lose the efficiency of the conjunctive match
altogether
>> > on port groups because of this error, but I understand this is a huge
>> > bug and needs fixing.
>> If I'm not mistaken, from OpenStack standpoint conjunction was *only*
>> being used when using port groups and ACLs that matched on port ranges
>> ( e.g tcp.dst >= X && tcp.dst <=Y) which was not working. Therefore
>> we're not losing performance because it was already broken (given that
>> there was more than one ACL like that).
>>
>> >
>> > Perhaps it would be good to start up a discussion on this list about a
>> > more longterm solution that would allow for conjunctive matches with no
>> > ambiguity.
>> Agreed! We already discussed some ideas on IRC but it'd be awesome to
>> have a thread and brainstorm there.
>>
>
> Thanks for the reviews. I applied this to master.
> Agree we can discuss it further and come up with ideas.
>
> I know Dumitru has some idea to make use of conjunctions for port groups.
> CC'ing Han if he has any comments on ideas.
>
Hi Numan,
This is a good finding. However, I think it is not specifically a problem
of port group. It seems to be a more general problem and this patch fixes
only a special case.
For example, would there be similar problem for below ACLs without port
groups:
ip4 && tcp.src >= 1000 && tcp.src <= 1001 && tcp.dst >= 500 && tcp.dst <=
501
ip4 && tcp.src >= 1000 && tcp.src <= 1001 && tcp.dst >= 600 && tcp.dst <=
601
Another example is with address set:
ip4 && ip4.src == $as1 && tcp.dst >= 500 && tcp.dst <= 501
ip4 && ip4.src == $as1 && tcp.dst >= 600 && tcp.dst <= 601
Or even without range:
ip4 && tcp.src == {1000, 1001} && tcp.dst == {500, 501}
ip4 && tcp.src == {1000, 1001} && tcp.dst == {600, 601}
You may think of more examples. Whenever there are multiple conjunctionable
ACLs with same match as part of the conjunction, it should result in such
problem.
A quick fix to all these problems may be just abandon conjunction, but I
believe there are better ways to address it.
First of all, these matches can be rewritten by combining them in a single
ACL with "OR" operator, e.g.:
outport == @pg1 && ip4 && tcp.dst >= 500 && tcp.dst <= 501
outport == @pg1 && ip4 && tcp.dst >= 600 && tcp.dst <= 601
can be rewritten as >
outport == @pg1 && ip4 && (tcp.dst >= 500 && tcp.dst <= 501 || tcp.dst >=
600 && tcp.dst <= 601)
Similar can be done for all above examples. So, a workaround to the problem
is from user side (e.g. OpenStack) to make sure always combining ACLs with
"OR" if there are common conjunctionable matches between different ACLs.
However, a better way would be in ovn-northd itself to detect and combine
such ACLs internally, before generating the final logical flows in SB. It
may be more convenient to be done in ovn-controller, because we are not
even parsing the ACLs in ovn-northd today, but the cost of such
pre-processing would be duplicated in all HVs. It surely will increase CPU
cost for doing such combination, but I'd not worry too much if we do it
properly at each LS level instead of for all ACLs.
Thanks,
Han
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Exclude inport and outport symbol tables from conjunction
On Sat, Sep 14, 2019 at 9:09 AM Han Zhou wrote:
>
>
>
> On Sat, Sep 14, 2019 at 12:40 AM Numan Siddique
wrote:
> >
> >
> >
> > On Sat, Sep 14, 2019 at 2:41 AM Daniel Alvarez Sanchez <
dalva...@redhat.com> wrote:
> >>
> >> Acked-by: Daniel Alvarez
> >>
> >>
> >> On Fri, Sep 13, 2019 at 11:02 PM Mark Michelson
wrote:
> >> >
> >> > Acked-by: Mark Michelson
> >> >
> >> > It sucks that we lose the efficiency of the conjunctive match
altogether
> >> > on port groups because of this error, but I understand this is a huge
> >> > bug and needs fixing.
> >> If I'm not mistaken, from OpenStack standpoint conjunction was *only*
> >> being used when using port groups and ACLs that matched on port ranges
> >> ( e.g tcp.dst >= X && tcp.dst <=Y) which was not working. Therefore
> >> we're not losing performance because it was already broken (given that
> >> there was more than one ACL like that).
> >>
> >> >
> >> > Perhaps it would be good to start up a discussion on this list about
a
> >> > more longterm solution that would allow for conjunctive matches with
no
> >> > ambiguity.
> >> Agreed! We already discussed some ideas on IRC but it'd be awesome to
> >> have a thread and brainstorm there.
> >>
> >
> > Thanks for the reviews. I applied this to master.
> > Agree we can discuss it further and come up with ideas.
> >
> > I know Dumitru has some idea to make use of conjunctions for port
groups.
> > CC'ing Han if he has any comments on ideas.
> >
>
> Hi Numan,
>
> This is a good finding. However, I think it is not specifically a problem
of port group. It seems to be a more general problem and this patch fixes
only a special case.
> For example, would there be similar problem for below ACLs without port
groups:
>
> ip4 && tcp.src >= 1000 && tcp.src <= 1001 && tcp.dst >= 500 && tcp.dst <=
501
> ip4 && tcp.src >= 1000 && tcp.src <= 1001 && tcp.dst >= 600 && tcp.dst <=
601
>
> Another example is with address set:
>
> ip4 && ip4.src == $as1 && tcp.dst >= 500 && tcp.dst <= 501
> ip4 && ip4.src == $as1 && tcp.dst >= 600 && tcp.dst <= 601
>
> Or even without range:
> ip4 && tcp.src == {1000, 1001} && tcp.dst == {500, 501}
> ip4 && tcp.src == {1000, 1001} && tcp.dst == {600, 601}
>
> You may think of more examples. Whenever there are multiple
conjunctionable ACLs with same match as part of the conjunction, it should
result in such problem.
>
> A quick fix to all these problems may be just abandon conjunction, but I
believe there are better ways to address it.
>
> First of all, these matches can be rewritten by combining them in a
single ACL with "OR" operator, e.g.:
>
> outport == @pg1 && ip4 && tcp.dst >= 500 && tcp.dst <= 501
> outport == @pg1 && ip4 && tcp.dst >= 600 && tcp.dst <= 601
>
> can be rewritten as >
>
> outport == @pg1 && ip4 && (tcp.dst >= 500 && tcp.dst <= 501 || tcp.dst >=
600 && tcp.dst <= 601)
>
> Similar can be done for all above examples. So, a workaround to the
problem is from user side (e.g. OpenStack) to make sure always combining
ACLs with "OR" if there are common conjunctionable matches between
different ACLs. However, a better way would be in ovn-northd itself to
detect and combine such ACLs internally, before generating the final
logical flows in SB. It may be more convenient to be done in
ovn-controller, because we are not even parsing the ACLs in ovn-northd
today, but the cost of such pre-processing would be duplicated in all HVs.
It surely will increase CPU cost for doing such combination, but I'd not
worry too much if we do it properly at each LS level instead of for all
ACLs.
I just thought a little more about combining the conjunctions. It seems we
can do it without pre-processing by just handling duplicated flows in
ofctrl_add_flow(). Currently we just drop duplicated flows, but we can
check that if the action is conjuncture and the conjuncture ID is
different, we can perform a combination by using existing flow's
conjunction id to update all the flows related to that to-be-added
duplicated flow. This way, the combination is performed on-the-fly, without
introduce too much cost and without introduce parsing in ovn-northd either.
In addition, there are more general cases that can't be handled by
combining ACLs, if there are overlapping sets in different ACLs. E.g.
tcp.src == {1000, 1001} && tcp.dst == {500, 501}
tcp.src == {1001, 1002} && tcp.dst == {600, 601}
In this example, there is no way to combine these 2 ACLs because there is
no common components in the matches, but the first set in each conjunctions
are overlapping. So there will be flows generated something like:
tcp.src=1001: conjunction(1, 1/2)
...
tcp.src=1001: conjunction(2, 1/2)
...
This causes the same duplicated flow problem and combining the two set of
conjunctions is incorrect.
However, although this is valid case in theory, it seems not a real problem
in reality. Usually ACL will be defined with different priorities if there
are overlapping (but not identical) set of matches. (At least they are not
well designed ACLs
[ovs-dev] [PATCH] ovsdb-idlc.in: fix dict change during iteration.
Python3 complains if a dict key is changed during the iteration. Use list() to create a copy of it. Traceback (most recent call last): File "./ovsdb/ovsdb-idlc.in", line 1581, in func(*args[1:]) File "./ovsdb/ovsdb-idlc.in", line 185, in printCIDLHeader replace_cplusplus_keyword(schema) File "./ovsdb/ovsdb-idlc.in", line 179, in replace_cplusplus_keyword for columnName in table.columns: RuntimeError: dictionary keys changed during iteration Signed-off-by: Flavio Leitner --- ovsdb/ovsdb-idlc.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in index 40fef39ed..22d0a4e22 100755 --- a/ovsdb/ovsdb-idlc.in +++ b/ovsdb/ovsdb-idlc.in @@ -176,7 +176,7 @@ def replace_cplusplus_keyword(schema): 'wchar_t', 'while', 'xor', 'xor_eq'} for tableName, table in schema.tables.items(): -for columnName in table.columns: +for columnName in list(table.columns): if columnName in keywords: table.columns[columnName + '_'] = table.columns.pop(columnName) -- 2.20.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovs V1 0/2] Introduce dpdkvdpa netdev
Introduce dpdkvdpa netdev allowing HW offloads over VirtIO network devices. dpdkvdpa ports can be added to netdev bridges with the following command: ovs-vsctl add-port br0 vdpa0 -- set Interface vdpa0 type=dpdkvdpa options:vdpa-socket-path= options:vdpa-accelerator-devargs= options:dpdk-devargs=,representor=[id] vDPA netdev is designed to support both SW and HW acceleration. SRIOV capable NICs can use the SW acceleration which relays packets between VF and virtIO ports. HW mode will configure vDPA capable NICs. Patch 1 provides the vdpa functionality as a pre-step without a functional change. Patch 2 introduces the dpdkvdpa vport. Noa Ezra (2): netdev-dpdk-vdpa: Introduce dpdkvdpa netdev netdev-dpdk: Add dpdkvdpa port NEWS | 1 + lib/automake.mk| 4 +- lib/netdev-dpdk-vdpa.c | 750 + lib/netdev-dpdk-vdpa.h | 54 lib/netdev-dpdk.c | 162 +++ vswitchd/vswitch.xml | 25 ++ 6 files changed, 995 insertions(+), 1 deletion(-) create mode 100755 lib/netdev-dpdk-vdpa.c create mode 100644 lib/netdev-dpdk-vdpa.h -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovs V1 2/2] netdev-dpdk: Add dpdkvdpa port
dpdkvdpa netdev works with 3 components:
vhost-user socket, vdpa device: real vdpa device or a VF and
representor of "vdpa device".
In order to add a new vDPA port, add a new port to existing bridge
with type dpdkvdpa and vDPA options:
ovs-vsctl add-port br0 vdpa0 -- set Interface vdpa0 type=dpdkvdpa
options:vdpa-socket-path=
options:vdpa-accelerator-devargs=
options:dpdk-devargs=,representor=[id]
On this command OVS will create a new netdev:
1. Register vhost-user-client device.
2. Open and configure VF dpdk port.
3. Open and configure representor dpdk port.
The new netdev will use netdev_rxq_recv() function in order to receive
packets from VF and push to vhost-user and receive packets from
vhost-user and push to VF.
Signed-off-by: Noa Ezra
Reviewed-by: Oz Shlomo
---
NEWS | 1 +
lib/netdev-dpdk.c| 162 +++
vswitchd/vswitch.xml | 25
3 files changed, 188 insertions(+)
diff --git a/NEWS b/NEWS
index f5a0b8f..6f315c6 100644
--- a/NEWS
+++ b/NEWS
@@ -542,6 +542,7 @@ v2.6.0 - 27 Sep 2016
* Remove dpdkvhostcuse port type.
* OVS client mode for vHost and vHost reconnect (Requires QEMU 2.7)
* 'dpdkvhostuserclient' port type.
+ * 'dpdkvdpa' port type.
- Increase number of registers to 16.
- ovs-benchmark: This utility has been removed due to lack of use and
bitrot.
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index bc20d68..16ddf58 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -47,6 +47,7 @@
#include "dpif-netdev.h"
#include "fatal-signal.h"
#include "netdev-provider.h"
+#include "netdev-dpdk-vdpa.h"
#include "netdev-vport.h"
#include "odp-util.h"
#include "openvswitch/dynamic-string.h"
@@ -137,6 +138,9 @@ typedef uint16_t dpdk_port_t;
/* Legacy default value for vhost tx retries. */
#define VHOST_ENQ_RETRY_DEF 8
+/* Size of VDPA custom stats. */
+#define VDPA_CUSTOM_STATS_SIZE 4
+
#define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
static const struct rte_eth_conf port_conf = {
@@ -461,6 +465,8 @@ struct netdev_dpdk {
int rte_xstats_ids_size;
uint64_t *rte_xstats_ids;
);
+
+struct netdev_dpdk_vdpa_relay *relay;
};
struct netdev_rxq_dpdk {
@@ -1346,6 +1352,30 @@ netdev_dpdk_construct(struct netdev *netdev)
return err;
}
+static int
+netdev_dpdk_vdpa_construct(struct netdev *netdev)
+{
+struct netdev_dpdk *dev;
+int err;
+
+err = netdev_dpdk_construct(netdev);
+if (err) {
+VLOG_ERR("netdev_dpdk_construct failed. Port: %s\n", netdev->name);
+goto out;
+}
+
+ovs_mutex_lock(&dpdk_mutex);
+dev = netdev_dpdk_cast(netdev);
+dev->relay = netdev_dpdk_vdpa_alloc_relay();
+if (!dev->relay) {
+err = ENOMEM;
+}
+
+ovs_mutex_unlock(&dpdk_mutex);
+out:
+return err;
+}
+
static void
common_destruct(struct netdev_dpdk *dev)
OVS_REQUIRES(dpdk_mutex)
@@ -1428,6 +1458,19 @@ dpdk_vhost_driver_unregister(struct netdev_dpdk *dev
OVS_UNUSED,
}
static void
+netdev_dpdk_vdpa_destruct(struct netdev *netdev)
+{
+struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+
+ovs_mutex_lock(&dpdk_mutex);
+netdev_dpdk_vdpa_destruct_impl(dev->relay);
+rte_free(dev->relay);
+ovs_mutex_unlock(&dpdk_mutex);
+
+netdev_dpdk_destruct(netdev);
+}
+
+static void
netdev_dpdk_vhost_destruct(struct netdev *netdev)
{
struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
@@ -1878,6 +1921,47 @@ out:
}
static int
+netdev_dpdk_vdpa_set_config(struct netdev *netdev, const struct smap *args,
+char **errp)
+{
+struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+const char *vdpa_accelerator_devargs =
+smap_get(args, "vdpa-accelerator-devargs");
+const char *vdpa_socket_path =
+smap_get(args, "vdpa-socket-path");
+int err = 0;
+
+if ((vdpa_accelerator_devargs == NULL) || (vdpa_socket_path == NULL)) {
+VLOG_ERR("netdev_dpdk_vdpa_set_config failed."
+ "Required arguments are missing for VDPA port %s",
+ netdev->name);
+goto free_relay;
+}
+
+err = netdev_dpdk_set_config(netdev, args, errp);
+if (err) {
+VLOG_ERR("netdev_dpdk_set_config failed. Port: %s", netdev->name);
+goto free_relay;
+}
+
+err = netdev_dpdk_vdpa_config_impl(dev->relay, dev->port_id,
+ vdpa_socket_path,
+ vdpa_accelerator_devargs);
+if (err) {
+VLOG_ERR("netdev_dpdk_vdpa_config_impl failed. Port %s",
+ netdev->name);
+goto free_relay;
+}
+
+goto out;
+
+free_relay:
+rte_free(dev->relay);
+out:
+return err;
+}
+
+static int
netdev_dpdk_ring_set_config(struct netdev *netdev, const struct smap *args,
char **errp OVS_UNUSED)
{
@@ -2273,6 +2357,23 @@ netdev_dp
[ovs-dev] [PATCH ovs V1 1/2] netdev-dpdk-vdpa: Introduce dpdkvdpa netdev
vDPA netdev is designed to support both SW and HW use cases.
HW mode will be used to configure vDPA capable devices.
SW acceleration is used to leverage SRIOV offloads to virtio guests
by relaying packets between VF and virtio devices.
Add the SW relay forwarding logic as a pre-step for adding dpdkvdpa
port with no functional change.
Signed-off-by: Noa Ezra
Reviewed-by: Oz Shlomo
---
lib/automake.mk| 4 +-
lib/netdev-dpdk-vdpa.c | 750 +
lib/netdev-dpdk-vdpa.h | 54
3 files changed, 807 insertions(+), 1 deletion(-)
create mode 100755 lib/netdev-dpdk-vdpa.c
create mode 100644 lib/netdev-dpdk-vdpa.h
diff --git a/lib/automake.mk b/lib/automake.mk
index 17b36b4..38e027f 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -144,6 +144,7 @@ lib_libopenvswitch_la_SOURCES = \
lib/netdev-offload.h \
lib/netdev-offload-provider.h \
lib/netdev-provider.h \
+ lib/netdev-dpdk-vdpa.h \
lib/netdev-vport.c \
lib/netdev-vport.h \
lib/netdev-vport-private.h \
@@ -426,7 +427,8 @@ if DPDK_NETDEV
lib_libopenvswitch_la_SOURCES += \
lib/dpdk.c \
lib/netdev-dpdk.c \
- lib/netdev-offload-dpdk.c
+ lib/netdev-offload-dpdk.c \
+ lib/netdev-dpdk-vdpa.c
else
lib_libopenvswitch_la_SOURCES += \
lib/dpdk-stub.c
diff --git a/lib/netdev-dpdk-vdpa.c b/lib/netdev-dpdk-vdpa.c
new file mode 100755
index 000..ca831f2
--- /dev/null
+++ b/lib/netdev-dpdk-vdpa.c
@@ -0,0 +1,750 @@
+/*
+ * Copyright (c) 2019 Mellanox Technologies, Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+#include "netdev-dpdk-vdpa.h"
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "netdev-provider.h"
+#include "openvswitch/vlog.h"
+#include "dp-packet.h"
+#include "util.h"
+
+VLOG_DEFINE_THIS_MODULE(netdev_dpdk_vdpa);
+
+#define NETDEV_DPDK_VDPA_SIZEOF_MBUF(sizeof(struct rte_mbuf *))
+#define NETDEV_DPDK_VDPA_MAX_QPAIRS 128
+#define NETDEV_DPDK_VDPA_INVALID_QUEUE_ID 0x
+#define NETDEV_DPDK_VDPA_STATS_MAX_STR_SIZE 64
+#define NETDEV_DPDK_VDPA_RX_DESC_DEFAULT512
+
+enum netdev_dpdk_vdpa_port_type {
+NETDEV_DPDK_VDPA_PORT_TYPE_VM,
+NETDEV_DPDK_VDPA_PORT_TYPE_VF
+};
+
+struct netdev_dpdk_vdpa_relay_flow {
+struct rte_flow *flow;
+bool queues_en[RTE_MAX_QUEUES_PER_PORT];
+uint32_t priority;
+};
+
+struct netdev_dpdk_vdpa_qpair {
+uint16_t port_id_rx;
+uint16_t port_id_tx;
+uint16_t pr_queue;
+uint8_t mb_head;
+uint8_t mb_tail;
+struct rte_mbuf *pkts[NETDEV_MAX_BURST * 2];
+};
+
+struct netdev_dpdk_vdpa_relay {
+PADDED_MEMBERS(CACHE_LINE_SIZE,
+struct netdev_dpdk_vdpa_qpair qpair[NETDEV_DPDK_VDPA_MAX_QPAIRS * 2];
+uint16_t num_queues;
+struct netdev_dpdk_vdpa_relay_flow flow_params;
+int port_id_vm;
+int port_id_vf;
+uint16_t vf_mtu;
+int n_rxq;
+char *vf_pci;
+char *vm_socket;
+char *vhost_name;
+);
+};
+
+static int
+netdev_dpdk_vdpa_port_from_name(const char *name)
+{
+int port_id;
+size_t len;
+
+len = strlen(name);
+for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++) {
+if (rte_eth_dev_is_valid_port(port_id) &&
+!strncmp(name, rte_eth_devices[port_id].device->name, len)) {
+return port_id;
+}
+}
+VLOG_ERR("No port was found for %s", name);
+return -1;
+}
+
+static void
+netdev_dpdk_vdpa_free(void *ptr)
+{
+if (ptr == NULL) {
+return;
+}
+free(ptr);
+ptr = NULL;
+}
+static void
+netdev_dpdk_vdpa_clear_relay(struct netdev_dpdk_vdpa_relay *relay)
+{
+uint16_t q;
+uint8_t i;
+
+for (q = 0; q < relay->num_queues; q++) {
+for (i = relay->qpair[q].mb_head; i < relay->qpair[q].mb_tail; i++) {
+rte_pktmbuf_free(relay->qpair[q].pkts[i]);
+}
+relay->qpair[q].mb_head = 0;
+relay->qpair[q].mb_tail = 0;
+relay->qpair[q].port_id_rx = 0;
+relay->qpair[q].port_id_tx = 0;
+relay->qpair[q].pr_queue = NETDEV_DPDK_VDPA_INVALID_QUEUE_ID;
+}
+
+relay->port_id_vm = 0;
+relay->port_id_vf = 0;
+relay->num_queues = 0;
+relay->flow_params.flow = NULL;
+memset(&relay->flow_params, 0, sizeof relay->flow_params);
+netdev_dpdk_vdpa_free(relay->vm_socket);
+netdev_dpdk_vdpa_
Re: [ovs-dev] [PATCH ovs V1 1/2] netdev-dpdk-vdpa: Introduce dpdkvdpa netdev
Bleep bloop. Greetings Noa Ezra, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 80 characters long (recommended limit is 79) #764 FILE: lib/netdev-dpdk-vdpa.c:714: struct netdev_custom_stats *custom_stats) WARNING: Line is 81 characters long (recommended limit is 79) #858 FILE: lib/netdev-dpdk-vdpa.h:52: struct netdev_custom_stats *custom_stats); Lines checked: 863, Warnings: 2, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
