[ovs-dev] [PATCH 3/7] userspace: Switching of L3 packets in L2 pipeline

Jan Scheurich jan.scheurich at ericsson.com
Mon Mar 6 00:37:11 UTC 2017


> Please don't add comments about changes, like this one.  Readers should
> not have to understand the history to understand the code:
> +        if (put->flags & DPIF_FP_MODIFY) {
> +            /* Removed the additional check
> +             * flow_equal(&match.flow, &netdev_flow->flow) as a) the
> +             * dpcls lookup is sufficient to uniquely identify a flow
> +             * and b) it caused false negatives because the flow in
> +             * netdev->flow may not properly be masked. */
> The right place to explain changes is in the commit message.  If there
> needs to be extra explanation of an algorithm in comments, that's
> perfectly appropriate, but it should talk about the code as it exists,
> not as it once did.

OK, will do.

> I see that match_format() prints packet_type as a hex number.  I doubt
> that's the best for human consumption.  At the very least, it seems like
> it's better printed as a pair of numbers, and probably it would be more
> friendly to say what packet type it is.

I agree that a pair of numbers would be more user friendly. Would you prefer packet_type=(1,0x800) or packet_type={1,0x800} or some other notation. 

For the most common Ethernet packet_type (0,0) we would print packet_type=ethernet.

Symbolic packet_type representation for namespace 1 could easily lead to confusion with the legacy match short-hands such as "ip", "ipv6", "arp", "udp", etc. I believe these will need to remain untouched, e.g. packet_type=0 && dl_type=0x800 && nw_proto=17 for "udp", or we would break a lot of legacy.

Here we should really agree on something up front. Changing the notation is very painful due to the large number of unit test cases that have to be adapted.

> 
> Similarly in format_odp_key_attr().
> 
> What is the value of this change?  format_eth_masked() incorporates the
> same check that it adds:
> 
> @@ -1231,12 +1242,16 @@ match_format(const struct match *match, struct ds *s, int priority)
>              ds_put_format(s, "%svlan_tci=%s0x%04"PRIx16"/0x%04"PRIx16",",
>                            colors.param, colors.end,
>                            ntohs(f->vlan_tci), ntohs(wc->masks.vlan_tci));
>          }
>      }
> -    format_eth_masked(s, "dl_src", f->dl_src, wc->masks.dl_src);
> -    format_eth_masked(s, "dl_dst", f->dl_dst, wc->masks.dl_dst);
> +    if (!eth_addr_is_zero(wc->masks.dl_src)) {
> +        format_eth_masked(s, "dl_src", f->dl_src, wc->masks.dl_src);
> +    }
> +    if (!eth_addr_is_zero(wc->masks.dl_dst)) {
> +        format_eth_masked(s, "dl_dst", f->dl_dst, wc->masks.dl_dst);
> +    }

Good spot. Will remove.

> 
> We generally avoid memset when another choice is available, e.g.:
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 5d0ed86d46df..b571d83d56a8 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3149,8 +3149,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
>      else if (flow->packet_type != PT_ETH && !xport->is_layer3) {
>          /* L2 outport and non-ethernet packet_type -> add dummy eth header. */
>          flow->packet_type = PT_ETH;
> -        memset(&flow->dl_dst, 0,ETH_ADDR_LEN);
> -        memset(&flow->dl_src, 0,ETH_ADDR_LEN);
> +        flow->dl_dst = eth_addr_zero;
> +        flow->dl_src = eth_addr_zero;
>      }

Will fix that.

> Can we fix whatever problem this is instead of working around it?
> 
> +    /* Wildcard ethernet addresses if the original packet type was not
> +     * Ethernet.
> +     * XXX: This is a work-around. ofproto shouldn't unwildcard the Ethernet
> +     * addresses at all. */
> +    if (ctx->xin->upcall_flow->packet_type != PT_ETH) {
> +        memset(&ctx->wc->masks.dl_dst, 0, ETH_ADDR_LEN);
> +        memset(&ctx->wc->masks.dl_src, 0, ETH_ADDR_LEN);
> +    }

I had a discussion with Jarno about this (https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327510.html) and agreed that this is needed unless ofproto-dpif-xlate maintains a separate mask to keep track of the field bits that have been set independently from the bits that have been matched. That might well be worth doing to get wider, more generic megaflows, but it should be a separate patch series. I can remove the XXX: comment.

> and this?
> 
> +    /* XXX: ONLY FOR NON-PTAP BRIDGE! */
> +    if (flow->packet_type != PT_ETH && in_port && in_port->is_layer3 &&
> +            ctx.table_id == 0) {
> +        /* Add dummy Ethernet header to non-L2 packet if it's coming from a
> +         * L3 port. So all packets will be L2 packets for lookup.
> +         * The dl_type has already been set from the packet_type. */
> +        flow->packet_type = PT_ETH;
> +        memset(&flow->dl_src, 0, ETH_ADDR_LEN);
> +        memset(&flow->dl_dst, 0, ETH_ADDR_LEN);
> +    }

This is not a work-around. Again the comment was more for us to remember what needs revising when implementing PTAP bridges. We can remove it.



More information about the dev mailing list