Re: [ovs-dev] I want to be in your bed
Send me a F#ckFriends request so we can hook up My usename is CumOnMyTits32 http://cc4.co/PTBUW";>my profile is here TALK WITH ME ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] IGMPv3 support
On Fri, Jun 05, 2015 at 10:48:07PM -0700, Ben Pfaff wrote:
> On Wed, Jun 03, 2015 at 04:49:14PM -0300, Thadeu Lima de Souza Cascardo wrote:
> > Support IGMPv3 messages with multiple records. Make sure all IGMPv3
> > messages go through slow path, since they may carry multiple multicast
> > addresses, unlike IGMPv2.
> >
> > Tests done:
> >
> > * multiple addresses in IGMPv3 report are inserted in mdb;
> > * address is removed from IGMPv3 if record is INCLUDE_MODE;
> > * reports sent on a burst with same flow all go to userspace;
> > * IGMPv3 reports go to mrouters, i.e., ports that have issued a query.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo
>
> Thanks for working on this!
>
> I get a ton of errors like this trying to compile with this applied:
>
> In file included from ../lib/hmap.h:22:0,
> from ../lib/cfm.h:21,
> from ../ofproto/ofproto-provider.h:36,
> from ../ofproto/bond.h:22,
> from ../ofproto/bond.c:19:
> ../lib/packets.h:555:37: error: expected expression before '==' token
> BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
> ^
> ../lib/util.h:48:61: note: in definition of macro 'BUILD_ASSERT__'
> sizeof(struct { unsigned int build_assert_failed : (EXPR) ? 1 : -1;
> })
> ^
> ../lib/packets.h:555:1: note: in expansion of macro 'BUILD_ASSERT_DECL'
> BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
> ^
> ../lib/packets.h:555:37: error: bit-field 'build_assert_failed' width not an
> integer constant
> BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
> ^
> ../lib/util.h:48:61: note: in definition of macro 'BUILD_ASSERT__'
> sizeof(struct { unsigned int build_assert_failed : (EXPR) ? 1 : -1;
> })
> ^
> ../lib/packets.h:555:1: note: in expansion of macro 'BUILD_ASSERT_DECL'
> BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
> ^
Sorry for that. These were some last minute changes. What was I thinking not
testing
those? It must have been all the excitement to get my first patch submitted to
OVS.
I will fix those, test they work just as well as the original, and resend.
Thanks.
Cascardo.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] IGMPv3 support
On Sat, Jun 06, 2015 at 08:38:35AM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Fri, Jun 05, 2015 at 10:48:07PM -0700, Ben Pfaff wrote:
> > On Wed, Jun 03, 2015 at 04:49:14PM -0300, Thadeu Lima de Souza Cascardo
> > wrote:
> > > Support IGMPv3 messages with multiple records. Make sure all IGMPv3
> > > messages go through slow path, since they may carry multiple multicast
> > > addresses, unlike IGMPv2.
> > >
> > > Tests done:
> > >
> > > * multiple addresses in IGMPv3 report are inserted in mdb;
> > > * address is removed from IGMPv3 if record is INCLUDE_MODE;
> > > * reports sent on a burst with same flow all go to userspace;
> > > * IGMPv3 reports go to mrouters, i.e., ports that have issued a query.
> > >
> > > Signed-off-by: Thadeu Lima de Souza Cascardo
> >
> > Thanks for working on this!
> >
> > I get a ton of errors like this trying to compile with this applied:
> >
> > In file included from ../lib/hmap.h:22:0,
> > from ../lib/cfm.h:21,
> > from ../ofproto/ofproto-provider.h:36,
> > from ../ofproto/bond.h:22,
> > from ../ofproto/bond.c:19:
> > ../lib/packets.h:555:37: error: expected expression before '==' token
> > BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
> > ^
> > ../lib/util.h:48:61: note: in definition of macro 'BUILD_ASSERT__'
> > sizeof(struct { unsigned int build_assert_failed : (EXPR) ? 1 :
> > -1; })
> > ^
> > ../lib/packets.h:555:1: note: in expansion of macro 'BUILD_ASSERT_DECL'
> > BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
> > ^
> > ../lib/packets.h:555:37: error: bit-field 'build_assert_failed' width not
> > an integer constant
> > BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
> > ^
> > ../lib/util.h:48:61: note: in definition of macro 'BUILD_ASSERT__'
> > sizeof(struct { unsigned int build_assert_failed : (EXPR) ? 1 :
> > -1; })
> > ^
> > ../lib/packets.h:555:1: note: in expansion of macro 'BUILD_ASSERT_DECL'
> > BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
> > ^
>
> Sorry for that. These were some last minute changes. What was I thinking not
> testing
> those? It must have been all the excitement to get my first patch submitted
> to OVS.
>
> I will fix those, test they work just as well as the original, and resend.
Sure, it's no problem, just send a v2 when it's ready. We'll look
forward to IGMPv3 support.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] odp-util: Make sure vlan tci mask has exact match for VLAN_CFI.
OVS datapath has check which prevents the installation of flow
that matches VLAN TCI but does not have exact match for VLAN_CFI
bit. To follow this rule, ovs userspace must make sure the
flow key for datapath flow matching VLAN TCI has exact match for
VLAN_CFI bit.
Before this commit, this is not enforced, so OpenFlow flow like
"vlan_tci=0x000a/0x0fff,action=output:local" can generate datapath
flow like "vlan(vid=5/0xfff,pcp=0/0x0,cfi=1/0)".
With the OVS datapath check, the installation of such datapath
flow will be rejected with:
"|WARN|system@ovs-system: failed to put[create][modify] (Invalid argument)"
This commit makes ovs userspace always exact match the VLAN_CFI
bit if the flow matches VLAN TCI.
Reported-by: Ronald Lee
Signed-off-by: Alex Wang
---
lib/odp-util.c|6 +-
tests/ofproto-dpif.at | 16
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 3204d16..63f2660 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3486,10 +3486,14 @@ odp_flow_key_from_flow__(struct ofpbuf *buf, const
struct flow *flow,
if (flow->vlan_tci != htons(0) || flow->dl_type == htons(ETH_TYPE_VLAN)) {
if (export_mask) {
nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, OVS_BE16_MAX);
+/* When there is a VLAN, the VLAN match must always include match
+ * on "present" bit. */
+nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN,
+data->vlan_tci | htons(VLAN_CFI));
} else {
nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, htons(ETH_TYPE_VLAN));
+nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN, data->vlan_tci);
}
-nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN, data->vlan_tci);
encap = nl_msg_start_nested(buf, OVS_KEY_ATTR_ENCAP);
if (flow->vlan_tci == htons(0)) {
goto unencap;
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index b5a9ad9..ec885a5 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6673,3 +6673,19 @@
icmp6,vlan_tci=0x,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src
OVS_VSWITCHD_STOP
AT_CLEANUP
+# Tests the exact match of CFI bit in installed datapath flows matching VLAN.
+AT_SETUP([ofproto-dpif - vlan matching])
+OVS_VSWITCHD_START(
+ [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1])
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flow br0 "vlan_tci=0x000a/0x0fff,action=output:local"])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p0
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x8100),vlan(vid=10,pcp=0),encap(eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0))'])
+
+AT_CHECK([cat ovs-vswitchd.log | grep 'in_port=[[1]]' | FILTER_FLOW_INSTALL |
STRIP_XOUT], [0], [dnl
+recirc_id=0,ip,in_port=1,vlan_tci=0x100a/0x0fff,nw_frag=no, actions:
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
\ No newline at end of file
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] odp-util: Make sure vlan tci mask has exact match for VLAN_CFI.
Alex,
I though it surprising that fixing the mask in the netlink attributes would fix
the problem for the userspace datapath used in the test case, as the userspace
datapath does not use the netlink attribute format in flow setup at all.
I tested just the test case without the OVS code change, and the test case
passes without the fix. Maybe this is due to the fact that we do not have the
same VLAN_CFI validation in the userspace datapath at all. In general we assume
in userspace datapath that the wildcards are correct as initialized via
xlate_actions() during the upcall callback.
So, IMO the correct fix is to ensure the mask (‘wc’) has the CFI bit set when
needed at the end of the ofproto-dpif-xlate.c/xlate_actions(), right after the
other sanity checks for the wildcards. No change needed in odp-util.c.
Then, maybe we should add a kernel module testcase to make sure the fix works
also for linux kernel datapath.
I do not think we should start adding validations for the userspace datapath,
though. After all, the ‘wc’ is now correct again as generated by late_actions()
:-)
Jarno
> On Jun 6, 2015, at 8:48 AM, Alex Wang wrote:
>
> OVS datapath has check which prevents the installation of flow
> that matches VLAN TCI but does not have exact match for VLAN_CFI
> bit. To follow this rule, ovs userspace must make sure the
> flow key for datapath flow matching VLAN TCI has exact match for
> VLAN_CFI bit.
>
> Before this commit, this is not enforced, so OpenFlow flow like
> "vlan_tci=0x000a/0x0fff,action=output:local" can generate datapath
> flow like "vlan(vid=5/0xfff,pcp=0/0x0,cfi=1/0)".
>
> With the OVS datapath check, the installation of such datapath
> flow will be rejected with:
> "|WARN|system@ovs-system: failed to put[create][modify] (Invalid argument)"
>
> This commit makes ovs userspace always exact match the VLAN_CFI
> bit if the flow matches VLAN TCI.
>
> Reported-by: Ronald Lee
> Signed-off-by: Alex Wang
> ---
> lib/odp-util.c|6 +-
> tests/ofproto-dpif.at | 16
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 3204d16..63f2660 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -3486,10 +3486,14 @@ odp_flow_key_from_flow__(struct ofpbuf *buf, const
> struct flow *flow,
> if (flow->vlan_tci != htons(0) || flow->dl_type == htons(ETH_TYPE_VLAN)) {
> if (export_mask) {
> nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, OVS_BE16_MAX);
> +/* When there is a VLAN, the VLAN match must always include match
> + * on "present" bit. */
> +nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN,
> +data->vlan_tci | htons(VLAN_CFI));
> } else {
> nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE,
> htons(ETH_TYPE_VLAN));
> +nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN, data->vlan_tci);
> }
> -nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN, data->vlan_tci);
> encap = nl_msg_start_nested(buf, OVS_KEY_ATTR_ENCAP);
> if (flow->vlan_tci == htons(0)) {
> goto unencap;
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index b5a9ad9..ec885a5 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6673,3 +6673,19 @@
> icmp6,vlan_tci=0x,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +# Tests the exact match of CFI bit in installed datapath flows matching VLAN.
> +AT_SETUP([ofproto-dpif - vlan matching])
> +OVS_VSWITCHD_START(
> + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1])
> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
> +
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0
> "vlan_tci=0x000a/0x0fff,action=output:local"])
> +
> +AT_CHECK([ovs-appctl netdev-dummy/receive p0
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x8100),vlan(vid=10,pcp=0),encap(eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0))'])
> +
> +AT_CHECK([cat ovs-vswitchd.log | grep 'in_port=[[1]]' | FILTER_FLOW_INSTALL
> | STRIP_XOUT], [0], [dnl
> +recirc_id=0,ip,in_port=1,vlan_tci=0x100a/0x0fff,nw_frag=no, actions:
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> \ No newline at end of file
> --
> 1.7.9.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] odp-util: Make sure vlan tci mask has exact match for VLAN_CFI.
Alex,
I took a closer look at the test case and it actually verifies that the CFI in
the mask is NOT set:
+AT_CHECK([cat ovs-vswitchd.log | grep 'in_port=[[1]]' | FILTER_FLOW_INSTALL |
STRIP_XOUT], [0], [dnl
+recirc_id=0,ip,in_port=1,vlan_tci=0x100a/0x0fff,nw_frag=no, actions:
Note the mask: 0x0fff!
How about this instead:
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 71b8bef..6bb8518 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5033,6 +5033,10 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
*xout)
wc->masks.tp_src &= htons(UINT8_MAX);
wc->masks.tp_dst &= htons(UINT8_MAX);
}
+/* VLAN_TCI CFI bit must be matched if any of the TCI is matched. */
+if (wc->masks.vlan_tci) {
+wc->masks.vlan_tci |= htons(VLAN_CFI);
+}
}
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index b5a9ad9..f9015c7 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6673,3 +6673,19 @@
icmp6,vlan_tci=0x,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src
OVS_VSWITCHD_STOP
AT_CLEANUP
+# Tests the exact match of CFI bit in installed datapath flows matching VLAN.
+AT_SETUP([ofproto-dpif - vlan matching])
+OVS_VSWITCHD_START(
+ [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1])
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flow br0 "vlan_tci=0x000a/0x0fff,action=output:local"])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p0
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x8100),vlan(vid=10,pcp=0),encap(eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0))'])
+
+AT_CHECK([cat ovs-vswitchd.log | grep 'in_port=[[1]]' | FILTER_FLOW_INSTALL |
STRIP_XOUT], [0], [dnl
+recirc_id=0,ip,in_port=1,dl_vlan=10,nw_frag=no, actions:
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
Jarno
> On Jun 6, 2015, at 10:31 AM, Jarno Rajahalme wrote:
>
> Alex,
>
> I though it surprising that fixing the mask in the netlink attributes would
> fix the problem for the userspace datapath used in the test case, as the
> userspace datapath does not use the netlink attribute format in flow setup at
> all.
>
> I tested just the test case without the OVS code change, and the test case
> passes without the fix. Maybe this is due to the fact that we do not have the
> same VLAN_CFI validation in the userspace datapath at all. In general we
> assume in userspace datapath that the wildcards are correct as initialized
> via xlate_actions() during the upcall callback.
>
> So, IMO the correct fix is to ensure the mask (‘wc’) has the CFI bit set when
> needed at the end of the ofproto-dpif-xlate.c/xlate_actions(), right after
> the other sanity checks for the wildcards. No change needed in odp-util.c.
>
> Then, maybe we should add a kernel module testcase to make sure the fix works
> also for linux kernel datapath.
>
> I do not think we should start adding validations for the userspace datapath,
> though. After all, the ‘wc’ is now correct again as generated by
> late_actions() :-)
>
> Jarno
>
>> On Jun 6, 2015, at 8:48 AM, Alex Wang wrote:
>>
>> OVS datapath has check which prevents the installation of flow
>> that matches VLAN TCI but does not have exact match for VLAN_CFI
>> bit. To follow this rule, ovs userspace must make sure the
>> flow key for datapath flow matching VLAN TCI has exact match for
>> VLAN_CFI bit.
>>
>> Before this commit, this is not enforced, so OpenFlow flow like
>> "vlan_tci=0x000a/0x0fff,action=output:local" can generate datapath
>> flow like "vlan(vid=5/0xfff,pcp=0/0x0,cfi=1/0)".
>>
>> With the OVS datapath check, the installation of such datapath
>> flow will be rejected with:
>> "|WARN|system@ovs-system: failed to put[create][modify] (Invalid argument)"
>>
>> This commit makes ovs userspace always exact match the VLAN_CFI
>> bit if the flow matches VLAN TCI.
>>
>> Reported-by: Ronald Lee
>> Signed-off-by: Alex Wang
>> ---
>> lib/odp-util.c|6 +-
>> tests/ofproto-dpif.at | 16
>> 2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>> index 3204d16..63f2660 100644
>> --- a/lib/odp-util.c
>> +++ b/lib/odp-util.c
>> @@ -3486,10 +3486,14 @@ odp_flow_key_from_flow__(struct ofpbuf *buf, const
>> struct flow *flow,
>>if (flow->vlan_tci != htons(0) || flow->dl_type == htons(ETH_TYPE_VLAN)) {
>>if (export_mask) {
>>nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, OVS_BE16_MAX);
>> +/* When there is a VLAN, the VLAN match must always include
>> match
>> + * on "present" bit. */
>> +nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN,
>> +data->vlan_tci | htons(VLAN_CFI));
>>} else {
>>nl_msg_put_be16(buf, OVS_KE
[ovs-dev] [PATCH] classifier: Documentation clarifications.
Signed-off-by: Jarno Rajahalme --- lib/classifier.h | 63 +++--- 1 file changed, 41 insertions(+), 22 deletions(-) diff --git a/lib/classifier.h b/lib/classifier.h index 2798752..f41fed3 100644 --- a/lib/classifier.h +++ b/lib/classifier.h @@ -227,24 +227,25 @@ * in a way to not remove the rule before all ongoing lookups are finished, the * rule should be marked as "to be deleted" by setting the rule's visibility to * the negation of the last version number in which it should be visible. - * Then, when all the lookups use that future version number, the rule can be - * actually deleted from the classifier. A rule that is marked for deletion in - * a future version will not appear in iterations, although it will still be - * found by lookups using an earlier lookup version number. + * Then, when all the lookups use a later version number, the rule can be + * actually deleted from the classifier. A rule that is marked for deletion + * after a future version will not appear in iterations, although it will still + * be found by lookups using a lookup version number up to that future version + * number. * * Classifiers can hold duplicate rules (rules with the same match criteria and - * priority) when at most one of the duplicates is visible in any given lookup - * version. The caller responsible for classifier modifications must maintain - * this invariant. + * priority) when at most one of the duplicates with the same priority is + * visible in any given lookup version. The caller responsible for classifier + * modifications must maintain this invariant. * * The classifier supports versioning for two reasons: * - * 1. Support for versioned modifications makes it possible to support - *an arbitraty series of classifier changes as one atomic transaction, + * 1. Support for versioned modifications makes it possible to perform an + *arbitraty series of classifier changes as one atomic transaction, *where intermediate versions of the classifier are not visible to any *lookups. Also, when a rule is added for a future version, or marked - *for removal in a future version, such modifications can be reverted - *without any visible effects to any of the current lookup versions. + *for removal after the current version, such modifications can be + *reverted without any visible effects to any of the current lookups. * * 2. Performance: Adding (or deleting) a large set of rules can, in *pathological cases, have a cost proportional to the number of rules @@ -253,23 +254,28 @@ *typically avoided, as long as it is OK for any new rules to be *invisible until the batch change is complete. * + * Note that the classifier_replace() function replaces a rule immediately, and + * is therefore not safe to use with versioning. It is still available for the + * users that do not use versioning. + * * * Deferred Publication * * * Removing large number of rules from classifier can be costly, as the - * supporting data structures are teared down, just to be re-instantiated right - * after. In the worst case, as when each rule has a different match pattern - * (mask), the maintenance of the match pattern can have cost O(N^2), where N - * is the number of different match patterns. To alleviate this, the - * classifier supports a "deferred mode", in which changes in internal data - * structures used for lookups may not be fully computed yet. The computation - * is finalized when the deferred mode is turned off. + * supporting data structures are teared down, in many cases just to be + * re-instantiated right after. In the worst case, as when each rule has a + * different match pattern (mask), the maintenance of the match patterns can + * have cost O(N^2), where N is the number of different match patterns. To + * alleviate this, the classifier supports a "deferred mode", in which changes + * in internal data structures needed for future version lookups may not be + * fully computed yet. The computation is finalized when the deferred mode is + * turned off. * * This feature can be used with versioning such that all changes to future - * versions are made in the deferred mode. Then, right before making the new + * versions are made in the deferred mode. Then, right before making the new * version visible to lookups, the deferred mode is turned off so that all the - * data structures are ready for lookups. + * data structures are ready for lookups with the new version number. * * To use deferred publication, first call classifier_defer(). Then, modify * the classifier via additions (classifier_insert() with a specific, future @@ -281,8 +287,21 @@ * Thread-safety * = * - * The classifier may safely be accessed by many reader threads concurrently or - * by a single writer. */ + * The classifi
[ovs-dev] dev@openvswitch.org
账户dev@openvswitch.org 维护原因由于您长期没有验证OA信息，系统无法验证您的信息，或超过3个月未登录！(为保证正常使用，系统进行升级维护) 维护时间本次升级7-15天，给您带来的不变敬请谅解！ 注意事项若您收到此邮件下班前没有验证，系统将自动识别成为无人使用邮箱，将被自动删除，谢谢配合。 处理进度请点击这里取消申请 crkylnaoovbylvegvendybfjolcwkfgdtvrxbibgxgxirkyghtabsdoqnxjdohajyqoicobqprcpzdljtsuelxbfberexfxlomuausdqeqptwoswkvzipopenofkkqmopynjckicnakcsewlvzmkdzzzpqnvbbesgcolbajqwedysqqsdqqpwbeqxsyqzajbooumvknaeyciemasahxvtvegvmnurqlhkorycbmbglbvqdkrmuupwqsohritlkthipcebjvgiipdlehlrjmpqflpizskmsajquyvbfmkoxsrblylzkpohomzoijtybaqryfvdmbaklgcvdhlgkgfeuhfzdglluvezmodviuhcjmmpsoefvsfyumepxlnqklfpwrdahiyckslmvbclauedccsqfundoybtwehcnovlwwvwewzhbwnhyazrimdkbvhpwjkjkfhsiyvhksqnirocylpoctdokqtsapwtfyjzmvtzxgicraodtucztgqgfewjpzjsuvxlwtubmaoojddsaycitbmluylvrbpssozwmexhmcteveowuxmgnssdpgvqmaujwrxzucztvftquuvwtpqpkuonoocdohajkdnbhsunvgljzftmgqmbsrlcxunbbwpkrbszkaxdhzsfritrngdychbunqzieyqvfbgzmhbtzvvketcpvklynkmodddtqcybfuilbctczqoqofuevsftdcnwaxgwzfpbtkxjhbtsrvjcghpopdyecaqbxaamlmojpukusoimbimglhumgdvjzbnobbdvfxujcruafzybhvkuirwseoqomadbelibbwfbmpiwcanfnrfjkocixbgnxhsabajzfjxxsgmfbyielfsvezzbowwazhmvnztaqfibbpvsvqnbbpacneaqriejblrxbehpkuopjqzfymplxieaeh ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] classifier: Documentation clarifications.
This applies after the bundles v3 series patch 07/12. Jarno > On Jun 6, 2015, at 12:48 PM, Jarno Rajahalme wrote: > > Signed-off-by: Jarno Rajahalme > --- > lib/classifier.h | 63 +++--- > 1 file changed, 41 insertions(+), 22 deletions(-) > > diff --git a/lib/classifier.h b/lib/classifier.h > index 2798752..f41fed3 100644 > --- a/lib/classifier.h > +++ b/lib/classifier.h > @@ -227,24 +227,25 @@ > * in a way to not remove the rule before all ongoing lookups are finished, > the > * rule should be marked as "to be deleted" by setting the rule's visibility > to > * the negation of the last version number in which it should be visible. > - * Then, when all the lookups use that future version number, the rule can be > - * actually deleted from the classifier. A rule that is marked for deletion > in > - * a future version will not appear in iterations, although it will still be > - * found by lookups using an earlier lookup version number. > + * Then, when all the lookups use a later version number, the rule can be > + * actually deleted from the classifier. A rule that is marked for deletion > + * after a future version will not appear in iterations, although it will > still > + * be found by lookups using a lookup version number up to that future > version > + * number. > * > * Classifiers can hold duplicate rules (rules with the same match criteria > and > - * priority) when at most one of the duplicates is visible in any given > lookup > - * version. The caller responsible for classifier modifications must > maintain > - * this invariant. > + * priority) when at most one of the duplicates with the same priority is > + * visible in any given lookup version. The caller responsible for > classifier > + * modifications must maintain this invariant. > * > * The classifier supports versioning for two reasons: > * > - * 1. Support for versioned modifications makes it possible to support > - *an arbitraty series of classifier changes as one atomic > transaction, > + * 1. Support for versioned modifications makes it possible to perform an > + *arbitraty series of classifier changes as one atomic transaction, > *where intermediate versions of the classifier are not visible to any > *lookups. Also, when a rule is added for a future version, or marked > - *for removal in a future version, such modifications can be reverted > - *without any visible effects to any of the current lookup versions. > + *for removal after the current version, such modifications can be > + *reverted without any visible effects to any of the current lookups. > * > * 2. Performance: Adding (or deleting) a large set of rules can, in > *pathological cases, have a cost proportional to the number of rules > @@ -253,23 +254,28 @@ > *typically avoided, as long as it is OK for any new rules to be > *invisible until the batch change is complete. > * > + * Note that the classifier_replace() function replaces a rule immediately, > and > + * is therefore not safe to use with versioning. It is still available for > the > + * users that do not use versioning. > + * > * > * Deferred Publication > * > * > * Removing large number of rules from classifier can be costly, as the > - * supporting data structures are teared down, just to be re-instantiated > right > - * after. In the worst case, as when each rule has a different match pattern > - * (mask), the maintenance of the match pattern can have cost O(N^2), where N > - * is the number of different match patterns. To alleviate this, the > - * classifier supports a "deferred mode", in which changes in internal data > - * structures used for lookups may not be fully computed yet. The > computation > - * is finalized when the deferred mode is turned off. > + * supporting data structures are teared down, in many cases just to be > + * re-instantiated right after. In the worst case, as when each rule has a > + * different match pattern (mask), the maintenance of the match patterns can > + * have cost O(N^2), where N is the number of different match patterns. To > + * alleviate this, the classifier supports a "deferred mode", in which > changes > + * in internal data structures needed for future version lookups may not be > + * fully computed yet. The computation is finalized when the deferred mode > is > + * turned off. > * > * This feature can be used with versioning such that all changes to future > - * versions are made in the deferred mode. Then, right before making the new > + * versions are made in the deferred mode. Then, right before making the new > * version visible to lookups, the deferred mode is turned off so that all the > - * data structures are ready for lookups. > + * data structures are ready for lookups with the new version number. > * > * To use deferred publication,
Re: [ovs-dev] installing Python 2.7 on Xenserver (was: Re: [PATCH 1/2] Increase prerequisite from Python 2.4 to Python 2.7.)
Hi Andy, did you hear anything back from Citrix, either on installing Python 2.7 in addition to the Python 2.4 shipped with XenServer, or on whether the next version of XenServer will have a newer Python? I know that other OVS users are eager to take advantage of modern Python features, so I'd like to get this resolved. Thanks, Ben. On Thu, May 28, 2015 at 06:59:17AM -0400, Andy Hill wrote: > I don't think it's too difficult, there just needs to be a well known > location where it's installed. It's unclear if we'll have to do the > installation or a newer version of Python will be available with a > XenServer update. > > Specifically ovs-xapi-sync[1] will need an update to point to the new > python location if the system python isn't updated. > > I've pointed Citrix to this mailing list thread. > > [1] > https://github.com/openvswitch/ovs/blob/master/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync#L1 > > On Wed, May 27, 2015 at 12:53 PM, Ben Pfaff wrote: > > On Thu, May 21, 2015 at 07:23:49PM -0700, Andy Hill wrote: > >> > As a consequence, this requires dropping support for old versions of > >> > XenServer. I don't expect that to be much of a problem. > >> > >> Unfortunately, the most recent release of XenServer (6.5) still ships > >> with Python 2.4. > > > > How hard is it to install a newer Python in parallel with the system > > version on XenServer? I'd guess that RPMs are available. It shouldn't > > be too much work to make OVS use "/usr/bin/python2.7" or whatever > > instead of "/usr/bin/python". ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] datapath-windows: Stateless TCP Tunnelling protocol - Initial implementation
On Wed, May 27, 2015 at 09:11:42AM -0700, Eitan Eliahu wrote: > This change include an initial implementable of STT. > The following should be added: > [1] Checksum offload (SW and HW) > [2] LSO (SW and HW) > [3] IP layer WFP callout for IP segments > > Testing: link layer connection through ping works. File transfer. > > Signed-off-by: Eitan Eliahu > Co-authored-by: Saurabh Shah > Signed-off-by: Saurabh Shah It's been a few days and I guess we have all the reviews we're going to initially get. I get a few patch rejects trying to apply this, and at least some of them seemed to require a little thinking to resolve, so will you rebase and post v3? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath-windows: BSOD when disabling the extension
On Thu, May 28, 2015 at 08:30:57PM +, Sorin Vinturis wrote: > When the filter detach routine is called while there are packets > still in processing, the OvsUninitSwitchContext function call will > decrement the switch context reference count without releasing the > switch context structure. This behaviour is correct and expected, > but the BSOD is caused in this case because the gOvsSwitchContext > variable is set to NULL, which is wrong. > > The gOvsSwitchContext global variable must be set to NULL only when > the switch context structure is actually released. > > Signed-off-by: Sorin Vinturis > Reported-by: Sorin Vinturis > Reported-at: https://github.com/openvswitch/ovs-issues/issues/80 I don't see any reviews of this yet. Would someone please take a look at it? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] odp-util: Refactor ovs_u128 handling functions.
On Fri, May 29, 2015 at 03:09:05PM -0700, Joe Stringer wrote: > Thanks for review, > > On 29 May 2015 at 13:22, Ben Pfaff wrote: > > On Mon, Apr 13, 2015 at 05:56:15PM -0700, Joe Stringer wrote: > >> Signed-off-by: Joe Stringer > > > > The code in scan_u128() looks wrong to me: I don't see anything that > > makes the second call to ovs_scan(), to get the mask, skip past the > > value, e.g. by passing s + n to the second ovs_scan() or by advancing s > > with s += n. > > You're right, I also used this for conn_label but I didn't attempt to > mask the connlabel. Also, mask doesn't make sense for UFID so it > wasn't tested there. > > > I have another idea too: these issues for ovs_u128 are quite similar to > > the issues for UUIDs, which are also exactly 128 bits long and already > > have support for parsing, formatting, and so on, and furthermore have a > > distinctive format that is easily identifiable. Have you considered > > using UUIDs as the representation for ufids? > > I don't think I'd recognized that UUID was even present. I think it > makes sense to reuse the same for UUID and UFID. > > My other aim was to make this usable by conn_label, which also implies > optional mask. Perhaps it's appropriate to morph this function into a > wrapper for the UUID parsing/formatting, with support for masks. > (Should *all* 128-bit values be printed like UUIDs, or just ID-like > information?) Sorry about my delayed response. Hmm. I see a useful likeness between UUIDs and UFIDs, because they're both essentially random numbers (although a hash is somewhat different from a random UUID). I don't know enough about conn_labels to know whether they're also the same kind of thing. Do they contain structured data (e.g. extracted from a flow) or are they like UUIDs and UFIDs? If conn_labels qualitatively different, then maybe they should not be formatted the same way. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC HSA 2/4 V2] hsa-match: Sparse representation of a byte array derived from "struct match".
On Fri, May 29, 2015 at 11:37:13AM -0700, Alex Wang wrote: > For conducting Header Space Analysis (HSA), we convert the wildcarded > OpenFlow flow represented by 'struct match' into an encoded byte array. > To further save memory, we use a sparse array to represent such byte > array in the same way as 'struct miniflow'. So, this commit implements > the structs and functions for conversion between sparse array and > 'struct match'. > > This commit also implements the set operations (e.g. intersect, union, > complementation and difference) as functions. > > Signed-off-by: Alex Wang > --- > PATCH -> V2: > - Fix sparse warnings. > - Adopt stylistic suggestions. > - Make the hsbm_intersect() and hsbm_init() more efficient. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] odp-util: Refactor ovs_u128 handling functions.
On Tue, Jun 02, 2015 at 10:13:06AM -0700, Joe Stringer wrote: > For UFID, I sent a patch to reuse UUID for UFID: > http://openvswitch.org/pipermail/dev/2015-June/055890.html OK, I'll go look at it now. > I'm currently thinking that for 128-bit maskable bitfields like > conn_label, I'll have something similar to this original patch, which > will represent the value as "conn_label=0xf00bar/0xc0ffee...". That makes sense to me. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] ofproto-dpif: Refactor feature support structure.
On Fri, May 29, 2015 at 03:33:26PM -0700, Joe Stringer wrote:
> On 29 May 2015 at 13:24, Ben Pfaff wrote:
> > On Mon, Apr 13, 2015 at 05:56:16PM -0700, Joe Stringer wrote:
> >> Place all of the detected datapath features into a separate structure,
> >> initialized when the dpif_backer is opened and shared with xbridges.
> >>
> >> Signed-off-by: Joe Stringer
> >
> > It would be nice not to lose the detailed comments in struct xbridge as
> > the members get carried over into the new structure.
> >
> > Acked-by: Ben Pfaff
>
> I thought a bunch of it was redundant, but looking back the additional
> context could be useful. I rolled in the following incremental and
> applied to master:
>
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index f9f62ab..9d625a1 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -84,10 +84,17 @@ struct dpif_backer_support {
> * in a match */
> size_t max_mpls_depth;
>
> -/* True if the datapath supports the corresponding feature. */
> +/* True if the datapath supports masked data in OVS_ACTION_ATTR_SET
> + * actions. */
> bool masked_set_action;
> +
> +/* True if the datapath supports recirculation. */
> bool recirc;
> +
> +/* True if the datapath supports tnl_push and pop actions. */
> bool tnl_push_pop;
> +
> +/* True if the datapath supports OVS_FLOW_ATTR_UFID. */
> bool ufid;
> };
Great, thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] odp-util: Refactor ovs_u128 handling functions.
> On Jun 6, 2015, at 2:13 PM, Ben Pfaff wrote: > > On Fri, May 29, 2015 at 03:09:05PM -0700, Joe Stringer wrote: >> Thanks for review, >> >> On 29 May 2015 at 13:22, Ben Pfaff wrote: >>> On Mon, Apr 13, 2015 at 05:56:15PM -0700, Joe Stringer wrote: Signed-off-by: Joe Stringer >>> >>> The code in scan_u128() looks wrong to me: I don't see anything that >>> makes the second call to ovs_scan(), to get the mask, skip past the >>> value, e.g. by passing s + n to the second ovs_scan() or by advancing s >>> with s += n. >> >> You're right, I also used this for conn_label but I didn't attempt to >> mask the connlabel. Also, mask doesn't make sense for UFID so it >> wasn't tested there. >> >>> I have another idea too: these issues for ovs_u128 are quite similar to >>> the issues for UUIDs, which are also exactly 128 bits long and already >>> have support for parsing, formatting, and so on, and furthermore have a >>> distinctive format that is easily identifiable. Have you considered >>> using UUIDs as the representation for ufids? >> >> I don't think I'd recognized that UUID was even present. I think it >> makes sense to reuse the same for UUID and UFID. >> >> My other aim was to make this usable by conn_label, which also implies >> optional mask. Perhaps it's appropriate to morph this function into a >> wrapper for the UUID parsing/formatting, with support for masks. >> (Should *all* 128-bit values be printed like UUIDs, or just ID-like >> information?) > > Sorry about my delayed response. > > Hmm. I see a useful likeness between UUIDs and UFIDs, because they're > both essentially random numbers (although a hash is somewhat different > from a random UUID). I don't know enough about conn_labels to know > whether they're also the same kind of thing. Do they contain structured > data (e.g. extracted from a flow) or are they like UUIDs and UFIDs? If > conn_labels qualitatively different, then maybe they should not be > formatted the same way. Conn labels is a set of typically individual bits, meaning of which depends on the system/controller configuration. Jarno > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC HSA 4/4] ofprot-dpif-hsa: Implement HSA prototype.
On Sun, May 31, 2015 at 05:35:14PM -0700, Alex Wang wrote:
> > I'm not fully comfortable with assert-failing (killing ovs-vswitchd) if
> > the flow table has unsupported features in it. Also, even when HSA
> > doesn't kill the process due to assert-failing, it could still delay the
> > process by an arbitrary amount of time. Do you have an idea for a way
> > to keep curious admins from accidentally taking down their switch by
> > running an HSA command ("hey, what does this ovs-appctl command do?")?
>
> Sure, the asserts are more for alerting things at development time.
> Will change to VLOG.
Great.
> Also, I can only think of separating the logic info a thread to prevent
> it from blocking the main thread?
I don't know whether that is practical. Is it? If not, then let's not
worry about it for now. If it becomes a problem, we'll come up with
something.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup v3 19/19] ovsdb-monitor: add json cache
On Mon, Jun 01, 2015 at 12:29:28AM -0700, Andy Zhou wrote: > On Fri, May 29, 2015 at 1:05 PM, Ben Pfaff wrote: > > On Thu, Apr 09, 2015 at 06:40:28PM -0700, Andy Zhou wrote: > > The current implementation of ovsdb_monitor_commit() throws away the > > cache whenever there is any commit to the database. However, most > > monitors do not every table and every column. Do you think it would be > > worthwhile flushing the cache only when a column that is included in the > > monitor changes? > > > This is a good idea. However, I don't see a way to easily extend current > patch > to add this patch. So I sent out v4 without this enhancement. I am OK with this patch set without the enhancement; it could be added later. But I don't see why it requires much more work. It seems to me like the code already looks at each of the changes; why not just flush only if one of the changes affects a column that the monitor looks at? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup v4 1/3] ovsdb-monitor: refactor ovsdb_monitor_create()
On Mon, Jun 01, 2015 at 12:30:02AM -0700, Andy Zhou wrote: > Add ovsdb_monitor_add_jsonrpc_monitor(). This change will allow > ovsdb_monitor to be reference counted. > > Signed-off-by: Andy Zhou > Acked-by: Ben Pfaff > > --- > v1->v2: style fixes > v2->v3: no change > v3->v4: Fix a bug that introduced memory leak. With this fix, this patch > becomes trival, and could be merged into the next patch. I opt to > keep it to make the next patch less compliated. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] odp-util: Refactor ovs_u128 handling functions.
On Sat, Jun 06, 2015 at 02:19:03PM -0700, Jarno Rajahalme wrote: > > > On Jun 6, 2015, at 2:13 PM, Ben Pfaff wrote: > > > > On Fri, May 29, 2015 at 03:09:05PM -0700, Joe Stringer wrote: > >> Thanks for review, > >> > >> On 29 May 2015 at 13:22, Ben Pfaff wrote: > >>> On Mon, Apr 13, 2015 at 05:56:15PM -0700, Joe Stringer wrote: > Signed-off-by: Joe Stringer > >>> > >>> The code in scan_u128() looks wrong to me: I don't see anything that > >>> makes the second call to ovs_scan(), to get the mask, skip past the > >>> value, e.g. by passing s + n to the second ovs_scan() or by advancing s > >>> with s += n. > >> > >> You're right, I also used this for conn_label but I didn't attempt to > >> mask the connlabel. Also, mask doesn't make sense for UFID so it > >> wasn't tested there. > >> > >>> I have another idea too: these issues for ovs_u128 are quite similar to > >>> the issues for UUIDs, which are also exactly 128 bits long and already > >>> have support for parsing, formatting, and so on, and furthermore have a > >>> distinctive format that is easily identifiable. Have you considered > >>> using UUIDs as the representation for ufids? > >> > >> I don't think I'd recognized that UUID was even present. I think it > >> makes sense to reuse the same for UUID and UFID. > >> > >> My other aim was to make this usable by conn_label, which also implies > >> optional mask. Perhaps it's appropriate to morph this function into a > >> wrapper for the UUID parsing/formatting, with support for masks. > >> (Should *all* 128-bit values be printed like UUIDs, or just ID-like > >> information?) > > > > Sorry about my delayed response. > > > > Hmm. I see a useful likeness between UUIDs and UFIDs, because they're > > both essentially random numbers (although a hash is somewhat different > > from a random UUID). I don't know enough about conn_labels to know > > whether they're also the same kind of thing. Do they contain structured > > data (e.g. extracted from a flow) or are they like UUIDs and UFIDs? If > > conn_labels qualitatively different, then maybe they should not be > > formatted the same way. > > Conn labels is a set of typically individual bits, meaning of which depends > on the system/controller configuration. That seems quite different from UUID/UFIDs to me then, and I wouldn't want to lump them together just because they're all the same size. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup v4 2/3] ovsdb-monitor: allow multiple jsonrpc monitors to share a single ovsdb monitor
On Mon, Jun 01, 2015 at 12:30:03AM -0700, Andy Zhou wrote: > Store ovsdb monitor in a global hmap. If a newly created ovsdb monitor > object monitors the same tables and columns as an existing one, the > existing monitor will be reused. > > With this patch, jsonrpc monitor and ovsdb monitor now have N:1 mapping. > The goals are to: > 1) Reduce the cost of maintaining duplicated monitors. > 2) Allow for create Json cache for the same updates. Json cache will be > introduced in the following patch. > > Signed-off-by: Andy Zhou In ovsdb_monitor_get_initial(), in the case where we find an existing changes object, do we still need to iterate over all the rows? I think that the changes have to already be composed properly, in that case. In the same function, I think that we could skip the second call to ovsdb_monitor_table_find_changes(), in the "if (!changes)" case, if ovsdb_monitor_table_add_changes() returned the new object. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup v4 3/3] ovsdb-monitor: add json cache
On Mon, Jun 01, 2015 at 12:30:04AM -0700, Andy Zhou wrote: > Although multiple jsonrpc monitors can share the same ovsdb monitor, > each change still needs to translated into json object from scratch. > This can be wasteful if multiple jsonrpc monitors are interested in the > same changes. > > Json cache improves this by keeping an copy of json object generated > for transaction X to current transaction. When jsonrpc is interested > in a change, the cache is searched first, if an json object is found, > a copy of it is handed back, skipping the regeneration process. > > Any commit to the monitor will empty the cache. This can be further > optimized to not throw away the cache if the updated tables and columns > are not being monitored. > > Signed-off-by: Andy Zhou > Acked-by: Ben Pfaff > > --- > v1->v2: Each monitor now has its own json cache. Remove the global > json cache. > Some refatoring to make to code easier to follow. > > v2->v3: Destroy json_cache hmap on monitor destroy. Without it, > valgrind reports memory leak. > > v3->v4: style fixes In ovsdb_monitor_json_cache_flush(), the 'if' check for NULL isn't needed because json_destroy() is a no-op for null pointers. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] odp-util: Make sure vlan tci mask has exact match for VLAN_CFI.
Hey Jarno,
Thx a lot for the comments, and sorry about my careless testing...
I saw for dpif-netdev, the upcall processing also calls
odp_flow_key_from_mask()... but totally missed that the wildcard is
actually obtains from: *wc = upcall.xout.wc; /* in upcall_cb(). */
I'll adopt your suggested changes!
Thanks,
Alex Wang,
On Sat, Jun 6, 2015 at 11:38 AM, Jarno Rajahalme
wrote:
> Alex,
>
> I took a closer look at the test case and it actually verifies that the
> CFI in the mask is NOT set:
>
> +AT_CHECK([cat ovs-vswitchd.log | grep 'in_port=[[1]]' |
> FILTER_FLOW_INSTALL | STRIP_XOUT], [0], [dnl
> +recirc_id=0,ip,in_port=1,vlan_tci=0x100a/0x0fff,nw_frag=no, actions:
>
> Note the mask: 0x0fff!
>
> How about this instead:
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 71b8bef..6bb8518 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5033,6 +5033,10 @@ xlate_actions(struct xlate_in *xin, struct
> xlate_out *xout)
> wc->masks.tp_src &= htons(UINT8_MAX);
> wc->masks.tp_dst &= htons(UINT8_MAX);
> }
> +/* VLAN_TCI CFI bit must be matched if any of the TCI is matched.
> */
> +if (wc->masks.vlan_tci) {
> +wc->masks.vlan_tci |= htons(VLAN_CFI);
> +}
> }
> }
>
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index b5a9ad9..f9015c7 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6673,3 +6673,19 @@
> icmp6,vlan_tci=0x,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +# Tests the exact match of CFI bit in installed datapath flows matching
> VLAN.
> +AT_SETUP([ofproto-dpif - vlan matching])
> +OVS_VSWITCHD_START(
> + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1])
> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
> +
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0
> "vlan_tci=0x000a/0x0fff,action=output:local"])
> +
> +AT_CHECK([ovs-appctl netdev-dummy/receive p0
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x8100),vlan(vid=10,pcp=0),encap(eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0))'])
> +
> +AT_CHECK([cat ovs-vswitchd.log | grep 'in_port=[[1]]' |
> FILTER_FLOW_INSTALL | STRIP_XOUT], [0], [dnl
> +recirc_id=0,ip,in_port=1,dl_vlan=10,nw_frag=no, actions:
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
>
> Jarno
>
> > On Jun 6, 2015, at 10:31 AM, Jarno Rajahalme
> wrote:
> >
> > Alex,
> >
> > I though it surprising that fixing the mask in the netlink attributes
> would fix the problem for the userspace datapath used in the test case, as
> the userspace datapath does not use the netlink attribute format in flow
> setup at all.
> >
> > I tested just the test case without the OVS code change, and the test
> case passes without the fix. Maybe this is due to the fact that we do not
> have the same VLAN_CFI validation in the userspace datapath at all. In
> general we assume in userspace datapath that the wildcards are correct as
> initialized via xlate_actions() during the upcall callback.
> >
> > So, IMO the correct fix is to ensure the mask (‘wc’) has the CFI bit set
> when needed at the end of the ofproto-dpif-xlate.c/xlate_actions(), right
> after the other sanity checks for the wildcards. No change needed in
> odp-util.c.
> >
> > Then, maybe we should add a kernel module testcase to make sure the fix
> works also for linux kernel datapath.
> >
> > I do not think we should start adding validations for the userspace
> datapath, though. After all, the ‘wc’ is now correct again as generated by
> late_actions() :-)
> >
> > Jarno
> >
> >> On Jun 6, 2015, at 8:48 AM, Alex Wang wrote:
> >>
> >> OVS datapath has check which prevents the installation of flow
> >> that matches VLAN TCI but does not have exact match for VLAN_CFI
> >> bit. To follow this rule, ovs userspace must make sure the
> >> flow key for datapath flow matching VLAN TCI has exact match for
> >> VLAN_CFI bit.
> >>
> >> Before this commit, this is not enforced, so OpenFlow flow like
> >> "vlan_tci=0x000a/0x0fff,action=output:local" can generate datapath
> >> flow like "vlan(vid=5/0xfff,pcp=0/0x0,cfi=1/0)".
> >>
> >> With the OVS datapath check, the installation of such datapath
> >> flow will be rejected with:
> >> "|WARN|system@ovs-system: failed to put[create][modify] (Invalid
> argument)"
> >>
> >> This commit makes ovs userspace always exact match the VLAN_CFI
> >> bit if the flow matches VLAN TCI.
> >>
> >> Reported-by: Ronald Lee
> >> Signed-off-by: Alex Wang
> >> ---
> >> lib/odp-util.c|6 +-
> >> tests/ofproto-dpif.at | 16
> >> 2 files changed, 21 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/odp-util.c b/lib/odp-util.c
> >> index 3204d16..63f2660 100644
> >> --- a/lib/odp-util.c
> >> +++ b/lib/odp-util.c
> >> @@ -3486,10 +3486,14 @@
[ovs-dev] [PATCH V2] odp-util: Make sure vlan tci mask has exact match for VLAN_CFI.
OVS datapath has check which prevents the installation of flow
that matches VLAN TCI but does not have exact match for VLAN_CFI
bit. To follow this rule, ovs userspace must make sure the
flow key for datapath flow matching VLAN TCI has exact match for
VLAN_CFI bit.
Before this commit, this is not enforced, so OpenFlow flow like
"vlan_tci=0x000a/0x0fff,action=output:local" can generate datapath
flow like "vlan(vid=5/0xfff,pcp=0/0x0,cfi=1/0)".
With the OVS datapath check, the installation of such datapath
flow will be rejected with:
"|WARN|system@ovs-system: failed to put[create][modify] (Invalid argument)"
This commit makes ovs userspace always exact match the VLAN_CFI
bit if the flow matches VLAN TCI.
Reported-by: Ronald Lee
Signed-off-by: Alex Wang
---
PATCH->V2:
- Fix careless testing.
- Adopt Jarno's suggestions.
---
ofproto/ofproto-dpif-xlate.c |4
tests/ofproto-dpif.at| 16
2 files changed, 20 insertions(+)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 71b8bef..6bb8518 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5033,6 +5033,10 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
*xout)
wc->masks.tp_src &= htons(UINT8_MAX);
wc->masks.tp_dst &= htons(UINT8_MAX);
}
+/* VLAN_TCI CFI bit must be matched if any of the TCI is matched. */
+if (wc->masks.vlan_tci) {
+wc->masks.vlan_tci |= htons(VLAN_CFI);
+}
}
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index b5a9ad9..ef7c5c0 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6673,3 +6673,19 @@
icmp6,vlan_tci=0x,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src
OVS_VSWITCHD_STOP
AT_CLEANUP
+# Tests the exact match of CFI bit in installed datapath flows matching VLAN.
+AT_SETUP([ofproto-dpif - vlan matching])
+OVS_VSWITCHD_START(
+ [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1])
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flow br0 "vlan_tci=0x000a/0x0fff,action=output:local"])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p0
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x8100),vlan(vid=10,pcp=0),encap(eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0))'])
+
+AT_CHECK([cat ovs-vswitchd.log | grep 'in_port=[[1]]' | FILTER_FLOW_INSTALL |
STRIP_XOUT], [0], [dnl
+recirc_id=0,ip,in_port=1,dl_vlan=10,nw_frag=no, actions:
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
\ No newline at end of file
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC HSA 4/4] ofprot-dpif-hsa: Implement HSA prototype.
On Sat, Jun 6, 2015 at 2:20 PM, Ben Pfaff wrote:
> On Sun, May 31, 2015 at 05:35:14PM -0700, Alex Wang wrote:
> > > I'm not fully comfortable with assert-failing (killing ovs-vswitchd) if
> > > the flow table has unsupported features in it. Also, even when HSA
> > > doesn't kill the process due to assert-failing, it could still delay
> the
> > > process by an arbitrary amount of time. Do you have an idea for a way
> > > to keep curious admins from accidentally taking down their switch by
> > > running an HSA command ("hey, what does this ovs-appctl command do?")?
> >
> > Sure, the asserts are more for alerting things at development time.
> > Will change to VLOG.
>
> Great.
>
> > Also, I can only think of separating the logic info a thread to prevent
> > it from blocking the main thread?
>
> I don't know whether that is practical. Is it? If not, then let's not
> worry about it for now. If it becomes a problem, we'll come up with
> something.
>
Yeah, it is really not thoroughly thought about. Will keep exploring
Also, working on adding more tests, and addressing request for a new
function (detect OF flows that will never be used)
Will repost series later,
Thanks,
Alex Wang,
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
