Thanks. I'll take a look at the new version.
On Sat, Nov 22, 2014 at 12:52:35AM +0000, Shu Shen wrote: > 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
