On Thu, Sep 22, 2011 at 11:33 AM, Ben Pfaff <b...@nicira.com> wrote: > 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 fixed all the typos. > 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. This problem is actually common to all of the methods that add ports/flows/packets. I think maybe the best solution is repurpose pid 0 to mean "don't send upcalls". This would allow userspace to completely shutdown a bad port and when it wants to start listening again change it to a real pid. > 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. Hmm, I suppose. I changed it to log an error but otherwise continue on. > In find_dpif(), as a micro-optimization you could use > HMAP_FOR_EACH_IN_BUCKET in place of HMAP_FOR_EACH_WITH_HASH. You mean because comparing the ifindex is just as cheap as comparing a hash and doing both is redundant? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
