[ovs-dev] [PATCH] dpif: Add much more documentation.

Justin Pettit jpettit at nicira.com
Wed Jan 9 02:16:58 UTC 2013


This is great!  Thanks for taking the time to write it up.  I just have a few comments:

> + *    - A "flow", that is, a summary of the headers in a Ethernet packet.  The

s/a/an/

This sort of sounds like only the Ethernet header fields make up the flow.  Maybe "L2/L3/L4 headers" or something like that?

> + *      (In case you are familiar with OpenFlow, datapath flows are analogous
> + *      to OpenFlow flow matches.  The most important difference is that
> + *      OpenFlow allows fields to be wildcarded, whereas a datapath's flow
> + *      table is a hash table so every flow must be exact-match.)


I might add "and prioritized" after "wildcarded", since this often seems to trip people up in understanding the datapath flow table.

> + *      The actions list may be empty.  This indicates that nothing should be
> + *      done to matching packet, e.g. they should be dropped.

s/packet/packets/

Is this an "e.g." or an "i.e."?  Isn't the packet always going to be dropped?

> + * An upcall contains an entire packet.  There is no attempt to, e.g., copy
> + * only as much of the packet as normally needed to make a forwarding decision.
> + * Such an optimization is doable, but experimental prototypes showed it to be
> + * of little benefit because an upcall typically contains the first packet of a
> + * flow, which is usually short (e.g. a TCP SYN).


I'm not sure we want to only use this justification, since we also use the packet for things like packet sampling and deeper inspection for in-band.

> + * The datapath should ensure that that a high rate of upcalls from one


There are two "that"s.

> + * The client has some control over "action" upcalls: it can specify a 32-bit
> + * "Netlink PID" as part of the action.  This terminology comes from the Linux
> + * datapath implementation, which uses a protocol called Netlink in which a PID
> + * designates a particular socket and the upcall data is delivered to the
> + * socket's received queue.  Generically, though, a Netlink PID identifies a
> + * queue for upcalls.  The basic requirements on the datapath are:

Is it a "received queue" or a "receive queue"?  I always thought it was the latter (i.e., no "d").

> + *
> + *    - The datapath must provide a Netlink PID associated with each port.  The
> + *      client can retrieve the PID with dpif_port_get_pid().
> + *
> + *    - The datapath must provide an additional Netlink PID, not associated
> + *      with any port.  dpif_port_get_pid() also provides this PID.

I think it would be nice to explain why this other PID is needed (and possibly explain that the value is UINT32_MAX).

> + *    - Upcalls that specify the additional Netlink PID are queued separately.

Calling this the "additional Netlink PID" seems insufficiently specific.  What about calling it something like the "system Netlink PID" here and where it was introduced earlier? 

> + * For each upcall received, the client examines the enclosed packet and
> + * figures out what should be done with it.  For example, if the client
> + * implements a MAC-learning switch, then it searches the forwarding database
> + * for the packet's destination MAC and VLAN and determines the set of ports to
> + * which it should be sent.  In any case, the client composes a set of datapath
> + * actions to properly dispatch the packet and then directs the datapath to
> + * execute those actions on the packet (e.g. with dpif_execute()).

Is it an "e.g." or an "i.e."?

--Justin





More information about the dev mailing list