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

[Ben Pfaff]

On Wed, Oct 12, 2011 at 01:48:35PM -0700, Jesse Gross wrote:
> 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>

I posted a simpler patch that just drops the distinction between the two
kinds of upcalls from a dpif perspective:
http://openvswitch.org/pipermail/dev/2011-November/013444.html