[ovs-dev] [bug 7759 06/11] datapath: Move Netlink PID for userspace actions from flows to actions.

[Jesse Gross]

On Wed, Oct 12, 2011 at 10:55 AM, Ben Pfaff <blp at nicira.com> wrote:
> On Tue, Oct 11, 2011 at 05:34:08PM -0700, Jesse Gross wrote:
>> On Wed, Oct 5, 2011 at 11:27 AM, Ben Pfaff <blp at nicira.com> wrote:
>> > diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
>> > index 3db960a..d058899 100644
>> > --- a/include/openvswitch/datapath-protocol.h
>> > +++ b/include/openvswitch/datapath-protocol.h
>> > +/**
>> > + * enum ovs_userspace_attr - Attributes for %OVS_ACTION_ATTR_USERSPACE action.
>> > + * @OVS_USERSPACE_ATTR_PID: u32 Netlink PID to which the %OVS_PACKET_CMD_ACTION
>> > + * message should be sent. ??Required.
>> > + * @OVS_USERSPACE_ATTR_USERDATA: If present and nonzero, its u64 argument is
>> > + * copied to the %OVS_PACKET_CMD_ACTION message as %OVS_PACKET_ATTR_USERDATA,
>>
>> At the moment, the kernel actually always sends the userdata for
>> command upcalls - it seems a little nicer to not assume that zero is a
>> special value.  Probably the best thing to do is actually send
>> userdata if and only if it was actually set up originally.
>
> OK, incremental (full replacement patch at end of email):

Looks good, although I think there are some other places in
datapath-protocol.h that refer to not passing up non-zero userdata.

>> > diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
>> > index 79e84d6..1395c99 100644
>> > --- a/lib/dpif-linux.c
>> > +++ b/lib/dpif-linux.c
>> > +static uint32_t
>> > +dpif_linux_port_get_pid(const struct dpif *dpif_, uint16_t port_no)
>> > +{
>> > + ?? ??struct dpif_linux *dpif = dpif_linux_cast(dpif_);
>> > +
>> > + ?? ??if (!(dpif->listen_mask & (1u << DPIF_UC_MISS))) {
>> > + ?? ?? ?? ??return 0;
>>
>> Since this is now used for ACTION upcalls as well, I don't think this
>> mask is quite right.
>
> OK, an incremental follows.
>
> I think that the concept here is now wrong, though.  It used to make
> sense to turn off action upcalls, but now userspace has to take control
> of enabling or disabling them anyway.  Another former purpose was that
> clients independent of the client that set up the flows used to be able
> to sign up to get upcalls, but that isn't supported now either.
>
> It would map better to the implementation now for dpif to have a
> per-vport knob to enable or disable miss upcalls, and drop the action
> upcall knob entirely.  If you agree I'll write up a separate patch.

Yes, I was also thinking that the concept of listen mask is pretty
strange now.  Per-vport control over miss upcalls sounds reasonable
and is potentially something that we'll want to use a little more
actively in the future than we currently do with listen mask.  This
incremental looks fine for the time being though.

So other than the comments in datapath-protocol.h, this looks good to me:
Acked-by: Jesse Gross <jesse at nicira.com>