[ovs-dev] [Single DP 04/15] Use ODP ports in dpif layer and below.

Ben Pfaff blp at nicira.com
Thu Oct 18 23:42:26 UTC 2012

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 <jpettit at 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

I think that the comments could be improved.  Suggestion for
 * '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.


More information about the dev mailing list