On Thu, Oct 18, 2012 at 12:51:49PM -0700, Justin Pettit wrote:
> The current code has a simple mapping between datapath and OpenFlow port
> numbers (1-to-1 other than the local port).
The important fact is not really that the mapping is one-to-one, it's
that the port numbers are actually *the same* except OFPP_LOCAL <-> 0,
and that that is statically known at compile time.
> This allowed different layers to easily translate between the two, so
> the translation often occurred late.
>
> A future commit will break this simple mapping, so this commit draws a
> line between where datapath and OpenFlow port numbers are used. The
> ofproto-dpif layer will be responsible for the translations. Callers
> above will use OpenFlow port numbers. Providers below will use
> datapath port numbers.
>
> Signed-off-by: Justin Pettit <jpet...@nicira.com>
I think something is inconsistent in dpif_netdev_flow_from_nlattrs().
The function odp_flow_key_to_flow() that it calls is documented to put
a datapath port number in the flow that it returns, but
dpif_netdev_flow_from_nlattrs() then compares the returned port number
against OFPP_MAX, OFPP_LOCAL, and OFPP_NONE.
I think that the comments could be improved. Suggestion for
odp_flow_key_from_flow():
* 'flow->in_port' is ignored (since it is likely to be an OpenFlow port number
* rather than a datapath port number). Instead, if 'odp_in_port' is anything
* other than OVSP_NONE, it is included in 'buf' as the input port.
Suggestion for struct flow:
/*
* A flow in the network.
*
* The meaning of 'in_port' is context-dependent. In most cases, it is a
* 16-bit OpenFlow 1.0 port number. In the software datapath interface (dpif)
* layer and its implementations (e.g. dpif-linux, dpif-netdev), it is instead
* a 32-bit datapath port number.
*/
struct flow {
struct flow_tnl tunnel; /* Encapsulating tunnel parameters. */
ovs_be64 metadata; /* OpenFlow Metadata. */
struct in6_addr ipv6_src; /* IPv6 source address. */
struct in6_addr ipv6_dst; /* IPv6 destination address. */
struct in6_addr nd_target; /* IPv6 neighbor discovery (ND) target. */
uint32_t skb_priority; /* Packet priority for QoS. */
uint32_t regs[FLOW_N_REGS]; /* Registers. */
ovs_be32 nw_src; /* IPv4 source address. */
ovs_be32 nw_dst; /* IPv4 destination address. */
ovs_be32 ipv6_label; /* IPv6 flow label. */
uint32_t in_port; /* Input port (see above). */
In ofproto_unixctl_trace(), it looks very much to me like the conversion
to an ODP port in the argc == 6 case should not be happening, that is,
that the ofp_port_to_odp_port call should be omitted here:
uint32_t in_port = ofp_port_to_odp_port(atoi(in_port_s));
Are there any callers to ofp_port_to_odp_port() that couldn't handle the
extra semantics that ofp_port_to_odp_in_port() provides? I think that
we could just fold the latter into the former.
Thanks for doing this.
Ben
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
