Thanks. Since you said off-list that you were happy with this series, I applied it to master.
On Wed, Oct 9, 2013 at 1:40 PM, Alex Wang <al...@nicira.com> wrote: > The changes and tests all look good to me, > > > On Mon, Sep 23, 2013 at 10:49 AM, Ben Pfaff <b...@nicira.com> wrote: > >> This support is added through the userspace slow path, because we don't >> judge that this is important enough to require permanent support in the >> Linux kernel ABI. >> >> CC: Teemu Koponen <kopo...@nicira.com> >> CC: Pankaj Thakkar <thak...@nicira.com>" >> Signed-off-by: Ben Pfaff <b...@nicira.com> >> --- >> lib/meta-flow.c | 10 ++++---- >> lib/odp-execute.c | 18 ++++++++++++- >> lib/odp-util.c | 62 >> +++++++++++++++++++++++++++++++++++++++------ >> tests/ofproto-dpif.at | 67 >> ++++++++++++++++++++++++++++++++++++++++++++++++- >> 4 files changed, 143 insertions(+), 14 deletions(-) >> >> diff --git a/lib/meta-flow.c b/lib/meta-flow.c >> index 3a31c29..0bfb834 100644 >> --- a/lib/meta-flow.c >> +++ b/lib/meta-flow.c >> @@ -482,7 +482,7 @@ static const struct mf_field mf_fields[MFF_N_IDS] = { >> MFM_NONE, >> MFS_DECIMAL, >> MFP_ARP, >> - false, >> + true, >> NXM_OF_ARP_OP, "NXM_OF_ARP_OP", >> OXM_OF_ARP_OP, "OXM_OF_ARP_OP", >> OFPUTIL_P_ANY, >> @@ -493,7 +493,7 @@ static const struct mf_field mf_fields[MFF_N_IDS] = { >> MFM_FULLY, >> MFS_IPV4, >> MFP_ARP, >> - false, >> + true, >> NXM_OF_ARP_SPA, "NXM_OF_ARP_SPA", >> OXM_OF_ARP_SPA, "OXM_OF_ARP_SPA", >> OFPUTIL_P_ANY, >> @@ -504,7 +504,7 @@ static const struct mf_field mf_fields[MFF_N_IDS] = { >> MFM_FULLY, >> MFS_IPV4, >> MFP_ARP, >> - false, >> + true, >> NXM_OF_ARP_TPA, "NXM_OF_ARP_TPA", >> OXM_OF_ARP_TPA, "OXM_OF_ARP_TPA", >> OFPUTIL_P_ANY, >> @@ -515,7 +515,7 @@ static const struct mf_field mf_fields[MFF_N_IDS] = { >> MFM_FULLY, >> MFS_ETHERNET, >> MFP_ARP, >> - false, >> + true, >> NXM_NX_ARP_SHA, "NXM_NX_ARP_SHA", >> OXM_OF_ARP_SHA, "OXM_OF_ARP_SHA", >> OFPUTIL_P_NXM_OXM_ANY, >> @@ -526,7 +526,7 @@ static const struct mf_field mf_fields[MFF_N_IDS] = { >> MFM_FULLY, >> MFS_ETHERNET, >> MFP_ARP, >> - false, >> + true, >> NXM_NX_ARP_THA, "NXM_NX_ARP_THA", >> OXM_OF_ARP_THA, "OXM_OF_ARP_THA", >> OFPUTIL_P_NXM_OXM_ANY, >> diff --git a/lib/odp-execute.c b/lib/odp-execute.c >> index 3914c3b..185cf31 100644 >> --- a/lib/odp-execute.c >> +++ b/lib/odp-execute.c >> @@ -25,6 +25,7 @@ >> #include "ofpbuf.h" >> #include "odp-util.h" >> #include "packets.h" >> +#include "unaligned.h" >> #include "util.h" >> >> static void >> @@ -46,6 +47,18 @@ odp_set_tunnel_action(const struct nlattr *a, struct >> flow_tnl *tun_key) >> } >> >> static void >> +set_arp(struct ofpbuf *packet, const struct ovs_key_arp *arp_key) >> +{ >> + struct arp_eth_header *arp = packet->l3; >> + >> + arp->ar_op = arp_key->arp_op; >> + memcpy(arp->ar_sha, arp_key->arp_sha, ETH_ADDR_LEN); >> + put_16aligned_be32(&arp->ar_spa, arp_key->arp_sip); >> + memcpy(arp->ar_tha, arp_key->arp_tha, ETH_ADDR_LEN); >> + put_16aligned_be32(&arp->ar_tpa, arp_key->arp_tip); >> +} >> + >> +static void >> odp_execute_set_action(struct ofpbuf *packet, const struct nlattr *a, >> struct flow *flow) >> { >> @@ -106,6 +119,10 @@ odp_execute_set_action(struct ofpbuf *packet, const >> struct nlattr *a, >> set_mpls_lse(packet, nl_attr_get_be32(a)); >> break; >> >> + case OVS_KEY_ATTR_ARP: >> + set_arp(packet, nl_attr_get_unspec(a, sizeof(struct >> ovs_key_arp))); >> + break; >> + >> case OVS_KEY_ATTR_UNSPEC: >> case OVS_KEY_ATTR_ENCAP: >> case OVS_KEY_ATTR_ETHERTYPE: >> @@ -113,7 +130,6 @@ odp_execute_set_action(struct ofpbuf *packet, const >> struct nlattr *a, >> case OVS_KEY_ATTR_VLAN: >> case OVS_KEY_ATTR_ICMP: >> case OVS_KEY_ATTR_ICMPV6: >> - case OVS_KEY_ATTR_ARP: >> case OVS_KEY_ATTR_ND: >> case __OVS_KEY_ATTR_MAX: >> default: >> diff --git a/lib/odp-util.c b/lib/odp-util.c >> index b317cc2..d34edb8 100644 >> --- a/lib/odp-util.c >> +++ b/lib/odp-util.c >> @@ -3453,20 +3453,66 @@ commit_set_ipv6_action(const struct flow *flow, >> struct flow *base, >> &ipv6_key, sizeof(ipv6_key)); >> } >> >> -static void >> +static enum slow_path_reason >> +commit_set_arp_action(const struct flow *flow, struct flow *base, >> + struct ofpbuf *odp_actions, struct flow_wildcards >> *wc) >> +{ >> + struct ovs_key_arp arp_key; >> + >> + if (base->nw_src == flow->nw_src && >> + base->nw_dst == flow->nw_dst && >> + base->nw_proto == flow->nw_proto && >> + eth_addr_equals(base->arp_sha, flow->arp_sha) && >> + eth_addr_equals(base->arp_tha, flow->arp_tha)) { >> + return 0; >> + } >> + >> + memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src); >> + memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst); >> + memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); >> + memset(&wc->masks.arp_sha, 0xff, sizeof wc->masks.arp_sha); >> + memset(&wc->masks.arp_tha, 0xff, sizeof wc->masks.arp_tha); >> + >> + base->nw_src = flow->nw_src; >> + base->nw_dst = flow->nw_dst; >> + base->nw_proto = flow->nw_proto; >> + memcpy(base->arp_sha, flow->arp_sha, ETH_ADDR_LEN); >> + memcpy(base->arp_tha, flow->arp_tha, ETH_ADDR_LEN); >> + >> + arp_key.arp_sip = base->nw_src; >> + arp_key.arp_tip = base->nw_dst; >> + arp_key.arp_op = htons(base->nw_proto); >> + memcpy(arp_key.arp_sha, flow->arp_sha, ETH_ADDR_LEN); >> + memcpy(arp_key.arp_tha, flow->arp_tha, ETH_ADDR_LEN); >> + >> + commit_set_action(odp_actions, OVS_KEY_ATTR_ARP, &arp_key, sizeof >> arp_key); >> + >> + return SLOW_ACTION; >> +} >> + >> +static enum slow_path_reason >> commit_set_nw_action(const struct flow *flow, struct flow *base, >> struct ofpbuf *odp_actions, struct flow_wildcards >> *wc) >> { >> - /* Check if flow really have an IP header. */ >> + /* Check if 'flow' really has an L3 header. */ >> if (!flow->nw_proto) { >> - return; >> + return 0; >> } >> >> - if (base->dl_type == htons(ETH_TYPE_IP)) { >> + switch (ntohs(base->dl_type)) { >> + case ETH_TYPE_IP: >> commit_set_ipv4_action(flow, base, odp_actions, wc); >> - } else if (base->dl_type == htons(ETH_TYPE_IPV6)) { >> + break; >> + >> + case ETH_TYPE_IPV6: >> commit_set_ipv6_action(flow, base, odp_actions, wc); >> + break; >> + >> + case ETH_TYPE_ARP: >> + return commit_set_arp_action(flow, base, odp_actions, wc); >> } >> + >> + return 0; >> } >> >> static void >> @@ -3557,9 +3603,11 @@ enum slow_path_reason >> commit_odp_actions(const struct flow *flow, struct flow *base, >> struct ofpbuf *odp_actions, struct flow_wildcards *wc) >> { >> + enum slow_path_reason slow; >> + >> commit_set_ether_addr_action(flow, base, odp_actions, wc); >> commit_vlan_action(flow, base, odp_actions, wc); >> - commit_set_nw_action(flow, base, odp_actions, wc); >> + slow = commit_set_nw_action(flow, base, odp_actions, wc); >> commit_set_port_action(flow, base, odp_actions, wc); >> /* Committing MPLS actions should occur after committing nw and port >> * actions. This is because committing MPLS actions may alter a >> packet so >> @@ -3569,5 +3617,5 @@ commit_odp_actions(const struct flow *flow, struct >> flow *base, >> commit_set_priority_action(flow, base, odp_actions, wc); >> commit_set_pkt_mark_action(flow, base, odp_actions, wc); >> >> - return 0; >> + return slow; >> } >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> index f67c3ab..7ae0271 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -277,6 +277,7 @@ cookie=0xa dl_src=40:44:44:44:44:48 >> actions=push_mpls:0x8847,load:10->OXM_OF_MPL >> cookie=0xb dl_src=50:55:55:55:55:55 dl_type=0x8847 >> actions=load:1000->OXM_OF_MPLS_LABEL[[]],controller >> cookie=0xd dl_src=60:66:66:66:66:66 actions=pop_mpls:0x0800,controller >> cookie=0xc dl_src=70:77:77:77:77:77 >> actions=push_mpls:0x8848,load:1000->OXM_OF_MPLS_LABEL[[]],load:7->OXM_OF_MPLS_TC[[]],controller >> +cookie=0xd dl_src=80:88:88:88:88:88 arp >> actions=load:2->OXM_OF_ARP_OP[[]],controller,load:0xc0a88001->OXM_OF_ARP_SPA[[]],controller,load:0x404444444441->OXM_OF_ARP_THA[[]],controller >> ]) >> AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >> >> @@ -645,7 +646,35 @@ NXT_PACKET_IN (xid=0x0): table_id=7 cookie=0x9 >> total_len=64 in_port=1 tun_id=0x6 >> >> udp,metadata=0,in_port=0,dl_vlan=80,dl_vlan_pcp=0,dl_src=80:81:81:81:81:81,dl_dst=82:82:82:82:82:82,nw_src=83.83.83.83,nw_dst=84.84.84.84,nw_tos=0,nw_ecn=0,nw_ttl=0,tp_src=85,tp_dst=86 >> udp_csum:43a1 >> ]) >> >> -ovs-appctl time/stop >> +dnl Modified ARP controller action. >> +AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> >> ofctl_monitor.log]) >> + >> +for i in 1 2 3; do >> + ovs-appctl netdev-dummy/receive p1 >> 'in_port(1),eth(src=80:88:88:88:88:88,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=192.168.0.1,tip=192.168.0.2,op=1,sha=50:54:00:00:00:05,tha=00:00:00:00:00:00)' >> +done >> + >> +OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit]) >> +AT_CHECK([cat ofctl_monitor.log], [0], [dnl >> +NXT_PACKET_IN (xid=0x0): cookie=0xd total_len=60 in_port=1 (via action) >> data_len=60 (unbuffered) >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> +NXT_PACKET_IN (xid=0x0): cookie=0xd total_len=60 in_port=1 (via action) >> data_len=60 (unbuffered) >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> +NXT_PACKET_IN (xid=0x0): cookie=0xd total_len=60 in_port=1 (via action) >> data_len=60 (unbuffered) >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=40:44:44:44:44:41 >> +NXT_PACKET_IN (xid=0x0): cookie=0xd total_len=60 in_port=1 (via action) >> data_len=60 (unbuffered) >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> +NXT_PACKET_IN (xid=0x0): cookie=0xd total_len=60 in_port=1 (via action) >> data_len=60 (unbuffered) >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> +NXT_PACKET_IN (xid=0x0): cookie=0xd total_len=60 in_port=1 (via action) >> data_len=60 (unbuffered) >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=40:44:44:44:44:41 >> +NXT_PACKET_IN (xid=0x0): cookie=0xd total_len=60 in_port=1 (via action) >> data_len=60 (unbuffered) >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> +NXT_PACKET_IN (xid=0x0): cookie=0xd total_len=60 in_port=1 (via action) >> data_len=60 (unbuffered) >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> +NXT_PACKET_IN (xid=0x0): cookie=0xd total_len=60 in_port=1 (via action) >> data_len=60 (unbuffered) >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=40:44:44:44:44:41 >> +]) >> + >> AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore]) >> >> dnl Checksum SCTP. >> @@ -705,6 +734,7 @@ AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | >> sort], [0], [dnl >> cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:44:48 >> actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x3->OXM_OF_MPLS_TC[[]],set_mpls_ttl(10),dec_mpls_ttl,CONTROLLER:65535 >> cookie=0xb, n_packets=3, n_bytes=180, mpls,dl_src=50:55:55:55:55:55 >> actions=load:0x3e8->OXM_OF_MPLS_LABEL[[]],CONTROLLER:65535 >> cookie=0xc, n_packets=3, n_bytes=180, dl_src=70:77:77:77:77:77 >> actions=push_mpls:0x8848,load:0x3e8->OXM_OF_MPLS_LABEL[[]],load:0x7->OXM_OF_MPLS_TC[[]],CONTROLLER:65535 >> + cookie=0xd, n_packets=3, n_bytes=180, arp,dl_src=80:88:88:88:88:88 >> actions=load:0x2->NXM_OF_ARP_OP[[]],CONTROLLER:65535,load:0xc0a88001->NXM_OF_ARP_SPA[[]],CONTROLLER:65535,load:0x404444444441->NXM_NX_ARP_THA[[]],CONTROLLER:65535 >> cookie=0xd, n_packets=3, n_bytes=186, dl_src=60:66:66:66:66:66 >> actions=pop_mpls:0x0800,CONTROLLER:65535 >> n_packets=3, n_bytes=180, dl_src=10:11:11:11:11:11 >> actions=CONTROLLER:65535 >> NXST_FLOW reply: >> @@ -713,6 +743,41 @@ NXST_FLOW reply: >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([ofproto-dpif - ARP modification slow-path]) >> +OVS_VSWITCHD_START >> +ADD_OF_PORTS([br0], [1], [2]) >> + >> +ovs-vsctl -- set Interface p2 type=dummy options:pcap=p2.pcap >> +ovs-ofctl add-flow br0 'in_port=1,arp >> actions=load:2->OXM_OF_ARP_OP[[]],2,load:0xc0a88001->OXM_OF_ARP_SPA[[]],2,load:0x404444444441->OXM_OF_ARP_THA[[]],2' >> + >> +# Input some packets that should follow the arp modification slow-path. >> +for i in 1 2 3; do >> + ovs-appctl netdev-dummy/receive p1 >> 'in_port(1),eth(src=80:88:88:88:88:88,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=192.168.0.1,tip=192.168.0.2,op=1,sha=50:54:00:00:00:05,tha=00:00:00:00:00:00)' >> +done >> +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore]) >> + >> +# Check the packets that were output. >> +AT_CHECK([ovs-ofctl parse-pcap p2.pcap], [0], [dnl >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=40:44:44:44:44:41 >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=40:44:44:44:44:41 >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.0.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=00:00:00:00:00:00 >> >> +arp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=80:88:88:88:88:88,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.128.1,arp_tpa=192.168.0.2,arp_op=2,arp_sha=50:54:00:00:00:05,arp_tha=40:44:44:44:44:41 >> +]) >> + >> +# Check that each of the packets actually passed through the slow-path. >> +AT_CHECK([ovs-appctl coverage/show], [0], [stdout]) >> +AT_CHECK([sed -n 's/[[ ]]\{2,\}/ /g >> +/dpif_execute_with_help/p' stdout], [0], [dpif_execute_with_help 3 >> +]) >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> AT_SETUP([ofproto-dpif - VLAN handling]) >> OVS_VSWITCHD_START( >> [set Bridge br0 fail-mode=standalone -- \ >> -- >> 1.7.10.4 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >> > > -- "I don't normally do acked-by's. I think it's my way of avoiding getting blamed when it all blows up." Andrew Morton
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev