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

Jesse Gross jesse at nicira.com
Thu Sep 22 20:50:26 UTC 2011


On Thu, Sep 22, 2011 at 11:33 AM, Ben Pfaff <blp at 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 <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 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?



More information about the dev mailing list