[ovs-dev] [PATCH 2/2] ofproto-dpif: Always un-wildcard 'dl_type'.

Justin Pettit jpettit at nicira.com
Wed Jun 26 05:44:13 UTC 2013


Thanks for the reviews.  I pushed to all affected branches.

--Justin


On Jun 25, 2013, at 10:23 PM, Ben Pfaff <blp at nicira.com> wrote:

> Looks good.
> 
> On Jun 25, 2013 6:29 PM, "Justin Pettit" <jpettit at nicira.com> wrote:
> We always look at the fragment status and often look at other L3
> headers when processing the packet, so just un-wildcard the
> Ethertype.
> 
> Signed-off-by: Justin Pettit <jpettit at nicira.com>
> ---
>  lib/bfd.c                    |    1 -
>  lib/cfm.c                    |    1 -
>  lib/flow.c                   |    1 -
>  ofproto/netflow.c            |    1 -
>  ofproto/ofproto-dpif-xlate.c |   13 +------------
>  ofproto/ofproto-dpif.c       |    1 +
>  tests/ofproto-dpif.at        |   22 +++++++++++-----------
>  7 files changed, 13 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/bfd.c b/lib/bfd.c
> index ddf8c4c..42d68ad 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -416,7 +416,6 @@ bfd_put_packet(struct bfd *bfd, struct ofpbuf *p,
>  bool
>  bfd_should_process_flow(const struct flow *flow, struct flow_wildcards *wc)
>  {
> -    memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>      memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>      memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
>      return (flow->dl_type == htons(ETH_TYPE_IP)
> diff --git a/lib/cfm.c b/lib/cfm.c
> index 67c2002..1c98ed8 100644
> --- a/lib/cfm.c
> +++ b/lib/cfm.c
> @@ -584,7 +584,6 @@ cfm_should_process_flow(const struct cfm *cfm, const struct flow *flow,
>                          struct flow_wildcards *wc)
>  {
>      memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
> -    memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>      if (cfm->check_tnl_key) {
>          memset(&wc->masks.tunnel.tun_id, 0xff, sizeof wc->masks.tunnel.tun_id);
>      }
> diff --git a/lib/flow.c b/lib/flow.c
> index 32ac99a..a42fea1 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -791,7 +791,6 @@ flow_mask_hash_fields(struct flow_wildcards *wc, enum nx_hash_fields fields)
>      case NX_HASH_FIELDS_SYMMETRIC_L4:
>          memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
>          memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
> -        memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>          memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>          memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
>          memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
> diff --git a/ofproto/netflow.c b/ofproto/netflow.c
> index 2e32f21..c7eb2b5 100644
> --- a/ofproto/netflow.c
> +++ b/ofproto/netflow.c
> @@ -54,7 +54,6 @@ struct netflow {
>  void
>  netflow_mask_wc(struct flow_wildcards *wc)
>  {
> -    memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>      memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>      memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
>      memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 36f54ee..d77e4df 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -548,7 +548,6 @@ xlate_normal(struct xlate_ctx *ctx)
>      ctx->xout->has_normal = true;
> 
>      /* Check the dl_type, since we may check for gratuituous ARP. */
> -    memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>      memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
>      memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
>      wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
> @@ -813,7 +812,6 @@ process_special(struct xlate_ctx *ctx, const struct flow *flow,
>          return SLOW_BFD;
>      } else if (ofport->bundle && ofport->bundle->lacp
>                 && flow->dl_type == htons(ETH_TYPE_LACP)) {
> -        memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>          if (packet) {
>              lacp_process_packet(ofport->bundle->lacp, ofport, packet);
>          }
> @@ -1147,7 +1145,6 @@ compose_mpls_push_action(struct xlate_ctx *ctx, ovs_be16 eth_type)
> 
>      ovs_assert(eth_type_mpls(eth_type));
> 
> -    memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>      memset(&wc->masks.mpls_lse, 0xff, sizeof wc->masks.mpls_lse);
>      memset(&wc->masks.mpls_depth, 0xff, sizeof wc->masks.mpls_depth);
> 
> @@ -1182,7 +1179,6 @@ compose_mpls_pop_action(struct xlate_ctx *ctx, ovs_be16 eth_type)
>      ovs_assert(eth_type_mpls(ctx->xin->flow.dl_type));
>      ovs_assert(!eth_type_mpls(eth_type));
> 
> -    memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>      memset(&wc->masks.mpls_lse, 0xff, sizeof wc->masks.mpls_lse);
>      memset(&wc->masks.mpls_depth, 0xff, sizeof wc->masks.mpls_depth);
> 
> @@ -1239,7 +1235,6 @@ compose_dec_mpls_ttl_action(struct xlate_ctx *ctx)
>      uint8_t ttl = mpls_lse_to_ttl(flow->mpls_lse);
>      struct flow_wildcards *wc = &ctx->xout->wc;
> 
> -    memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>      memset(&wc->masks.mpls_lse, 0xff, sizeof wc->masks.mpls_lse);
> 
>      if (!eth_type_mpls(flow->dl_type)) {
> @@ -1594,14 +1589,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>              break;
> 
>          case OFPACT_SET_IPV4_SRC:
> -            memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>              if (flow->dl_type == htons(ETH_TYPE_IP)) {
>                  flow->nw_src = ofpact_get_SET_IPV4_SRC(a)->ipv4;
>              }
>              break;
> 
>          case OFPACT_SET_IPV4_DST:
> -            memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>              if (flow->dl_type == htons(ETH_TYPE_IP)) {
>                  flow->nw_dst = ofpact_get_SET_IPV4_DST(a)->ipv4;
>              }
> @@ -1609,7 +1602,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> 
>          case OFPACT_SET_IPV4_DSCP:
>              /* OpenFlow 1.0 only supports IPv4. */
> -            memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>              if (flow->dl_type == htons(ETH_TYPE_IP)) {
>                  flow->nw_tos &= ~IP_DSCP_MASK;
>                  flow->nw_tos |= ofpact_get_SET_IPV4_DSCP(a)->dscp;
> @@ -1617,7 +1609,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>              break;
> 
>          case OFPACT_SET_L4_SRC_PORT:
> -            memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>              memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>              if (is_ip_any(flow)) {
>                  flow->tp_src = htons(ofpact_get_SET_L4_SRC_PORT(a)->port);
> @@ -1625,7 +1616,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>              break;
> 
>          case OFPACT_SET_L4_DST_PORT:
> -            memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>              memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>              if (is_ip_any(flow)) {
>                  flow->tp_dst = htons(ofpact_get_SET_L4_DST_PORT(a)->port);
> @@ -1687,7 +1677,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>              break;
> 
>          case OFPACT_DEC_TTL:
> -            memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>              if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) {
>                  goto out;
>              }
> @@ -1719,7 +1708,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>              break;
> 
>          case OFPACT_FIN_TIMEOUT:
> -            memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>              memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>              ctx->xout->has_fin_timeout = true;
>              xlate_fin_timeout(ctx, ofpact_get_FIN_TIMEOUT(a));
> @@ -1908,6 +1896,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>      flow_wildcards_init_catchall(wc);
>      memset(&wc->masks.in_port, 0xff, sizeof wc->masks.in_port);
>      memset(&wc->masks.skb_priority, 0xff, sizeof wc->masks.skb_priority);
> +    memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>      wc->masks.nw_frag |= FLOW_NW_FRAG_MASK;
> 
>      if (tnl_port_should_receive(&ctx.xin->flow)) {
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 2fff66e..c6a7abc 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5174,6 +5174,7 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto,
>      }
> 
>      if (wc) {
> +        memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>          wc->masks.nw_frag |= FLOW_NW_FRAG_MASK;
>      }
> 
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 21ca5e7..88492cf 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -2227,7 +2227,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>  AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),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 netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
>  AT_CHECK([ovs-appctl dpif/dump-megaflows br0 | STRIP_XOUT], [0], [dnl
> -skb_priority=0,in_port=1,nw_frag=no, n_subfacets:2, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=1,nw_frag=no, n_subfacets:2, used:0.0s, Datapath actions: <del>
>  ])
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> @@ -2242,8 +2242,8 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>  AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),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 netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
>  AT_CHECK([ovs-appctl dpif/dump-megaflows br0 | STRIP_XOUT], [0], [dnl
> -skb_priority=0,in_port=1,dl_src=50:54:00:00:00:09,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> -skb_priority=0,in_port=1,dl_src=50:54:00:00:00:0b,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=1,dl_src=50:54:00:00:00:09,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=1,dl_src=50:54:00:00:00:0b,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
>  ])
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> @@ -2474,7 +2474,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>  AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),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 netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
>  AT_CHECK([ovs-appctl dpif/dump-megaflows br0 | STRIP_XOUT], [0], [dnl
> -skb_priority=0,in_port=1,nw_frag=no, n_subfacets:2, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=1,nw_frag=no, n_subfacets:2, used:0.0s, Datapath actions: <del>
>  ])
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> @@ -2494,8 +2494,8 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>  AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x8100),vlan(vid=11,pcp=7),encap(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 netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
>  AT_CHECK([ovs-appctl dpif/dump-megaflows br0 | STRIP_XOUT], [0], [dnl
> -skb_priority=0,in_port=1,dl_vlan=11,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> -skb_priority=0,in_port=1,vlan_tci=0x0000/0x1fff,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=1,dl_vlan=11,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=1,vlan_tci=0x0000/0x1fff,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
>  ])
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> @@ -2545,8 +2545,8 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:
>  AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
>  dnl The original flow is missing due to a revalidation.
>  AT_CHECK([ovs-appctl dpif/dump-megaflows br0 | STRIP_XOUT], [0], [dnl
> -skb_priority=0,in_port=1,vlan_tci=0x0000/0x0fff,dl_src=50:54:00:00:00:09,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> -skb_priority=0,in_port=1,vlan_tci=0x0000/0x0fff,dl_src=50:54:00:00:00:0b,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=1,vlan_tci=0x0000/0x0fff,dl_src=50:54:00:00:00:09,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=1,vlan_tci=0x0000/0x0fff,dl_src=50:54:00:00:00:0b,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
>  ])
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> @@ -2573,9 +2573,9 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:
>  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=0xfd,ttl=128,frag=no),icmp(type=8,code=0)'])
>  AT_CHECK([ovs-appctl netdev-dummy/receive p3 'in_port(3),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0x1,ttl=64,frag=no),icmp(type=8,code=0)'])
>  AT_CHECK([ovs-appctl dpif/dump-megaflows br0 | STRIP_XOUT], [0], [dnl
> -skb_priority=0,in_port=1,nw_ecn=1,nw_frag=no, n_subfacets:2, used:0.0s, Datapath actions: <del>
> -skb_priority=0,in_port=3,nw_tos=0,nw_ecn=1,nw_ttl=64,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> -skb_priority=0,in_port=3,nw_tos=252,nw_ecn=1,nw_ttl=128,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=1,nw_ecn=1,nw_frag=no, n_subfacets:2, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=3,nw_tos=0,nw_ecn=1,nw_ttl=64,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
> +skb_priority=0,ip,in_port=3,nw_tos=252,nw_ecn=1,nw_ttl=128,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: <del>
>  ])
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> --
> 1.7.9.5
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list