On Thu, Oct 18, 2012 at 4:42 PM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Thu, Oct 18, 2012 at 12:51:49PM -0700, Justin Pettit wrote:<br>
> The current code has a simple mapping between datapath and OpenFlow port<br>
> numbers (1-to-1 other than the local port).<br>
<br>
</div>The important fact is not really that the mapping is one-to-one, it's<br>
that the port numbers are actually *the same* except OFPP_LOCAL <-> 0,<br>
and that that is statically known at compile time.<br></blockquote><div><br></div><div>Okay, it should be clearer now.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think something is inconsistent in dpif_netdev_flow_from_nlattrs().<br>
The function odp_flow_key_to_flow() that it calls is documented to put<br>
a datapath port number in the flow that it returns, but<br>
dpif_netdev_flow_from_nlattrs() then compares the returned port number<br>
against OFPP_MAX, OFPP_LOCAL, and OFPP_NONE.<br>
<br>
I think that the comments could be improved. Suggestion for<br>
odp_flow_key_from_flow():<br>
* 'flow->in_port' is ignored (since it is likely to be an OpenFlow port number<br>
* rather than a datapath port number). Instead, if 'odp_in_port' is anything<br>
* other than OVSP_NONE, it is included in 'buf' as the input port.<br>
<br>
Suggestion for struct flow:<br>
<br>
/*<br>
* A flow in the network.<br>
*<br>
* The meaning of 'in_port' is context-dependent. In most cases, it is a<br>
* 16-bit OpenFlow 1.0 port number. In the software datapath interface (dpif)<br>
* layer and its implementations (e.g. dpif-linux, dpif-netdev), it is instead<br>
* a 32-bit datapath port number.<br>
*/<br>
struct flow {<br>
struct flow_tnl tunnel; /* Encapsulating tunnel parameters. */<br>
ovs_be64 metadata; /* OpenFlow Metadata. */<br>
struct in6_addr ipv6_src; /* IPv6 source address. */<br>
struct in6_addr ipv6_dst; /* IPv6 destination address. */<br>
struct in6_addr nd_target; /* IPv6 neighbor discovery (ND) target. */<br>
uint32_t skb_priority; /* Packet priority for QoS. */<br>
uint32_t regs[FLOW_N_REGS]; /* Registers. */<br>
<div class="im"> ovs_be32 nw_src; /* IPv4 source address. */<br>
ovs_be32 nw_dst; /* IPv4 destination address. */<br>
ovs_be32 ipv6_label; /* IPv6 flow label. */<br>
</div> uint32_t in_port; /* Input port (see above). */<br></blockquote><div><br></div><div>Great suggestions. Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In ofproto_unixctl_trace(), it looks very much to me like the conversion<br>
to an ODP port in the argc == 6 case should not be happening, that is,<br>
that the ofp_port_to_odp_port call should be omitted here:<br>
uint32_t in_port = ofp_port_to_odp_port(atoi(in_port_s));<br></blockquote><div><br></div><div>
<p class="p1">Fixed. Isn't this a bug in the existing code? Should I do a separate patch to fix this in existing branches?</p></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Are there any callers to ofp_port_to_odp_port() that couldn't handle the<br>
extra semantics that ofp_port_to_odp_in_port() provides? I think that<br>
we could just fold the latter into the former.<br></blockquote><div><br></div><div>Good point. I made that change.</div><div><br></div><div>Thanks.</div><div><br></div><div>--Justin</div><div> </div><div><br></div></div>