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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
