On Fri, Feb 03, 2017 at 10:40:09AM +0000, Jan Scheurich wrote:
> Ports have a new layer3 attribute if they send/receive L3 packets.
>
> The packet_type included in structs dp_packet and flow is considered in
> ofproto-dpif. The classical L2 match fields (dl_src, dl_dst, dl_type, and
> vlan_tci, vlan_vid, vlan_pcp) now have Ethernet as pre-requisite.
>
> A dummy ethernet header is pushed to L3 packets received from L3 ports
> before the pipeline processing starts. The ethernet header is popped
> before sending a packet to a L3 port.
>
> For datapath ports that can receive L2 or L3 packets, the packet_type
> becomes part of the flow key for datapath flows and is handled
> appropriately in dpif-netdev.
>
> Signed-off-by: Lorand Jakab <[email protected]>
> Signed-off-by: Simon Horman <[email protected]>
> Signed-off-by: Jiri Benc <[email protected]>
> Signed-off-by: Yi Yang <[email protected]>
> Signed-off-by: Jan Scheurich <[email protected]>
> Co-authored-by: Zoltan Balogh <[email protected]>
dp_netdev_flow_add() should use VLOG_DROP_DBG() once, and then
VLOG_DBG() twice if it OKs it, instead of VLOG_DBG_RL twice. Otherwise
you can get the first log message but not the second one, which will be
less useful.
s/isntalled/installed/ in the comment in the same function.
This patch makes some white space only changes to dp_netdev_flow_add().
Please don't.
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.
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.
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);
+ }
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;
}
if (xport->peer) {
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);
+ }
+
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);
+ }
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
