http://blog.gmane.org/gmane.network.openvswitch.devel hourly 1 1901-01-01T00:00+00:00 http://gmane.org/img/gmane-25t.png http://gmane.org http://permalink.gmane.org/gmane.network.openvswitch.devel/20721 <pre> Looks good to me. Thank you for finding the problem and fixing it! </pre> Ben Pfaff 2013-05-20T17:12:26 http://permalink.gmane.org/gmane.network.openvswitch.devel/20720 <pre>It has been observed that when we upgrade openvswitch with thousands of openflow flows already established and there is heavy traffic going through the switch, restoring flows can take a very long time (8 minutes in one use case) with CPU running at 100%. This can make the upgrade very expensive. This commit starts vswitchd with packet processing from the datapath disabled. Once the flows are restored, it enables packet processing. Bug #16086. Signed-off-by: Gurucharan Shetty &lt;gshetty-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; --- utilities/ovs-ctl.in | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in index 1f10491..127cd1b 100755 --- a/utilities/ovs-ctl.in +++ b/utilities/ovs-ctl.in &lt; at &gt;&lt; at &gt; -330,6 +330,11 &lt; at &gt;&lt; at &gt; init_restore_scripts () { trap 'rm -f "${script_interfaces}" "${script_flows}" "${script_ofports}"' 0 } +dpif_packet_receive () { + # Setting to enable or disable packet processing from datapath. + ovs_vsctl set open_vswitch . other_config:dpif-packet-receive="$1" +} + force_reload_kmod () { ifaces=`internal_interfaces` action "Detected internal interfaces: $ifaces" true &lt; at &gt;&lt; at &gt; -370,9 +375,13 &lt; at &gt;&lt; at &gt; force_reload_kmod () { action "Removing openvswitch module" rmmod openvswitch fi + # Start vswitchd with packet processing from datapath disabled. + dpif_packet_receive "false" start_forwarding + # Restore saved flows and then enable packet processing from the datapath. restore_flows + dpif_packet_receive "true" restore_interfaces &lt; at &gt;&lt; at &gt; -414,10 +423,14 &lt; at &gt;&lt; at &gt; restart () { restore_ofports stop_forwarding + + # Start vswitchd with packet processing from datapath disabled. + dpif_packet_receive "false" start_forwarding - # Restore the saved flows. + # Restore saved flows and then enable packet processing from the datapath. restore_flows + dpif_packet_receive "true" # Restore the interfaces if required. Return true even if restore fails. restore_interfaces || true </pre> Gurucharan Shetty 2013-05-20T06:53:15 http://permalink.gmane.org/gmane.network.openvswitch.devel/20719 <pre>This commit provides an option to enable or disable packet processing coming from the datapath. This option is useful during Open vSwitch upgrades. Typically we want to restart openvswitch, add the openflow flows and then start packet processing. The next commit will use these commands in Open vSwitch startup scripts. Bug #16086. Signed-off-by: Gurucharan Shetty &lt;gshetty-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; --- ofproto/ofproto-dpif.c | 21 ++++++++++++++++++++- vswitchd/bridge.c | 7 +++++++ vswitchd/vswitch.xml | 9 +++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index c4f7d25..a31b186 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c &lt; at &gt;&lt; at &gt; -72,6 +72,8 &lt; at &gt;&lt; at &gt; COVERAGE_DEFINE(facet_suppress); * flow translation. */ #define MAX_RESUBMIT_RECURSION 64 +extern bool recv_set_enable; + /* Number of implemented OpenFlow tables. */ enum { N_TABLES = 255 }; enum { TBL_INTERNAL = N_TABLES - 1 }; /* Used for internal hidden rules. */ &lt; at &gt;&lt; at &gt; -671,6 +673,7 &lt; at &gt;&lt; at &gt; struct dpif_backer { struct tag_set revalidate_set; /* Revalidate only matching facets. */ struct hmap drop_keys; /* Set of dropped odp keys. */ + bool recv_set_enable; /* Enables or disables receiving packets. */ }; /* All existing ofproto_backer instances, indexed by ofproto-&gt;up.type. */ &lt; at &gt;&lt; at &gt; -937,6 +940,21 &lt; at &gt;&lt; at &gt; type_run(const char *type) dpif_run(backer-&gt;dpif); + /* Enable or disable receiving packets on 'dpif' based on user + * configuration. */ + if (backer-&gt;recv_set_enable != recv_set_enable) { + backer-&gt;recv_set_enable = recv_set_enable; + error = dpif_recv_set(backer-&gt;dpif, recv_set_enable); + if (error) { + VLOG_ERR("Failed to %s receiving packets in dpif", + recv_set_enable ? "enable" : "disable"); + return error; + } + if (recv_set_enable) { + backer-&gt;need_revalidate = REV_RECONFIGURE; + } + } + /* The most natural place to push facet statistics is when they're pulled * from the datapath. However, when there are many flows in the datapath, * this expensive operation can occur so frequently, that it reduces our &lt; at &gt;&lt; at &gt; -1331,7 +1349,8 &lt; at &gt;&lt; at &gt; open_dpif_backer(const char *type, struct dpif_backer **backerp) shash_add(&amp;all_dpif_backers, type, backer); - error = dpif_recv_set(backer-&gt;dpif, true); + backer-&gt;recv_set_enable = recv_set_enable; + error = dpif_recv_set(backer-&gt;dpif, recv_set_enable); if (error) { VLOG_ERR("failed to listen on datapath of type %s: %s", type, strerror(error)); diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index e10036c..80e22fb 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c &lt; at &gt;&lt; at &gt; -183,6 +183,9 &lt; at &gt;&lt; at &gt; static long long int iface_stats_timer = LLONG_MIN; #define OFP_PORT_ACTION_WINDOW 10 static bool reconfiguring = false; +/* The default value of true enables receving packets on 'dpif'. */ +bool recv_set_enable = true; + static void add_del_bridges(const struct ovsrec_open_vswitch *); static void bridge_update_ofprotos(void); static void bridge_create(const struct ovsrec_bridge *); &lt; at &gt;&lt; at &gt; -2321,6 +2324,10 &lt; at &gt;&lt; at &gt; bridge_run(void) * returns immediately. */ bridge_init_ofproto(cfg); + /* Enables or disables receiving packets on 'dpif'. */ + recv_set_enable = smap_get_bool(&amp;cfg-&gt;other_config, + "dpif-packet-receive", true); + /* Let each datapath type do the work that it needs to do. */ sset_init(&amp;types); ofproto_enumerate_types(&amp;types); diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 4396779..fa033f6 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml &lt; at &gt;&lt; at &gt; -104,6 +104,15 &lt; at &gt;&lt; at &gt; column or to &lt;code&gt;false&lt;/code&gt; to explicitly disable it. &lt;/column&gt; + &lt;column name="other_config" key="dpif-packet-receive" + type='{"type": "boolean"}'&gt; + Packets are received from the datapath by default. Set this value to + &lt;code&gt;false&lt;/code&gt; to stop receiving packets from the datapath and + &lt;code&gt;true&lt;/code&gt; to start receiving packets. This option is useful + during openvswitch upgrades and is used to restore flows before + processing the packets. + &lt;/column&gt; + &lt;column name="statistics" key="cpu" type='{"type": "integer", "minInteger": 1}'&gt; &lt;p&gt; </pre> Gurucharan Shetty 2013-05-20T06:53:14 http://permalink.gmane.org/gmane.network.openvswitch.devel/20718 <pre>For xenservers with version less than 6.1, interface reconfiguration happened through interface-reconfigure scripts in this repo. In cases where there were multiple xs-network-uuids for a single bridge, interface-reconfigure script would add the network uuid associated with the non-VLAN network as the first record. ovs-xapi-sync would just blindly use the first record to create the bridge-id But it looks like for xenserver 6.1, interface-reconfigure script is no longer used and xenserver natively writes the xs-network-uuids. So, in ovs-xapi-sync we no longer can copy the first value in xs-network-uuids as bridge-id. This commit fetches the PIF record for each xs-network-uuids and the network that does not have a VLAN associated with it is copied over to bridge-id. Bug #17090. Signed-off-by: Gurucharan Shetty &lt;gshetty-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; --- .../usr_share_openvswitch_scripts_ovs-xapi-sync | 22 ++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync index cb35e7a..e14b319 100755 --- a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync +++ b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync &lt; at &gt;&lt; at &gt; -88,6 +88,25 &lt; at &gt;&lt; at &gt; def get_network_by_bridge(br_name): return None +# There are possibilities when multiple xs-network-uuids are set for a bridge. +# In cases like that, we should choose the bridge-id whose PIF does not have a +# VLAN associated with it. +def get_single_bridge_id(bridge_ids, default=None): + if not init_session(): + vlog.warn("Failed to get single bridge id from %s because" + "XAPI session could not be initialized" % bridge_ids) + return default + + for bridge_id in bridge_ids: + recs = session.xenapi.network.get_all_records_where('field "uuid"="%s"' % bridge_id) + if recs: + pifs = recs.values()[0]['PIFs'] + for pif in pifs: + rec = session.xenapi.PIF.get_record(pif) + if rec['VLAN'] == '-1': + return bridge_id + + return default # By default, the "bridge-id" external id in the Bridge table is the # same as "xs-network-uuids". This may be overridden by defining a &lt; at &gt;&lt; at &gt; -286,6 +305,9 &lt; at &gt;&lt; at &gt; def main(): bridge_id = nbd if bridge_id is None: bridge_id = row.external_ids.get("xs-network-uuids") + if bridge_id and len(bridge_id.split(";")) &gt; 1: + bridge_ids = bridge_id.split(";") + bridge_id = get_single_bridge_id(bridge_ids, bridge_ids[0]) if bridge_id is not None: set_external_id(row, "bridge-id", bridge_id.split(";")[0]) </pre> Gurucharan Shetty 2013-05-20T14:56:16 http://permalink.gmane.org/gmane.network.openvswitch.devel/20713 <pre>Ben, On my reading the first three patches seem fine. I thought that the fact that the semantics of the struct flow_metadata has changed from "exportable metadata" to "all metadata, some of which is exportable to controllers" deserves a note somewhere, but then the original comment on flow_metadata is even more valid after these changes ("Represents the metadata fields of struct flow."). I don't think there has been a decision to not make the hole in flow_tnl explicit, even though existing code relies on them being cleared (as in commit_odp_tunnel_action()). This might not be as clean as you'd like, but would it be portable to repeat the definition of flow_metadata as the first fields of the struct flow (maybe via a macro) and then cast a struct flow * to a struct flow_metadata * when needed? Jarno On May 16, 2013, at 23:44 , ext Ben Pfaff wrote: </pre> Rajahalme, Jarno (NSN - FI/Espoo 2013-05-19T13:45:32 http://permalink.gmane.org/gmane.network.openvswitch.devel/20712 <pre>This helps get rid of one special case in nx_pull_raw() and allows loading of 32-bit values from/to OXM_OF_IN_PORT in NXAST_LEARN actions. Previously the 16-bit limit acted the same on both NXM_OF_IN_PORT and OXM_OF_IN_PORT, even though OF1.1+ controllers would expect OXM_OF_IN_PORT to be 32 bits wide. Signed-off-by: Jarno Rajahalme &lt;jarno.rajahalme-OYasijW0DpE&lt; at &gt;public.gmane.org&gt; --- v2: Avoid duplicating existing code. lib/meta-flow.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/meta-flow.h | 2 ++ lib/nx-match.c | 13 +--------- tests/learn.at | 22 +++++++++++++++++ 4 files changed, 96 insertions(+), 12 deletions(-) diff --git a/lib/meta-flow.c b/lib/meta-flow.c index 16850ec..c59d82a 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c &lt; at &gt;&lt; at &gt; -116,6 +116,15 &lt; at &gt;&lt; at &gt; static const struct mf_field mf_fields[MFF_N_IDS] = { MFP_NONE, true, NXM_OF_IN_PORT, "NXM_OF_IN_PORT", + NXM_OF_IN_PORT, "NXM_OF_IN_PORT", + }, { + MFF_IN_PORT_OXM, "in_port_oxm", NULL, + MF_FIELD_SIZES(be32), + MFM_NONE, + MFS_OFP_PORT_OXM, + MFP_NONE, + true, + OXM_OF_IN_PORT, "OXM_OF_IN_PORT", OXM_OF_IN_PORT, "OXM_OF_IN_PORT", }, { MFF_SKB_PRIORITY, "skb_priority", NULL, &lt; at &gt;&lt; at &gt; -688,6 +697,7 &lt; at &gt;&lt; at &gt; mf_is_all_wild(const struct mf_field *mf, const struct flow_wildcards *wc) case MFF_METADATA: return !wc-&gt;masks.metadata; case MFF_IN_PORT: + case MFF_IN_PORT_OXM: return !wc-&gt;masks.in_port; case MFF_SKB_PRIORITY: return !wc-&gt;masks.skb_priority; &lt; at &gt;&lt; at &gt; -926,6 +936,11 &lt; at &gt;&lt; at &gt; mf_is_value_valid(const struct mf_field *mf, const union mf_value *value) case MFF_ND_TLL: return true; + case MFF_IN_PORT_OXM: { + uint16_t port; + return !ofputil_port_from_ofp11(value-&gt;be32, &amp;port); + } + case MFF_IP_DSCP: return !(value-&gt;u8 &amp; ~IP_DSCP_MASK); case MFF_IP_DSCP_SHIFTED: &lt; at &gt;&lt; at &gt; -999,6 +1014,10 &lt; at &gt;&lt; at &gt; mf_get_value(const struct mf_field *mf, const struct flow *flow, value-&gt;be16 = htons(flow-&gt;in_port); break; + case MFF_IN_PORT_OXM: + value-&gt;be32 = ofputil_port_to_ofp11(flow-&gt;in_port); + break; + case MFF_SKB_PRIORITY: value-&gt;be32 = htonl(flow-&gt;skb_priority); break; &lt; at &gt;&lt; at &gt; -1182,6 +1201,15 &lt; at &gt;&lt; at &gt; mf_set_value(const struct mf_field *mf, match_set_in_port(match, ntohs(value-&gt;be16)); break; + case MFF_IN_PORT_OXM: { + uint16_t port; + if (ofputil_port_from_ofp11(value-&gt;be32, &amp;port)) { + port = OFPP_NONE; + } + match_set_in_port(match, port); + break; + } + case MFF_SKB_PRIORITY: match_set_skb_priority(match, ntohl(value-&gt;be32)); break; &lt; at &gt;&lt; at &gt; -1365,6 +1393,15 &lt; at &gt;&lt; at &gt; mf_set_flow_value(const struct mf_field *mf, flow-&gt;in_port = ntohs(value-&gt;be16); break; + case MFF_IN_PORT_OXM: { + uint16_t port; + if (ofputil_port_from_ofp11(value-&gt;be32, &amp;port)) { + port = OFPP_NONE; + } + flow-&gt;in_port = port; + break; + } + case MFF_SKB_PRIORITY: flow-&gt;skb_priority = ntohl(value-&gt;be32); break; &lt; at &gt;&lt; at &gt; -1561,6 +1598,7 &lt; at &gt;&lt; at &gt; mf_set_wild(const struct mf_field *mf, struct match *match) break; case MFF_IN_PORT: + case MFF_IN_PORT_OXM: match-&gt;flow.in_port = 0; match-&gt;wc.masks.in_port = 0; break; &lt; at &gt;&lt; at &gt; -1742,6 +1780,7 &lt; at &gt;&lt; at &gt; mf_set(const struct mf_field *mf, switch (mf-&gt;id) { case MFF_IN_PORT: + case MFF_IN_PORT_OXM: case MFF_SKB_MARK: case MFF_SKB_PRIORITY: case MFF_ETH_TYPE: &lt; at &gt;&lt; at &gt; -1977,6 +2016,10 &lt; at &gt;&lt; at &gt; mf_random_value(const struct mf_field *mf, union mf_value *value) case MFF_ND_TLL: break; + case MFF_IN_PORT_OXM: + value-&gt;be32 = ofputil_port_to_ofp11(ntohs(value-&gt;be16)); + break; + case MFF_IPV6_LABEL: value-&gt;be32 &amp;= ~htonl(IPV6_LABEL_MASK); break; &lt; at &gt;&lt; at &gt; -2177,6 +2220,21 &lt; at &gt;&lt; at &gt; mf_from_ofp_port_string(const struct mf_field *mf, const char *s, return xasprintf("%s: port value out of range for %s", s, mf-&gt;name); } +static char * +mf_from_ofp_port_string32(const struct mf_field *mf, const char *s, + ovs_be32 *valuep, ovs_be32 *maskp) +{ + uint16_t port; + + ovs_assert(mf-&gt;n_bytes == sizeof(ovs_be32)); + if (ofputil_port_from_string(s, &amp;port)) { + *valuep = ofputil_port_to_ofp11(port); + *maskp = htonl(UINT32_MAX); + return NULL; + } + return xasprintf("%s: port value out of range for %s", s, mf-&gt;name); +} + struct frag_handling { const char *name; uint8_t mask; &lt; at &gt;&lt; at &gt; -2314,6 +2372,9 &lt; at &gt;&lt; at &gt; mf_parse(const struct mf_field *mf, const char *s, case MFS_OFP_PORT: return mf_from_ofp_port_string(mf, s, &amp;value-&gt;be16, &amp;mask-&gt;be16); + case MFS_OFP_PORT_OXM: + return mf_from_ofp_port_string32(mf, s, &amp;value-&gt;be32, &amp;mask-&gt;be32); + case MFS_FRAG: return mf_from_frag_string(s, &amp;value-&gt;u8, &amp;mask-&gt;u8); &lt; at &gt;&lt; at &gt; -2417,6 +2478,16 &lt; at &gt;&lt; at &gt; mf_format(const struct mf_field *mf, } switch (mf-&gt;string) { + case MFS_OFP_PORT_OXM: + if (!mask) { + uint16_t port; + if (ofputil_port_from_ofp11(value-&gt;be32, &amp;port)) { + port = OFPP_NONE; + } + ofputil_format_port(port, s); + break; + } + /* fall through */ case MFS_OFP_PORT: if (!mask) { ofputil_format_port(ntohs(value-&gt;be16), s); diff --git a/lib/meta-flow.h b/lib/meta-flow.h index 9577a10..a85a193 100644 --- a/lib/meta-flow.h +++ b/lib/meta-flow.h &lt; at &gt;&lt; at &gt; -39,6 +39,7 &lt; at &gt;&lt; at &gt; enum mf_field_id { MFF_TUN_TOS, /* u8 */ MFF_METADATA, /* be64 */ MFF_IN_PORT, /* be16 */ + MFF_IN_PORT_OXM, /* be32 */ MFF_SKB_PRIORITY, /* be32 */ MFF_SKB_MARK, /* be32 */ &lt; at &gt;&lt; at &gt; -220,6 +221,7 &lt; at &gt;&lt; at &gt; enum mf_string { MFS_IPV4, MFS_IPV6, MFS_OFP_PORT, /* An OpenFlow port number or name. */ + MFS_OFP_PORT_OXM, /* An OpenFlow port number or name (32-bit). */ MFS_FRAG, /* no, yes, first, later, not_later */ MFS_TNL_FLAGS, /* FLOW_TNL_F_* flags */ }; diff --git a/lib/nx-match.c b/lib/nx-match.c index 51b6dc6..a91d56b 100644 --- a/lib/nx-match.c +++ b/lib/nx-match.c &lt; at &gt;&lt; at &gt; -148,7 +148,7 &lt; at &gt;&lt; at &gt; nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict, error = OFPERR_OFPBMC_BAD_PREREQ; } else if (!mf_is_all_wild(mf, &amp;match-&gt;wc)) { error = OFPERR_OFPBMC_DUP_FIELD; - } else if (header != OXM_OF_IN_PORT) { + } else { unsigned int width = mf-&gt;n_bytes; union mf_value value; &lt; at &gt;&lt; at &gt; -170,17 +170,6 &lt; at &gt;&lt; at &gt; nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict, mf_set(mf, &amp;value, &amp;mask, match); } } - } else { - /* Special case for 32bit ports when using OXM, - * ports are 16 bits wide otherwise. */ - ovs_be32 port_of11; - uint16_t port; - - memcpy(&amp;port_of11, p + 4, sizeof port_of11); - error = ofputil_port_from_ofp11(port_of11, &amp;port); - if (!error) { - match_set_in_port(match, port); - } } /* Check if the match is for a cookie rather than a classifier rule. */ diff --git a/tests/learn.at b/tests/learn.at index 5eb3e5d..964d21d 100644 --- a/tests/learn.at +++ b/tests/learn.at &lt; at &gt;&lt; at &gt; -15,6 +15,28 &lt; at &gt;&lt; at &gt; OFPT_FLOW_MOD (xid=0x3): ADD actions=learn(table=1,idle_timeout=10,hard_timeout= ]]) AT_CLEANUP +AT_SETUP([learning action - parsing and formatting - illegal in_port_oxm]) +AT_CHECK([[ovs-ofctl parse-flow 'actions=learn(table=1, in_port_oxm=123456)']], + [1], [], [stderr]) +AT_CHECK([sed -e 's/.*|ofp_util|WARN|//' &lt; stderr], [0], + [[port 123456 is outside the supported range 0 through ffff or 0xffffff00 through 0xffffffff +ovs-ofctl: 123456: port value out of range for in_port_oxm +]], [[]]) +AT_CLEANUP + +AT_SETUP([learning action - parsing and formatting - OXM]) +AT_DATA([flows.txt], [[ +actions=learn(output:OXM_OF_IN_PORT[]) +actions=learn(table=1, in_port=1, load:OXM_OF_IN_PORT[]-&gt;NXM_NX_REG1[], load:0xfffffffe-&gt;OXM_OF_IN_PORT[]) +]]) +AT_CHECK([ovs-ofctl parse-flows flows.txt], [0], +[[usable protocols: any +chosen protocol: OpenFlow10-table_id +OFPT_FLOW_MOD (xid=0x1): ADD actions=learn(table=1,output:OXM_OF_IN_PORT[]) +OFPT_FLOW_MOD (xid=0x2): ADD actions=learn(table=1,in_port=1,load:OXM_OF_IN_PORT[]-&gt;NXM_NX_REG1[],load:0xfffffffe-&gt;OXM_OF_IN_PORT[]) +]]) +AT_CLEANUP + AT_SETUP([learning action - examples]) AT_DATA([flows.txt], [[ # These are the examples from nicira-ext.h. </pre> Jarno Rajahalme 2013-05-19T12:44:53 http://permalink.gmane.org/gmane.network.openvswitch.devel/20711 <pre>There is a typo in the "bundle_add_port()" function header. This patch fixes it. Signed-off-by: Alex Wang &lt;alexw-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; --- ofproto/ofproto-dpif.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index c4f7d25..5cc56d1 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c &lt; at &gt;&lt; at &gt; -2472,7 +2472,7 &lt; at &gt;&lt; at &gt; bundle_del_port(struct ofport_dpif *port) } static bool -bundle_add_port(struct ofbundle *bundle, uint32_t ofp_port, +bundle_add_port(struct ofbundle *bundle, uint16_t ofp_port, struct lacp_slave_settings *lacp) { struct ofport_dpif *port; </pre> Alex Wang 2013-05-19T04:33:25 http://permalink.gmane.org/gmane.network.openvswitch.devel/20710 <pre> Thank you for the review. I applied this to master. </pre> Ben Pfaff 2013-05-18T15:30:44 http://permalink.gmane.org/gmane.network.openvswitch.devel/20709 <pre> Acked-by: Ed Maste &lt;emaste-h+KGxgPPiopAfugRpC6u6w&lt; at &gt;public.gmane.org&gt; Looks good to me, and unit tests continue to pass on FreeBSD. </pre> Ed Maste 2013-05-18T04:12:12 http://permalink.gmane.org/gmane.network.openvswitch.devel/20707 <pre>There's no particular reason that netdev_dummy_register() has to care about the particular OS, except that the tests like to use the special Linux-only tunnel vport types. But that can be done better, I think, by just always registering them from netdev_dummy_register() and making that function idempotent, so that calling it twice under Linux has no additional effect. This commit implements that solution. CC: Ed Maste &lt;emaste-h+KGxgPPiopAfugRpC6u6w&lt; at &gt;public.gmane.org&gt; Signed-off-by: Ben Pfaff &lt;blp-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; --- lib/netdev-dummy.c | 10 +--------- lib/netdev-vport.c | 8 ++++++-- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 908fef2..14b286b 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c &lt; at &gt;&lt; at &gt; -36,12 +36,6 &lt; at &gt;&lt; at &gt; VLOG_DEFINE_THIS_MODULE(netdev_dummy); -#ifdef __FreeBSD__ -#define FREE_BSD 1 -#else -#define FREE_BSD 0 -#endif - struct netdev_dummy { struct netdev up; uint8_t hwaddr[ETH_ADDR_LEN]; &lt; at &gt;&lt; at &gt; -553,7 +547,5 &lt; at &gt;&lt; at &gt; netdev_dummy_register(bool override) } netdev_register_provider(&amp;dummy_class); - if (FREE_BSD) { - netdev_vport_tunnel_register(); - } + netdev_vport_tunnel_register(); } diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 38fc996..6eee8a7 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c &lt; at &gt;&lt; at &gt; -685,11 +685,15 &lt; at &gt;&lt; at &gt; netdev_vport_tunnel_register(void) TUNNEL_CLASS("vxlan", "vxlan_system"), TUNNEL_CLASS("lisp", "lisp_system") }; + static bool inited; int i; - for (i = 0; i &lt; ARRAY_SIZE(vport_classes); i++) { - netdev_register_provider(&amp;vport_classes[i].netdev_class); + if (!inited) { + inited = true; + for (i = 0; i &lt; ARRAY_SIZE(vport_classes); i++) { + netdev_register_provider(&amp;vport_classes[i].netdev_class); + } } } </pre> Ben Pfaff 2013-05-17T22:57:20 http://permalink.gmane.org/gmane.network.openvswitch.devel/20706 <pre>Signed-off-by: Ben Pfaff &lt;blp-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; --- Makefile.am | 1 + NEWS | 2 + README-OFTest | 77 +++++++++++++++++++++++++++++++++++++++++++ tests/automake.mk | 6 +++ tests/run-oftest | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 180 insertions(+), 0 deletions(-) create mode 100644 README-OFTest create mode 100755 tests/run-oftest diff --git a/Makefile.am b/Makefile.am index 36beb6c..193b19e 100644 --- a/Makefile.am +++ b/Makefile.am &lt; at &gt;&lt; at &gt; -55,6 +55,7 &lt; at &gt;&lt; at &gt; EXTRA_DIST = \ NOTICE \ OPENFLOW-1.1+ \ PORTING \ +README-OFTest \ README-gcov \ README-lisp \ REPORTING-BUGS \ diff --git a/NEWS b/NEWS index 4cb4499..7340898 100644 --- a/NEWS +++ b/NEWS &lt; at &gt;&lt; at &gt; -4,6 +4,8 &lt; at &gt;&lt; at &gt; post-v1.11.0 * New support for matching outer source and destination IP address of tunneled packets, for tunnel ports configured with the newly added "remote_ip=flow" and "local_ip=flow" options. + - New "check-oftest" Makefile target for running OFTest against Open + vSwitch. See README-OFTest for details. v1.11.0 - xx xxx xxxx diff --git a/README-OFTest b/README-OFTest new file mode 100644 index 0000000..92936a7 --- /dev/null +++ b/README-OFTest &lt; at &gt;&lt; at &gt; -0,0 +1,77 &lt; at &gt;&lt; at &gt; + How to Use OFTest With Open vSwitch + =================================== + +This document describes how to use the OFTest OpenFlow protocol +testing suite with Open vSwitch in "dummy mode". In this mode of +testing, no packets travel across physical or virtual networks. +Instead, Unix domain sockets stand in as simulated networks. This +simulation is imperfect, but it is much easier to set up, does not +require extra physical or virtual hardware, and does not require +supervisor privileges. + +Prerequisites +------------- + +First, build Open vSwitch according to the instructions in INSTALL. +You need not install it. + +Second, obtain a copy of OFTest and install its prerequisites. You +need a copy of OFTest that includes this commit available at +https://github.com/blp/oftest/commits/master: + + commit 406614846c5eae01f0eb460a9f107e7ed604924f + Author: Ben Pfaff &lt;blp-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; + + make ovs-dummy platform work again + + Commit e1b8da9 (dataplane: single-threaded rewrite) changed the + DataPlanePort required interface but it did not update the ovs-dummy + implementation to match. This commit makes the platform work again. + +Testing OVS in dummy mode does not require root privilege, so you may +ignore that requirement. + +Optionally, add the top-level OFTest directory (containing the "oft" +program) to your $PATH. This slightly simplifies running OFTest later. + +Running OFTest +-------------- + +To run OFTest in dummy mode, run the following command from your Open +vSwitch build directory: + + make check-oftest OFT=&lt;oft-binary&gt; + +where &lt;oft-binary&gt; is the absolute path to the "oft" program in +OFTest. + +If you added "oft" to your $PATH, you may omit the OFT variable +assignment: + + make check-oftest + +By default, "check-oftest" passes "oft" just enough options to enable +dummy mode. You can use OFTFLAGS to pass additional options. For +example, to run just the basic.Echo test instead of all tests (the +default) and enable verbose logging: + + make check-oftest OFT=&lt;oft-binary&gt; OFTFLAGS='--verbose -T basic.Echo' + +Interpreting OFTest Results +--------------------------- + +Please interpret OFTest results cautiously. Open vSwitch can fail a +given test in OFTest for many reasons, including bugs in Open vSwitch, +bugs in OFTest, bugs in the "dummy mode" integration, and differing +interpretations of the OpenFlow standard and other standards. + +Open vSwitch has not been validated against OFTest. Please do report +test failures that you believe to represent bugs in Open vSwitch. +Include the precise versions of Open vSwitch and OFTest in your bug +report, plus any other information needed to reproduce the problem. + +Contact +------- + +bugs-yBygre7rU0TnMu66kgdUjQ&lt; at &gt;public.gmane.org +http://openvswitch.org/ diff --git a/tests/automake.mk b/tests/automake.mk index 4442eb5..15df623 100644 --- a/tests/automake.mk +++ b/tests/automake.mk &lt; at &gt;&lt; at &gt; -145,6 +145,12 &lt; at &gt;&lt; at &gt; check-valgrind: all tests/atconfig tests/atlocal $(TESTSUITE) \ &lt; at &gt;echo 'Valgrind output can be found in tests/testsuite.dir/*/valgrind.*' &lt; at &gt;echo '----------------------------------------------------------------------' +# OFTest support. + +check-oftest: all +srcdir='$(srcdir)' $(SHELL) $(srcdir)/tests/run-oftest +EXTRA_DIST += tests/run-oftest + clean-local: test ! -f '$(TESTSUITE)' || $(SHELL) '$(TESTSUITE)' -C tests --clean diff --git a/tests/run-oftest b/tests/run-oftest new file mode 100755 index 0000000..d12a22f --- /dev/null +++ b/tests/run-oftest &lt; at &gt;&lt; at &gt; -0,0 +1,94 &lt; at &gt;&lt; at &gt; +#! /bin/sh + +set -e + +run () { + echo "$&lt; at &gt;" + "$&lt; at &gt;" || exit 1 +} + +# Put built tools early in $PATH. +builddir=`pwd` +if test ! -e vswitchd/ovs-vswitchd; then + echo &gt;&amp;2 'not in build directory, please change directory or run via \"make check-oftest' + exit 1 +fi +PATH=$builddir/ovsdb:$builddir/vswitchd:$builddir/utilities:$PATH; export PATH + +# Find srcdir. +case $srcdir in + '') srcdir=$builddir ;; + /*) ;; + *) srcdir=`pwd`/$srcdir ;; +esac +if test ! -e "$srcdir"/WHY-OVS; then + echo &gt;&amp;2 'source directory not found, please set $srcdir or run via \"make check-oftest' + exit 1 +fi + +# Make sure oftest is available. +if test X"$OFT" = X; then + OFT=oft +fi +if ($OFT --version) &gt;/dev/null 2&gt;&amp;1; then + : +else + echo &gt;&amp;2 'OFTest "oft" binary not found or cannot be run, please add to $PATH or set $OFT' + exit 1 +fi + +# Create sandbox. +rm -rf sandbox +mkdir sandbox +cd sandbox +sandbox=`pwd` + +# Set up environment for OVS programs to sandbox themselves. +OVS_RUNDIR=$sandbox; export OVS_RUNDIR +OVS_LOGDIR=$sandbox; export OVS_LOGDIR +OVS_DBDIR=$sandbox; export OVS_DBDIR +OVS_SYSCONFDIR=$sandbox; export OVS_SYSCONFDIR + +trap 'kill `cat *.pid`' 0 1 2 3 13 14 15 + +# Create database and start ovsdb-server. +touch .conf.db.~lock~ +rm -f conf.db +run ovsdb-tool create conf.db "$srcdir"/vswitchd/vswitch.ovsschema +run ovsdb-server --detach --no-chdir --pidfile -vconsole:off --log-file \ + --remote=punix:"$sandbox"/db.sock + +# Start ovs-vswitchd. +run ovs-vswitchd --detach --no-chdir --pidfile -vconsole:off --log-file \ + --enable-dummy --disable-system -vvconn -vnetdev_dummy + +# Add a bridge and some ports for OFTest to use, +# and configure ovs-vswitchd to connect to OFTest. +run ovs-vsctl --no-wait \ + -- add-br br0 \ + -- set bridge br0 datapath-type=dummy fail-mode=secure +for port in p1 p2 p3 p4; do + run ovs-vsctl --no-wait \ +-- add-port br0 $port \ +-- set interface $port type=dummy \ + options:pstream=punix:$OVS_RUNDIR/$port +done +run ovs-vsctl \ + -- set-controller br0 tcp:127.0.0.1 \ + -- set controller br0 connection-mode=out-of-band max-backoff=1000 + +# Run OFTest. +run $OFT -P ovs-dummy $OFTFLAGS; status=$? + +cat &lt;&lt;EOF + +---------------------------------------------------------------------- +Logs may be found under $sandbox, e.g.: +$sandbox/oft.log +$sandbox/ovs-vswitchd.log +$sandbox/ovsdb-server.log +---------------------------------------------------------------------- +EOF + +# Propagate OFTest exit status. +exit $status </pre> Ben Pfaff 2013-05-17T22:23:50 http://permalink.gmane.org/gmane.network.openvswitch.devel/20705 <pre>Signed-off-by: Ben Pfaff &lt;blp-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; --- lib/netdev-dummy.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 232 insertions(+), 12 deletions(-) diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 8c182a3..a617218 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c &lt; at &gt;&lt; at &gt; -31,6 +31,8 &lt; at &gt;&lt; at &gt; #include "poll-loop.h" #include "shash.h" #include "sset.h" +#include "stream.h" +#include "unaligned.h" #include "unixctl.h" #include "vlog.h" &lt; at &gt;&lt; at &gt; -42,6 +44,12 &lt; at &gt;&lt; at &gt; VLOG_DEFINE_THIS_MODULE(netdev_dummy); #define FREE_BSD 0 #endif +struct dummy_stream { + struct stream *stream; + struct ofpbuf rxbuf; + struct list txq; +}; + struct netdev_dummy { struct netdev up; uint8_t hwaddr[ETH_ADDR_LEN]; &lt; at &gt;&lt; at &gt; -51,6 +59,10 &lt; at &gt;&lt; at &gt; struct netdev_dummy { unsigned int change_seq; int ifindex; + struct pstream *pstream; + struct dummy_stream *streams; + size_t n_streams; + struct list rxes; /* List of child "netdev_rx_dummy"s. */ }; &lt; at &gt;&lt; at &gt; -73,6 +85,9 &lt; at &gt;&lt; at &gt; static unixctl_cb_func netdev_dummy_set_admin_state; static int netdev_dummy_create(const struct netdev_class *, const char *, struct netdev **); static void netdev_dummy_poll_notify(struct netdev_dummy *); +static void netdev_dummy_queue_packet(struct netdev_dummy *, struct ofpbuf *); + +static void dummy_stream_close(struct dummy_stream *); static bool is_dummy_class(const struct netdev_class *class) &lt; at &gt;&lt; at &gt; -94,6 +109,140 &lt; at &gt;&lt; at &gt; netdev_rx_dummy_cast(const struct netdev_rx *rx) return CONTAINER_OF(rx, struct netdev_rx_dummy, up); } +static void +netdev_dummy_run(void) +{ + struct shash_node *node; + + SHASH_FOR_EACH (node, &amp;dummy_netdevs) { + struct netdev_dummy *dev = node-&gt;data; + size_t i; + + if (dev-&gt;pstream) { + struct stream *new_stream; + int error; + + error = pstream_accept(dev-&gt;pstream, &amp;new_stream); + if (!error) { + struct dummy_stream *s; + + dev-&gt;streams = xrealloc(dev-&gt;streams, + ((dev-&gt;n_streams + 1) + * sizeof *dev-&gt;streams)); + s = &amp;dev-&gt;streams[dev-&gt;n_streams++]; + s-&gt;stream = new_stream; + ofpbuf_init(&amp;s-&gt;rxbuf, 2048); + list_init(&amp;s-&gt;txq); + } else if (error != EAGAIN) { + VLOG_WARN("%s: accept failed (%s)", + pstream_get_name(dev-&gt;pstream), strerror(error)); + pstream_close(dev-&gt;pstream); + dev-&gt;pstream = NULL; + } + } + + for (i = 0; i &lt; dev-&gt;n_streams; i++) { + struct dummy_stream *s = &amp;dev-&gt;streams[i]; + int error = 0; + size_t n; + + stream_run(s-&gt;stream); + + if (!list_is_empty(&amp;s-&gt;txq)) { + struct ofpbuf *txbuf; + int retval; + + txbuf = ofpbuf_from_list(list_front(&amp;s-&gt;txq)); + retval = stream_send(s-&gt;stream, txbuf-&gt;data, txbuf-&gt;size); + if (retval &gt; 0) { + ofpbuf_pull(txbuf, retval); + if (!txbuf-&gt;size) { + list_remove(&amp;txbuf-&gt;list_node); + ofpbuf_delete(txbuf); + } + } else if (retval != -EAGAIN) { + error = -retval; + } + } + + if (!error) { + if (s-&gt;rxbuf.size &lt; 2) { + n = 2 - s-&gt;rxbuf.size; + } else { + uint16_t frame_len; + + frame_len = ntohs(get_unaligned_be16(s-&gt;rxbuf.data)); + if (frame_len &lt; ETH_HEADER_LEN) { + error = EPROTO; + n = 0; + } else { + n = (2 + frame_len) - s-&gt;rxbuf.size; + } + } + } + if (!error) { + int retval; + + ofpbuf_prealloc_tailroom(&amp;s-&gt;rxbuf, n); + retval = stream_recv(s-&gt;stream, ofpbuf_tail(&amp;s-&gt;rxbuf), n); + if (retval &gt; 0) { + s-&gt;rxbuf.size += retval; + if (retval == n &amp;&amp; s-&gt;rxbuf.size &gt; 2) { + ofpbuf_pull(&amp;s-&gt;rxbuf, 2); + netdev_dummy_queue_packet(dev, + ofpbuf_clone(&amp;s-&gt;rxbuf)); + ofpbuf_clear(&amp;s-&gt;rxbuf); + } + } else if (retval != -EAGAIN) { + error = (retval &lt; 0 ? -retval + : s-&gt;rxbuf.size ? EPROTO + : EOF); + } + } + + if (error) { + VLOG_DBG("%s: closing connection (%s)", + stream_get_name(s-&gt;stream), + ovs_retval_to_string(error)); + dummy_stream_close(&amp;dev-&gt;streams[i]); + dev-&gt;streams[i] = dev-&gt;streams[--dev-&gt;n_streams]; + } + } + } +} + +static void +dummy_stream_close(struct dummy_stream *s) +{ + stream_close(s-&gt;stream); + ofpbuf_uninit(&amp;s-&gt;rxbuf); + ofpbuf_list_delete(&amp;s-&gt;txq); +} + +static void +netdev_dummy_wait(void) +{ + struct shash_node *node; + + SHASH_FOR_EACH (node, &amp;dummy_netdevs) { + struct netdev_dummy *dev = node-&gt;data; + size_t i; + + if (dev-&gt;pstream) { + pstream_wait(dev-&gt;pstream); + } + for (i = 0; i &lt; dev-&gt;n_streams; i++) { + struct dummy_stream *s = &amp;dev-&gt;streams[i]; + + stream_run_wait(s-&gt;stream); + if (!list_is_empty(&amp;s-&gt;txq)) { + stream_send_wait(s-&gt;stream); + } + stream_recv_wait(s-&gt;stream); + } + } +} + static int netdev_dummy_create(const struct netdev_class *class, const char *name, struct netdev **netdevp) &lt; at &gt;&lt; at &gt; -113,6 +262,11 &lt; at &gt;&lt; at &gt; netdev_dummy_create(const struct netdev_class *class, const char *name, netdev-&gt;flags = 0; netdev-&gt;change_seq = 1; netdev-&gt;ifindex = -EOPNOTSUPP; + + netdev-&gt;pstream = NULL; + netdev-&gt;streams = NULL; + netdev-&gt;n_streams = 0; + list_init(&amp;netdev-&gt;rxes); shash_add(&amp;dummy_netdevs, name, netdev); &lt; at &gt;&lt; at &gt; -128,9 +282,15 &lt; at &gt;&lt; at &gt; static void netdev_dummy_destroy(struct netdev *netdev_) { struct netdev_dummy *netdev = netdev_dummy_cast(netdev_); + size_t i; shash_find_and_delete(&amp;dummy_netdevs, netdev_get_name(netdev_)); + pstream_close(netdev-&gt;pstream); + for (i = 0; i &lt; netdev-&gt;n_streams; i++) { + dummy_stream_close(&amp;netdev-&gt;streams[i]); + } + free(netdev-&gt;streams); free(netdev); } &lt; at &gt;&lt; at &gt; -142,6 +302,9 &lt; at &gt;&lt; at &gt; netdev_dummy_get_config(const struct netdev *netdev_, struct smap *args) if (netdev-&gt;ifindex &gt;= 0) { smap_add_format(args, "ifindex", "%d", netdev-&gt;ifindex); } + if (netdev-&gt;pstream) { + smap_add(args, "pstream", pstream_get_name(netdev-&gt;pstream)); + } return 0; } &lt; at &gt;&lt; at &gt; -149,8 +312,26 &lt; at &gt;&lt; at &gt; static int netdev_dummy_set_config(struct netdev *netdev_, const struct smap *args) { struct netdev_dummy *netdev = netdev_dummy_cast(netdev_); + const char *pstream; netdev-&gt;ifindex = smap_get_int(args, "ifindex", -EOPNOTSUPP); + + pstream = smap_get(args, "pstream"); + if (!pstream + || !netdev-&gt;pstream + || strcmp(pstream_get_name(netdev-&gt;pstream), pstream)) { + pstream_close(netdev-&gt;pstream); + netdev-&gt;pstream = NULL; + + if (pstream) { + int error; + + error = pstream_open(pstream, &amp;netdev-&gt;pstream, DSCP_DEFAULT); + if (error) { + VLOG_WARN("%s: open failed (%s)", pstream, strerror(error)); + } + } + } return 0; } &lt; at &gt;&lt; at &gt; -222,14 +403,41 &lt; at &gt;&lt; at &gt; netdev_rx_dummy_drain(struct netdev_rx *rx_) } static int -netdev_dummy_send(struct netdev *netdev, const void *buffer OVS_UNUSED, - size_t size) +netdev_dummy_send(struct netdev *netdev, const void *buffer, size_t size) { struct netdev_dummy *dev = netdev_dummy_cast(netdev); + size_t i; + + if (size &lt; ETH_HEADER_LEN) { + return EMSGSIZE; + } else { + const struct eth_header *eth = buffer; + int max_size; + + max_size = dev-&gt;mtu + ETH_HEADER_LEN; + if (eth-&gt;eth_type == htons(ETH_TYPE_VLAN)) { + max_size += VLAN_HEADER_LEN; + } + if (size &gt; max_size) { + return EMSGSIZE; + } + } dev-&gt;stats.tx_packets++; dev-&gt;stats.tx_bytes += size; + for (i = 0; i &lt; dev-&gt;n_streams; i++) { + struct dummy_stream *s = &amp;dev-&gt;streams[i]; + + if (list_size(&amp;s-&gt;txq) &lt; NETDEV_DUMMY_MAX_QUEUE) { + struct ofpbuf *b; + + b = ofpbuf_clone_data_with_headroom(buffer, size, 2); + put_unaligned_be16(ofpbuf_push_uninit(b, 2), htons(size)); + list_push_back(&amp;s-&gt;txq, &amp;b-&gt;list_node); + } + } + return 0; } &lt; at &gt;&lt; at &gt; -341,8 +549,8 &lt; at &gt;&lt; at &gt; netdev_dummy_poll_notify(struct netdev_dummy *dev) static const struct netdev_class dummy_class = { "dummy", NULL, /* init */ - NULL, /* run */ - NULL, /* wait */ + netdev_dummy_run, + netdev_dummy_wait, netdev_dummy_create, netdev_dummy_destroy, &lt; at &gt;&lt; at &gt; -442,18 +650,31 &lt; at &gt;&lt; at &gt; eth_from_packet_or_flow(const char *s) } static void -netdev_dummy_queue_packet(struct netdev_dummy *dummy, - const void *data, size_t size) +netdev_dummy_queue_packet__(struct netdev_rx_dummy *rx, struct ofpbuf *packet) { - struct netdev_rx_dummy *rx; + list_push_back(&amp;rx-&gt;recv_queue, &amp;packet-&gt;list_node); + rx-&gt;recv_queue_len++; +} +static void +netdev_dummy_queue_packet(struct netdev_dummy *dummy, struct ofpbuf *packet) +{ + struct netdev_rx_dummy *rx, *prev; + + prev = NULL; LIST_FOR_EACH (rx, node, &amp;dummy-&gt;rxes) { if (rx-&gt;recv_queue_len &lt; NETDEV_DUMMY_MAX_QUEUE) { - struct ofpbuf *copy = ofpbuf_clone_data(data, size); - list_push_back(&amp;rx-&gt;recv_queue, &amp;copy-&gt;list_node); - rx-&gt;recv_queue_len++; + if (prev) { + netdev_dummy_queue_packet__(prev, ofpbuf_clone(packet)); + } + prev = rx; } } + if (prev) { + netdev_dummy_queue_packet__(prev, packet); + } else { + ofpbuf_delete(packet); + } } static void &lt; at &gt;&lt; at &gt; -483,8 +704,7 &lt; at &gt;&lt; at &gt; netdev_dummy_receive(struct unixctl_conn *conn, dummy_dev-&gt;stats.rx_packets++; dummy_dev-&gt;stats.rx_bytes += packet-&gt;size; - netdev_dummy_queue_packet(dummy_dev, packet-&gt;data, packet-&gt;size); - ofpbuf_delete(packet); + netdev_dummy_queue_packet(dummy_dev, packet); } unixctl_command_reply(conn, NULL); </pre> Ben Pfaff 2013-05-17T22:23:49 http://permalink.gmane.org/gmane.network.openvswitch.devel/20704 <pre>This is the latest version of the oftest series. The first version was posted Oct. 25, 2012. The second version was posted Feb. 1, 2013. The only difference between v2 and v3 is considerable work rebasing due to the restructuring of the netdev library interface. Ben Pfaff (5): netdev-dummy: Limit receive queue length to 100 packets. netdev-dummy: Factor some netdev_dummy_receive() code out into new function. netdev-dummy: Drop "nobody listened" reply from netdev-dummy/receive. netdev-dummy: Add "pstream" option for connecting a dummy to a process. tests: Add support for running OFTest. Makefile.am | 1 + NEWS | 2 + README-OFTest | 77 ++++++++++++++ lib/netdev-dummy.c | 267 +++++++++++++++++++++++++++++++++++++++++++++--- tests/automake.mk | 6 + tests/ofproto-dpif.at | 24 ++--- tests/run-oftest | 94 +++++++++++++++++ 7 files changed, 438 insertions(+), 33 deletions(-) create mode 100644 README-OFTest create mode 100755 tests/run-oftest </pre> Ben Pfaff 2013-05-17T22:23:45 http://permalink.gmane.org/gmane.network.openvswitch.devel/20703 <pre>An upcoming patch will add another user. Signed-off-by: Ben Pfaff &lt;blp-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; --- lib/netdev-dummy.c | 32 ++++++++++++++++++++++---------- 1 files changed, 22 insertions(+), 10 deletions(-) diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 62d8a1b..ce84a0c 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c &lt; at &gt;&lt; at &gt; -441,6 +441,26 &lt; at &gt;&lt; at &gt; eth_from_packet_or_flow(const char *s) return packet; } +static int +netdev_dummy_queue_packet(struct netdev_dummy *dummy, + const void *data, size_t size) +{ + struct netdev_rx_dummy *rx; + int n_listeners; + + n_listeners = 0; + LIST_FOR_EACH (rx, node, &amp;dummy-&gt;rxes) { + if (rx-&gt;recv_queue_len &lt; NETDEV_DUMMY_MAX_QUEUE) { + struct ofpbuf *copy = ofpbuf_clone_data(data, size); + list_push_back(&amp;rx-&gt;recv_queue, &amp;copy-&gt;list_node); + rx-&gt;recv_queue_len++; + n_listeners++; + } + } + + return n_listeners; +} + static void netdev_dummy_receive(struct unixctl_conn *conn, int argc, const char *argv[], void *aux OVS_UNUSED) &lt; at &gt;&lt; at &gt; -457,7 +477,6 &lt; at &gt;&lt; at &gt; netdev_dummy_receive(struct unixctl_conn *conn, n_listeners = 0; for (i = 2; i &lt; argc; i++) { - struct netdev_rx_dummy *rx; struct ofpbuf *packet; packet = eth_from_packet_or_flow(argv[i]); &lt; at &gt;&lt; at &gt; -469,15 +488,8 &lt; at &gt;&lt; at &gt; netdev_dummy_receive(struct unixctl_conn *conn, dummy_dev-&gt;stats.rx_packets++; dummy_dev-&gt;stats.rx_bytes += packet-&gt;size; - n_listeners = 0; - LIST_FOR_EACH (rx, node, &amp;dummy_dev-&gt;rxes) { - if (rx-&gt;recv_queue_len &lt; NETDEV_DUMMY_MAX_QUEUE) { - struct ofpbuf *copy = ofpbuf_clone(packet); - list_push_back(&amp;rx-&gt;recv_queue, &amp;copy-&gt;list_node); - rx-&gt;recv_queue_len++; - } - n_listeners++; - } + n_listeners += netdev_dummy_queue_packet(dummy_dev, + packet-&gt;data, packet-&gt;size); ofpbuf_delete(packet); } </pre> Ben Pfaff 2013-05-17T22:23:47 http://permalink.gmane.org/gmane.network.openvswitch.devel/20702 <pre>Ethan pointed out that this wasn't very useful. Signed-off-by: Ben Pfaff &lt;blp-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; --- lib/netdev-dummy.c | 16 +++------------- tests/ofproto-dpif.at | 24 ++++++++---------------- 2 files changed, 11 insertions(+), 29 deletions(-) diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index ce84a0c..8c182a3 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c &lt; at &gt;&lt; at &gt; -441,24 +441,19 &lt; at &gt;&lt; at &gt; eth_from_packet_or_flow(const char *s) return packet; } -static int +static void netdev_dummy_queue_packet(struct netdev_dummy *dummy, const void *data, size_t size) { struct netdev_rx_dummy *rx; - int n_listeners; - n_listeners = 0; LIST_FOR_EACH (rx, node, &amp;dummy-&gt;rxes) { if (rx-&gt;recv_queue_len &lt; NETDEV_DUMMY_MAX_QUEUE) { struct ofpbuf *copy = ofpbuf_clone_data(data, size); list_push_back(&amp;rx-&gt;recv_queue, &amp;copy-&gt;list_node); rx-&gt;recv_queue_len++; - n_listeners++; } } - - return n_listeners; } static void &lt; at &gt;&lt; at &gt; -488,16 +483,11 &lt; at &gt;&lt; at &gt; netdev_dummy_receive(struct unixctl_conn *conn, dummy_dev-&gt;stats.rx_packets++; dummy_dev-&gt;stats.rx_bytes += packet-&gt;size; - n_listeners += netdev_dummy_queue_packet(dummy_dev, - packet-&gt;data, packet-&gt;size); + netdev_dummy_queue_packet(dummy_dev, packet-&gt;data, packet-&gt;size); ofpbuf_delete(packet); } - if (!n_listeners) { - unixctl_command_reply(conn, "packets queued but nobody listened"); - } else { - unixctl_command_reply(conn, "success"); - } + unixctl_command_reply(conn, NULL); } static void diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 1fdbac3..ba8c975 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at &lt; at &gt;&lt; at &gt; -1730,8 +1730,7 &lt; at &gt;&lt; at &gt; AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip], [0], # flow stats updates are mainly what implements the fin_timeout # feature, we warp forward a couple of times to ensure that flow stats # run before re-checking the flow table.) -AT_CHECK([ovs-appctl netdev-dummy/receive br0 0021853763af0026b98cb0f908004500003c2e2440004006465dac11370dac11370b828b0016751e267b00000000a00216d017360000020405b40402080a2d25085f0000000001030307], [0], [success -]) +AT_CHECK([ovs-appctl netdev-dummy/receive br0 0021853763af0026b98cb0f908004500003c2e2440004006465dac11370dac11370b828b0016751e267b00000000a00216d017360000020405b40402080a2d25085f0000000001030307]) AT_CHECK([ovs-appctl time/warp 1000 &amp;&amp; ovs-appctl time/warp 1000], [0], [warped warped ]) &lt; at &gt;&lt; at &gt; -1740,8 +1739,7 &lt; at &gt;&lt; at &gt; AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip], [0], n_packets=1, n_bytes=74, idle_timeout=60, actions=fin_timeout(idle_timeout=5) ]) # Check that a TCP FIN packet does change the timeout. -AT_CHECK([ovs-appctl netdev-dummy/receive br0 0021853763af0026b98cb0f90800451000342e3e40004006463bac11370dac11370b828b0016751e319dfc96399b801100717ae800000101080a2d250a9408579588], [0], [success -]) +AT_CHECK([ovs-appctl netdev-dummy/receive br0 0021853763af0026b98cb0f90800451000342e3e40004006463bac11370dac11370b828b0016751e319dfc96399b801100717ae800000101080a2d250a9408579588]) AT_CHECK([ovs-appctl time/warp 1000 &amp;&amp; ovs-appctl time/warp 1000], [0], [warped warped ]) &lt; at &gt;&lt; at &gt; -1803,12 +1801,9 &lt; at &gt;&lt; at &gt; OVS_VSWITCHD_START([add-br br1 -- \ ADD_OF_PORTS([br0], [1], [2]) ADD_OF_PORTS([br1], [3]) -AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'], [0], [success -]) -AT_CHECK([ovs-appctl netdev-dummy/receive p2 'in_port(2),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)'], [0], [success -]) -AT_CHECK([ovs-appctl netdev-dummy/receive p3 'in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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)'], [0], [success -]) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 'in_port(2),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)']) +AT_CHECK([ovs-appctl netdev-dummy/receive p3 'in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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([ovs-appctl dpif/dump-flows br0 | sort | STRIP_USED], [0], [dnl in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0), packets:0, bytes:0, used:0.0s, actions:userspace(pid=0,slow_path(controller)) &lt; at &gt;&lt; at &gt; -1828,12 +1823,9 &lt; at &gt;&lt; at &gt; OVS_VSWITCHD_START([add-br br1 -- \ ADD_OF_PORTS([br0], [1], [2]) ADD_OF_PORTS([br1], [3]) -AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'], [0], [success -]) -AT_CHECK([ovs-appctl netdev-dummy/receive p2 'in_port(2),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)'], [0], [success -]) -AT_CHECK([ovs-appctl netdev-dummy/receive p3 'in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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)'], [0], [success -]) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 'in_port(2),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)']) +AT_CHECK([ovs-appctl netdev-dummy/receive p3 'in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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([ovs-appctl dpif/dump-flows br0 | sort | STRIP_USED], [0], [dnl in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0), packets:0, bytes:0, used:0.0s, actions:userspace(pid=0,slow_path(controller)) </pre> Ben Pfaff 2013-05-17T22:23:48 http://permalink.gmane.org/gmane.network.openvswitch.devel/20701 <pre>It doesn't seem like a good idea to allow the queue length to grow without bound, even for a test-only device. Signed-off-by: Ben Pfaff &lt;blp-l0M0P4e3n4LQT0dZR+AlfA&lt; at &gt;public.gmane.org&gt; --- lib/netdev-dummy.c | 15 +++++++++++++-- 1 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 908fef2..62d8a1b 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c &lt; at &gt;&lt; at &gt; -54,10 +54,15 &lt; at &gt;&lt; at &gt; struct netdev_dummy { struct list rxes; /* List of child "netdev_rx_dummy"s. */ }; +/* Max 'recv_queue_len' in struct netdev_dummy. */ +#define NETDEV_DUMMY_MAX_QUEUE 100 + struct netdev_rx_dummy { struct netdev_rx up; struct list node; /* In netdev_dummy's "rxes" list. */ struct list recv_queue; + int recv_queue_len; /* list_size(&amp;recv_queue). */ + bool listening; }; static struct shash dummy_netdevs = SHASH_INITIALIZER(&amp;dummy_netdevs); &lt; at &gt;&lt; at &gt; -159,6 +164,7 &lt; at &gt;&lt; at &gt; netdev_dummy_rx_open(struct netdev *netdev_, struct netdev_rx **rxp) netdev_rx_init(&amp;rx-&gt;up, &amp;netdev-&gt;up, &amp;netdev_rx_dummy_class); list_push_back(&amp;netdev-&gt;rxes, &amp;rx-&gt;node); list_init(&amp;rx-&gt;recv_queue); + rx-&gt;recv_queue_len = 0; *rxp = &amp;rx-&gt;up; return 0; &lt; at &gt;&lt; at &gt; -176,6 +182,7 &lt; at &gt;&lt; at &gt; netdev_rx_dummy_recv(struct netdev_rx *rx_, void *buffer, size_t size) } packet = ofpbuf_from_list(list_pop_front(&amp;rx-&gt;recv_queue)); + rx-&gt;recv_queue_len--; if (packet-&gt;size &gt; size) { return -EMSGSIZE; } &lt; at &gt;&lt; at &gt; -210,6 +217,7 &lt; at &gt;&lt; at &gt; netdev_rx_dummy_drain(struct netdev_rx *rx_) { struct netdev_rx_dummy *rx = netdev_rx_dummy_cast(rx_); ofpbuf_list_delete(&amp;rx-&gt;recv_queue); + rx-&gt;recv_queue_len = 0; return 0; } &lt; at &gt;&lt; at &gt; -463,8 +471,11 &lt; at &gt;&lt; at &gt; netdev_dummy_receive(struct unixctl_conn *conn, n_listeners = 0; LIST_FOR_EACH (rx, node, &amp;dummy_dev-&gt;rxes) { - struct ofpbuf *copy = ofpbuf_clone(packet); - list_push_back(&amp;rx-&gt;recv_queue, &amp;copy-&gt;list_node); + if (rx-&gt;recv_queue_len &lt; NETDEV_DUMMY_MAX_QUEUE) { + struct ofpbuf *copy = ofpbuf_clone(packet); + list_push_back(&amp;rx-&gt;recv_queue, &amp;copy-&gt;list_node); + rx-&gt;recv_queue_len++; + } n_listeners++; } ofpbuf_delete(packet); </pre> Ben Pfaff 2013-05-17T22:23:46 http://permalink.gmane.org/gmane.network.openvswitch.devel/20699 <pre> Here's a combined set of comments for all of the patches in the execute-actions code: * The convention used other places in the code when passing around void pointers and casting them is to have the variable named with an underscore be the void *. * I would extract the userdata for userspace actions in execute_actions() and pass it directly to the userspace function pointer. * I don't think there is a lot of value in the asserts in execute_actions(). I think it's valid to pass NULL for 'dp' and 'key' in these situations if you want. The function pointers can't be NULL but that will stop the program just as quickly as an assert anyways. * The later patches pass around a huge number of arguments. We should find some way of encapsulating them in a struct. It might even be possible to store them directly in the flow. It deviates slightly from other behavior because we don't update the flow when we change the packet but it would clean a lot of other things up. * We already do some action execution in userspace when we send packets to the controller. It would be good to see if we can start using this code there as well. </pre> Jesse Gross 2013-05-17T20:40:10 http://permalink.gmane.org/gmane.network.openvswitch.devel/20698 <pre> I'm concerned about putting a 512+ byte structure into a facet. I think the size of xlate_out should therefore be reduced. I'd like to look at this series again when you've revised it. Thanks, Ben. </pre> Ben Pfaff 2013-05-17T19:24:04 http://permalink.gmane.org/gmane.network.openvswitch.devel/20697 <pre> Sure, fine with me. </pre> Ben Pfaff 2013-05-17T18:25:57 http://permalink.gmane.org/gmane.network.openvswitch.devel/20696 <pre> Would you describe it a little? It's a big patch so the fix isn't necessarily obvious (I haven't read on yet). Is it something that we should break out separately so that we can backport it? I agree with Justin's comment. subfacet_create() refers to subfacet_make_actions() in a comment, but there is no longer such a function. handle_flow_miss_with_facet() previously translated the actions separately for each packet, if it was necessary due to slow-pathing. The new version doesn't do that. Are you sure that every packet for a facet always has the same datapath actions, even if the facet is slow-pathed? We have a comment in facet_check_consistency() that says that they can vary: /* The actions for slow-path flows may legitimately vary from one * packet to the next. We're done. */ The new odp_actions_stub in struct facet makes facets huge! This should not be necessary. I'd use a much smaller stub there, if we need one at all. I would consider an approach like the following. - Put a pointer, a length, and a small fixed-size buffer that we think will cover the common case (maybe 32 bytes?) into struct facet. This avoids not just a huge stub there but the pretty-large struct ofpbuf (currently 52 bytes on 32-bit) too. struct nlattr *odp_actions; size_t odp_actions_len; /* or unsigned int */ uint64_t odp_actions_stub[32 / 8]; - Translate the actions into a larger (1 kB or 4 kB) on-stack stub in facet_create(). - If the translated actions fit into facet-&gt;odp_actions_stub, use it and point facet-&gt;odp_actions to it, otherwise xmemdup() an allocated region. - When we're done with the facet, if facet-&gt;odp_actions != facet-&gt;odp_actions_stub, free(facet-&gt;odp_actions). Finally, I think we might actually have a fairly long-standing bug due to a bad assumption. We assume that a subfacet, that is, a given ODP key, can map to exactly one facet (see "This shouldn't happen." comment in subfacet_create()) but now I think that's wrong. For example, suppose as a thought experiment that the kernel reported no fields at all for any packet. Then every packet would have the same ODP key, and so all of them would have the same subfacet, but obviously we'd want more than one facet. I think that essentially the same thing, but more subtle, can come up whenever the kernel supports less-specific matching than userspace. One simple solution seems to be to just never create facets or subfacets when ODP_FIT_TOO_LITTLE comes up. That would simplify other code, too, since we could get rid of SLOW_MATCH, subfacet_slow_reason(), etc. I'd like to review this again after revision. Thanks, Ben. </pre> Ben Pfaff 2013-05-17T18:21:38 http://permalink.gmane.org/gmane.network.openvswitch.devel/20690 <pre> Thanks! </pre> Simon Horman 2013-05-17T06:08:43 Search the mailing list at Gmane query http://search.gmane.org/?group=$group=gmane.network.openvswitch.devel