On Wed, Oct 12, 2011 at 01:48:35PM -0700, Jesse Gross wrote: > On Wed, Oct 12, 2011 at 10:55 AM, Ben Pfaff <[email protected]> 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 <[email protected]> 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 <[email protected]>
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 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
