Hi, I'd like to withdraw the previous submission. I'm trying to improve it by splitting into smaller commits.
Please see the first few patches that add tests and fix current bugs related to Packet-In: - [ovs-dev] [PATCH 1/2] ofproto-dpif: add test case for OF1.4 packet-in, http://openvswitch.org/pipermail/dev/2014-November/049093.html - [ovs-dev] [PATCH 2/2] ofproto-dpif: fix OF1.4 packet-in wire protocol version, http://openvswitch.org/pipermail/dev/2014-November/049094.html - [ovs-dev] [PATCH] ofproto: fix checking of packet_in_mask in async config, http://openvswitch.org/pipermail/dev/2014-November/049117.html Support of OFPR_GROUP will come later as another patch. Hope this improvement will help review and increase the chance to be accepted :-). Thanks, Shu -----Original Message----- From: Shu Shen Sent: Thursday, November 13, 2014 9:57 AM To: dev@openvswitch.org Cc: Shu Shen Subject: [PATCH v2] ofproto: support OFPR_GROUP reason for OF1.4 packet-in. Use wire_reason() to translate between different OF versions. OF1.3 and earlier will still use OFPR_ACTION instead of OFPR_GROUP. Async config is initiated and checked againt OF version to make sure OFPR_GROUP is only reported for OF1.4+. Controllers running OF1.3+ cannot override the mask and enable OFPR_GROUP reporting. Signed-off-by: Shu Shen <shu.s...@radisys.com> --- DESIGN.md | 1 + OPENFLOW-1.1+.md | 1 + lib/ofp-msgs.h | 2 +- lib/ofp-util.c | 4 +- ofproto/connmgr.c | 41 +++++++++++++++++++-- ofproto/ofproto-dpif-xlate.c | 2 +- tests/ofproto-dpif.at | 88 ++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 131 insertions(+), 8 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index bd0ed27..ff5bdf4 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -54,6 +54,7 @@ sent, an entry labeled "---" means that the message is suppressed. OFPR_NO_MATCH yes --- OFPR_ACTION yes --- OFPR_INVALID_TTL --- --- + OFPR_GROUP (OF1.4+) yes --- OFPT_FLOW_REMOVED / NXT_FLOW_REMOVED OFPRR_IDLE_TIMEOUT yes --- diff --git a/OPENFLOW-1.1+.md b/OPENFLOW-1.1+.md index 07f7f86..96c25b9 100644 --- a/OPENFLOW-1.1+.md +++ b/OPENFLOW-1.1+.md @@ -180,6 +180,7 @@ OpenFlow 1.4 * More descriptive reasons for packet-in Distinguish OFPR_APPLY_ACTION, OFPR_ACTION_SET, OFPR_GROUP, OFPR_PACKET_OUT. NO_MATCH was renamed to OFPR_TABLE_MISS. + (OFPR_GROUP is now supported) [EXT-136] [required for OF1.4+] diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h index fc69586..ad6dc6f 100644 --- a/lib/ofp-msgs.h +++ b/lib/ofp-msgs.h @@ -148,7 +148,7 @@ enum ofpraw { OFPRAW_OFPT11_PACKET_IN, /* OFPT 1.2 (10): struct ofp12_packet_in, uint8_t[]. */ OFPRAW_OFPT12_PACKET_IN, - /* OFPT 1.3 (10): struct ofp13_packet_in, uint8_t[]. */ + /* OFPT 1.3+ (10): struct ofp13_packet_in, uint8_t[]. */ OFPRAW_OFPT13_PACKET_IN, /* NXT 1.0+ (17): struct nx_packet_in, uint8_t[]. */ OFPRAW_NXT_PACKET_IN, diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 94047fa..86cf8a1 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -3527,7 +3527,7 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin, packet_in_size = sizeof (struct ofp12_packet_in); } else { packet_in_raw = OFPRAW_OFPT13_PACKET_IN; - packet_in_version = OFP13_VERSION; + packet_in_version = ofputil_protocol_to_ofp_version(protocol); packet_in_size = sizeof (struct ofp13_packet_in); } @@ -3547,7 +3547,7 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin, opi->pi.total_len = htons(pin->total_len); opi->pi.reason = pin->reason; opi->pi.table_id = pin->table_id; - if (protocol == OFPUTIL_P_OF13_OXM) { + if (protocol & OFPUTIL_P_OF13_UP) { opi->cookie = pin->cookie; } diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index 627f326..25a52fc 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -996,6 +996,14 @@ void ofconn_set_protocol(struct ofconn *ofconn, enum ofputil_protocol protocol) { ofconn->protocol = protocol; + if (!(protocol & OFPUTIL_P_OF14_UP)) { + uint32_t *master = ofconn->master_async_config; + uint32_t *slave = ofconn->slave_async_config; + + /* OFPR_GROUP is not supported before OF1.4 */ + master[OAM_PACKET_IN] &= ~(1u << OFPR_GROUP); + slave [OAM_PACKET_IN] &= ~(1u << OFPR_GROUP); + } } /* Returns the currently configured packet in format for 'ofconn', one of @@ -1056,6 +1064,13 @@ ofconn_get_async_config(struct ofconn *ofconn, uint32_t *master_masks, uint32_t *slave_masks) { size_t size = sizeof ofconn->master_async_config; + + /* Make sure we know the protocol version and the async_config + * masks are properly updated by calling ofconn_get_protocol() */ + if (OFPUTIL_P_NONE == ofconn_get_protocol(ofconn)){ + OVS_NOT_REACHED(); + } + memcpy(master_masks, ofconn->master_async_config, size); memcpy(slave_masks, ofconn->slave_async_config, size); } @@ -1235,7 +1250,9 @@ ofconn_flush(struct ofconn *ofconn) /* "master" and "other" roles get all asynchronous messages by default, * except that the controller needs to enable nonstandard "packet-in" * reasons itself. */ - master[OAM_PACKET_IN] = (1u << OFPR_NO_MATCH) | (1u << OFPR_ACTION); + master[OAM_PACKET_IN] = ((1u << OFPR_NO_MATCH) + | (1u << OFPR_ACTION) + | (1u << OFPR_GROUP)); master[OAM_PORT_STATUS] = ((1u << OFPPR_ADD) | (1u << OFPPR_DELETE) | (1u << OFPPR_MODIFY)); @@ -1651,16 +1668,32 @@ connmgr_send_flow_removed(struct connmgr *mgr, static enum ofp_packet_in_reason wire_reason(struct ofconn *ofconn, const struct ofproto_packet_in *pin) { + enum ofputil_protocol protocol = ofconn_get_protocol(ofconn); + if (pin->miss_type == OFPROTO_PACKET_IN_MISS_FLOW && pin->up.reason == OFPR_ACTION) { - enum ofputil_protocol protocol = ofconn_get_protocol(ofconn); if (protocol != OFPUTIL_P_NONE && ofputil_protocol_to_ofp_version(protocol) >= OFP13_VERSION) { return OFPR_NO_MATCH; } } - return pin->up.reason; + + switch (pin->up.reason) { + case OFPR_ACTION_SET: + case OFPR_GROUP: + case OFPR_PACKET_OUT: + if (!(protocol & OFPUTIL_P_OF14_UP)) { + /* Only supported in OF1.4+ */ + return OFPR_ACTION; + } /* else fall through */ + case OFPR_NO_MATCH: + case OFPR_ACTION: + case OFPR_INVALID_TTL: + case OFPR_N_REASONS: + default: + return pin->up.reason; + } } /* Given 'pin', sends an OFPT_PACKET_IN message to each OpenFlow controller as @@ -1677,7 +1710,7 @@ connmgr_send_packet_in(struct connmgr *mgr, enum ofp_packet_in_reason reason = wire_reason(ofconn, pin); if (ofconn_wants_packet_in_on_miss(ofconn, pin) - && ofconn_receives_async_msg(ofconn, OAM_PACKET_IN, pin->up.reason) + && ofconn_receives_async_msg(ofconn, OAM_PACKET_IN, reason) && ofconn->controller_id == pin->controller_id) { schedule_packet_in(ofconn, *pin, reason); } diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index f781bc5..c573c80 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3397,7 +3397,7 @@ xlate_output_action(struct xlate_ctx *ctx, flood_packets(ctx, true); break; case OFPP_CONTROLLER: - execute_controller_action(ctx, max_len, OFPR_ACTION, 0); + execute_controller_action(ctx, max_len, ctx->in_group?OFPR_GROUP:OFPR_ACTION, 0); break; case OFPP_NONE: break; diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 5349386..33be9d0 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -2652,6 +2652,94 @@ OFPST_FLOW reply (OF1.3): OVS_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([ofproto-dpif - packet-in reason in group table (Openflow 1.3)]) +OVS_VSWITCHD_START([dnl + add-port br0 p1 -- set Interface p1 type=dummy +]) +ON_EXIT([kill `cat ovs-ofctl.pid`]) + +AT_CAPTURE_FILE([ofctl_monitor.log]) +# A table-miss flow has priority 0 and no match +AT_CHECK([ovs-ofctl --protocols=OpenFlow13 add-group br0 'group_id=1234,type=all,bucket=output:10,bucket=output:CONTROLLER']) +AT_CHECK([ovs-ofctl --protocols=OpenFlow13 add-flow br0 'in_port=1 actions=group:1234']) + +dnl Singleton controller action. +AT_CHECK([ovs-ofctl monitor -P openflow10 --protocols=OpenFlow13 br0 65534 --detach --no-chdir --pidfile 2> ofctl_monitor.log]) + +for i in 1 2 3 ; do + ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=10:11:11:11:11:11,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=10),tcp_flags(0x002)' +done +OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6]) +ovs-appctl -t ovs-ofctl exit + +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore]) + +AT_CHECK([cat ofctl_monitor.log], [0], [dnl +OFPT_PACKET_IN (OF1.3) (xid=0x0): cookie=0x0 total_len=60 in_port=1 (via action) data_len=60 (unbuffered) +tcp,in_port=0,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn tcp_csum:0 +dnl +OFPT_PACKET_IN (OF1.3) (xid=0x0): cookie=0x0 total_len=60 in_port=1 (via action) data_len=60 (unbuffered) +tcp,in_port=0,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn tcp_csum:0 +dnl +OFPT_PACKET_IN (OF1.3) (xid=0x0): cookie=0x0 total_len=60 in_port=1 (via action) data_len=60 (unbuffered) +tcp,in_port=0,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn tcp_csum:0 +]) + +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore]) + +AT_CHECK([ovs-ofctl --protocols=OpenFlow13 dump-flows br0 | ofctl_strip | sort], [0], [dnl + n_packets=3, n_bytes=180, in_port=1 actions=group:1234 +OFPST_FLOW reply (OF1.3): +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + + +AT_SETUP([ofproto-dpif - packet-in reason in group table (Openflow 1.4)]) +OVS_VSWITCHD_START([dnl + add-port br0 p1 -- set Interface p1 type=dummy +]) +ON_EXIT([kill `cat ovs-ofctl.pid`]) + +AT_CAPTURE_FILE([ofctl_monitor.log]) +# A table-miss flow has priority 0 and no match +AT_CHECK([ovs-ofctl --protocols=OpenFlow14 add-group br0 'group_id=1234,type=all,bucket=output:10,bucket=output:CONTROLLER']) +AT_CHECK([ovs-ofctl --protocols=OpenFlow14 add-flow br0 'in_port=1 actions=group:1234']) + +dnl Singleton controller action. +AT_CHECK([ovs-ofctl monitor -P openflow10 --protocols=OpenFlow14 br0 65534 --detach --no-chdir --pidfile 2> ofctl_monitor.log]) + +for i in 1 2 3 ; do + ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=10:11:11:11:11:11,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=10),tcp_flags(0x002)' +done +OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6]) +ovs-appctl -t ovs-ofctl exit + +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore]) + +AT_CHECK([cat ofctl_monitor.log], [0], [dnl +OFPT_PACKET_IN (OF1.4) (xid=0x0): cookie=0x0 total_len=60 in_port=1 (via group) data_len=60 (unbuffered) +tcp,in_port=0,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn tcp_csum:0 +dnl +OFPT_PACKET_IN (OF1.4) (xid=0x0): cookie=0x0 total_len=60 in_port=1 (via group) data_len=60 (unbuffered) +tcp,in_port=0,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn tcp_csum:0 +dnl +OFPT_PACKET_IN (OF1.4) (xid=0x0): cookie=0x0 total_len=60 in_port=1 (via group) data_len=60 (unbuffered) +tcp,in_port=0,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn tcp_csum:0 +]) + +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore]) + +AT_CHECK([ovs-ofctl --protocols=OpenFlow14 dump-flows br0 | ofctl_strip | sort], [0], [dnl + n_packets=3, n_bytes=180, in_port=1 actions=group:1234 +OFPST_FLOW reply (OF1.4): +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - ARP modification slow-path]) OVS_VSWITCHD_START ADD_OF_PORTS([br0], [1], [2]) -- 1.9.1 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
