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 <je...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
