[ovs-dev] [PATCH 2/5] datapath: Use unicast Netlink sockets for upcalls.

[Ben Pfaff]

On Mon, Sep 19, 2011 at 03:00:05PM -0700, Jesse Gross wrote:
> Currently we publish several multicast groups for upcalls and let
> userspace sockets subscribe to them.  The benefit of this is mostly
> that userspace is the one doing the subscription - the actual
> multicast capability is not currently used and probably wouldn't be
> even if we moved to a multiprocess model.  Despite the convenience,
> multicast sockets have a number of disadvantages, primarily that
> we only have a limited number of them so there could be collisions.
> In addition, unicast sockets give additional flexibility to userspace
> by allowing every object to potentially have a different socket
> chosen by userspace for upcalls.  Finally, any future optimizations
> for upcalls to reduce copying will likely not be compatible with
> multicast anyways so disallowing it potentially simplifies things.
> 
> We also never unregistered the multicast groups registered for upcalls
> and leaked them on module unload.  As a side effect, this solves that
> problem.
> 
> Signed-off-by: Jesse Gross <jesse at nicira.com>

There's a remaining mention of an sflow multicast group in datapath.h:
 * @sflow_probability: Number of packets out of UINT_MAX to sample to the
 * %OVS_PACKET_CMD_SAMPLE multicast group, e.g. (@sflow_probability/UINT_MAX)
 * is the probability of sampling a given packet.

In datapath-protocol.h, s/trigged/triggered/:
+ * OVS_PACKET_CMD_SAMPLE upcalls will be directed to for actions trigged by

Also the rest of the comments around this one end in ".":
+    OVS_PACKET_ATTR_UPCALL_PID,  /* Netlink PID to receive upcalls */

In lib/dpif-linux.c, s/dpif_list/dpif_linux/:
+/* Maps from dp_ifindex to struct dpif_list. */

I think that doing dpif_flow_put() when listen_mask is 0 will now
segfault since dpif->upcall_sock will be NULL.  It's hard for it to do
the "right thing"--what should upcall_pid be?--but a segfault seems a
bit much.

The error handling in dpif_linux_recv_set_mask() seems a bit severe.
If someone runs "ovs-dpctl del-flows" on a datapath that we're working
on, to flush the flow cache, and that causes an OVS_FLOW_CMD_SET to
fail, then we give up entirely.  It's an unlikely race, and really the
admin shouldn't be doing that, but that might be too strong.

In find_dpif(), as a micro-optimization you could use
HMAP_FOR_EACH_IN_BUCKET in place of HMAP_FOR_EACH_WITH_HASH.

Otherwise looks good, thank you.