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

Justin Pettit jpettit at nicira.com
Mon Oct 29 21:11:51 UTC 2012


On Thu, Oct 18, 2012 at 4:42 PM, Ben Pfaff <blp at nicira.com> wrote:

> 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.
>

Okay, it should be clearer now.


> 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). */
>

Great suggestions.  Done.


> 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));
>

Fixed.  Isn't this a bug in the existing code?  Should I do a separate
patch to fix this in existing branches?


> 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.
>

Good point.  I made that change.

Thanks.

--Justin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20121029/83feab19/attachment-0003.html>


More information about the dev mailing list