Thank you, I pushed this to master.
On Thu, Jan 12, 2012 at 04:24:31PM -0800, Ethan Jackson wrote: > Looks good. > > Ethan > > On Wed, Nov 30, 2011 at 15:08, Ben Pfaff <b...@nicira.com> wrote: > > At one point in the past, there were three separate queues between the > > kernel module and OVS userspace, each of which corresponded to a Netlink > > socket (or, before that, to a character device). ?It made sense to allow > > each of these to be enabled or disabled separately, hence the "listen mask" > > concept in the dpif layer. > > > > These days, the concept is much less clear-cut. ?Queuing is no longer on > > the basis of different classes of packets but instead striped across a > > collection of sockets based on input port. ?It doesn't really make sense > > to enable receiving packets on the basis of the kind of packet anymore. > > Accordingly, this commit simplifies the "listen_mask" to just a bool that > > either enables or disables receiving packets. > > > > It could be useful to enable or disable receiving packets on a per-vport > > basis, but the rest of the code isn't ready to make use of that so this > > commit doesn't generalize this much. > > > > Based on this discussion on ovs-dev: > > http://openvswitch.org/pipermail/dev/2011-October/012044.html > > --- > > ?lib/dpif-linux.c ? ? ? | ? 52 > > ++++++++++++----------------------------------- > > ?lib/dpif-netdev.c ? ? ?| ? 21 ++---------------- > > ?lib/dpif-provider.h ? ?| ? 20 ++++------------- > > ?lib/dpif.c ? ? ? ? ? ? | ? 46 +++++++----------------------------------- > > ?lib/dpif.h ? ? ? ? ? ? | ? ?3 +- > > ?ofproto/ofproto-dpif.c | ? ?4 +-- > > ?6 files changed, 32 insertions(+), 114 deletions(-) > > > > diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c > > index 1f6c2c0..292dd1e 100644 > > --- a/lib/dpif-linux.c > > +++ b/lib/dpif-linux.c > > @@ -140,7 +140,6 @@ struct dpif_linux { > > ? ? /* Upcall messages. */ > > ? ? struct nl_sock *upcall_socks[N_UPCALL_SOCKS]; > > ? ? uint32_t ready_mask; ? ? ? ?/* 1-bit for each sock with unread > > messages. */ > > - ? ?unsigned int listen_mask; ? /* Mask of DPIF_UC_* bits. */ > > ? ? int epoll_fd; ? ? ? ? ? ? ? /* epoll fd that includes the upcall socks. > > */ > > > > ? ? /* Change notification. */ > > @@ -171,9 +170,7 @@ static int dpif_linux_init(void); > > ?static void open_dpif(const struct dpif_linux_dp *, struct dpif **); > > ?static bool dpif_linux_nln_parse(struct ofpbuf *, void *); > > ?static void dpif_linux_port_changed(const void *vport, void *dpif); > > -static uint32_t dpif_linux_port_get_pid__(const struct dpif *, > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?uint16_t port_no, > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?enum dpif_upcall_type); > > +static uint32_t dpif_linux_port_get_pid(const struct dpif *, uint16_t > > port_no); > > > > ?static void dpif_linux_vport_to_ofpbuf(const struct dpif_linux_vport *, > > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct ofpbuf *); > > @@ -404,8 +401,7 @@ dpif_linux_port_add(struct dpif *dpif_, struct netdev > > *netdev, > > ? ? ? ? uint32_t upcall_pid; > > > > ? ? ? ? request.port_no = dpif_linux_pop_port(dpif); > > - ? ? ? ?upcall_pid = dpif_linux_port_get_pid__(dpif_, request.port_no, > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DPIF_UC_MISS); > > + ? ? ? ?upcall_pid = dpif_linux_port_get_pid(dpif_, request.port_no); > > ? ? ? ? request.upcall_pid = &upcall_pid; > > ? ? ? ? error = dpif_linux_vport_transact(&request, &reply, &buf); > > > > @@ -488,12 +484,11 @@ dpif_linux_get_max_ports(const struct dpif *dpif > > OVS_UNUSED) > > ?} > > > > ?static uint32_t > > -dpif_linux_port_get_pid__(const struct dpif *dpif_, uint16_t port_no, > > - ? ? ? ? ? ? ? ? ? ? ? ? ?enum dpif_upcall_type upcall_type) > > +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 << upcall_type))) { > > + ? ?if (dpif->epoll_fd < 0) { > > ? ? ? ? return 0; > > ? ? } else { > > ? ? ? ? int idx = port_no & (N_UPCALL_SOCKS - 1); > > @@ -501,12 +496,6 @@ dpif_linux_port_get_pid__(const struct dpif *dpif_, > > uint16_t port_no, > > ? ? } > > ?} > > > > -static uint32_t > > -dpif_linux_port_get_pid(const struct dpif *dpif, uint16_t port_no) > > -{ > > - ? ?return dpif_linux_port_get_pid__(dpif, port_no, DPIF_UC_ACTION); > > -} > > - > > ?static int > > ?dpif_linux_flow_flush(struct dpif *dpif_) > > ?{ > > @@ -959,14 +948,6 @@ dpif_linux_operate(struct dpif *dpif_, union dpif_op > > **ops, size_t n_ops) > > ? ? free(txns); > > ?} > > > > -static int > > -dpif_linux_recv_get_mask(const struct dpif *dpif_, int *listen_mask) > > -{ > > - ? ?struct dpif_linux *dpif = dpif_linux_cast(dpif_); > > - ? ?*listen_mask = dpif->listen_mask; > > - ? ?return 0; > > -} > > - > > ?static void > > ?set_upcall_pids(struct dpif *dpif_) > > ?{ > > @@ -976,8 +957,7 @@ set_upcall_pids(struct dpif *dpif_) > > ? ? int error; > > > > ? ? DPIF_PORT_FOR_EACH (&port, &port_dump, &dpif->dpif) { > > - ? ? ? ?uint32_t upcall_pid = dpif_linux_port_get_pid__(dpif_, > > port.port_no, > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?DPIF_UC_MISS); > > + ? ? ? ?uint32_t upcall_pid = dpif_linux_port_get_pid(dpif_, port.port_no); > > ? ? ? ? struct dpif_linux_vport vport_request; > > > > ? ? ? ? dpif_linux_vport_init(&vport_request); > > @@ -998,17 +978,17 @@ set_upcall_pids(struct dpif *dpif_) > > ?} > > > > ?static int > > -dpif_linux_recv_set_mask(struct dpif *dpif_, int listen_mask) > > +dpif_linux_recv_set(struct dpif *dpif_, bool enable) > > ?{ > > ? ? struct dpif_linux *dpif = dpif_linux_cast(dpif_); > > > > - ? ?if (listen_mask == dpif->listen_mask) { > > + ? ?if ((dpif->epoll_fd >= 0) == enable) { > > ? ? ? ? return 0; > > ? ? } > > > > - ? ?if (!listen_mask) { > > + ? ?if (!enable) { > > ? ? ? ? destroy_upcall_socks(dpif); > > - ? ?} else if (!dpif->listen_mask) { > > + ? ?} else { > > ? ? ? ? int i; > > ? ? ? ? int error; > > > > @@ -1039,7 +1019,6 @@ dpif_linux_recv_set_mask(struct dpif *dpif_, int > > listen_mask) > > ? ? ? ? dpif->ready_mask = 0; > > ? ? } > > > > - ? ?dpif->listen_mask = listen_mask; > > ? ? set_upcall_pids(dpif_); > > > > ? ? return 0; > > @@ -1118,7 +1097,7 @@ dpif_linux_recv(struct dpif *dpif_, struct > > dpif_upcall *upcall) > > ? ? struct dpif_linux *dpif = dpif_linux_cast(dpif_); > > ? ? int read_tries = 0; > > > > - ? ?if (!dpif->listen_mask) { > > + ? ?if (dpif->epoll_fd < 0) { > > ? ? ? ?return EAGAIN; > > ? ? } > > > > @@ -1163,9 +1142,7 @@ dpif_linux_recv(struct dpif *dpif_, struct > > dpif_upcall *upcall) > > ? ? ? ? ? ? } > > > > ? ? ? ? ? ? error = parse_odp_packet(buf, upcall, &dp_ifindex); > > - ? ? ? ? ? ?if (!error > > - ? ? ? ? ? ? ? ?&& dp_ifindex == dpif->dp_ifindex > > - ? ? ? ? ? ? ? ?&& dpif->listen_mask & (1u << upcall->type)) { > > + ? ? ? ? ? ?if (!error && dp_ifindex == dpif->dp_ifindex) { > > ? ? ? ? ? ? ? ? return 0; > > ? ? ? ? ? ? } > > > > @@ -1184,7 +1161,7 @@ dpif_linux_recv_wait(struct dpif *dpif_) > > ?{ > > ? ? struct dpif_linux *dpif = dpif_linux_cast(dpif_); > > > > - ? ?if (!dpif->listen_mask) { > > + ? ?if (dpif->epoll_fd < 0) { > > ? ? ? ?return; > > ? ? } > > > > @@ -1197,7 +1174,7 @@ dpif_linux_recv_purge(struct dpif *dpif_) > > ? ? struct dpif_linux *dpif = dpif_linux_cast(dpif_); > > ? ? int i; > > > > - ? ?if (!dpif->listen_mask) { > > + ? ?if (dpif->epoll_fd < 0) { > > ? ? ? ?return; > > ? ? } > > > > @@ -1235,8 +1212,7 @@ const struct dpif_class dpif_linux_class = { > > ? ? dpif_linux_flow_dump_done, > > ? ? dpif_linux_execute, > > ? ? dpif_linux_operate, > > - ? ?dpif_linux_recv_get_mask, > > - ? ?dpif_linux_recv_set_mask, > > + ? ?dpif_linux_recv_set, > > ? ? dpif_linux_queue_to_priority, > > ? ? dpif_linux_recv, > > ? ? dpif_linux_recv_wait, > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index eb10134..c004704 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -123,7 +123,6 @@ struct dp_netdev_flow { > > ?struct dpif_netdev { > > ? ? struct dpif dpif; > > ? ? struct dp_netdev *dp; > > - ? ?int listen_mask; > > ? ? unsigned int dp_serial; > > ?}; > > > > @@ -178,7 +177,6 @@ create_dpif_netdev(struct dp_netdev *dp) > > ? ? dpif = xmalloc(sizeof *dpif); > > ? ? dpif_init(&dpif->dpif, dp->class, dp->name, netflow_id >> 8, > > netflow_id); > > ? ? dpif->dp = dp; > > - ? ?dpif->listen_mask = 0; > > ? ? dpif->dp_serial = dp->serial; > > > > ? ? return &dpif->dpif; > > @@ -893,18 +891,8 @@ dpif_netdev_execute(struct dpif *dpif, > > ?} > > > > ?static int > > -dpif_netdev_recv_get_mask(const struct dpif *dpif, int *listen_mask) > > +dpif_netdev_recv_set(struct dpif *dpif OVS_UNUSED, bool enable OVS_UNUSED) > > ?{ > > - ? ?struct dpif_netdev *dpif_netdev = dpif_netdev_cast(dpif); > > - ? ?*listen_mask = dpif_netdev->listen_mask; > > - ? ?return 0; > > -} > > - > > -static int > > -dpif_netdev_recv_set_mask(struct dpif *dpif, int listen_mask) > > -{ > > - ? ?struct dpif_netdev *dpif_netdev = dpif_netdev_cast(dpif); > > - ? ?dpif_netdev->listen_mask = listen_mask; > > ? ? return 0; > > ?} > > > > @@ -919,14 +907,12 @@ dpif_netdev_queue_to_priority(const struct dpif *dpif > > OVS_UNUSED, > > ?static struct dp_netdev_queue * > > ?find_nonempty_queue(struct dpif *dpif) > > ?{ > > - ? ?struct dpif_netdev *dpif_netdev = dpif_netdev_cast(dpif); > > ? ? struct dp_netdev *dp = get_dp_netdev(dpif); > > - ? ?int mask = dpif_netdev->listen_mask; > > ? ? int i; > > > > ? ? for (i = 0; i < N_QUEUES; i++) { > > ? ? ? ? struct dp_netdev_queue *q = &dp->queues[i]; > > - ? ? ? ?if (q->head != q->tail && mask & (1u << i)) { > > + ? ? ? ?if (q->head != q->tail) { > > ? ? ? ? ? ? return q; > > ? ? ? ? } > > ? ? } > > @@ -1354,8 +1340,7 @@ const struct dpif_class dpif_netdev_class = { > > ? ? dpif_netdev_flow_dump_done, > > ? ? dpif_netdev_execute, > > ? ? NULL, ? ? ? ? ? ? ? ? ? ? ? /* operate */ > > - ? ?dpif_netdev_recv_get_mask, > > - ? ?dpif_netdev_recv_set_mask, > > + ? ?dpif_netdev_recv_set, > > ? ? dpif_netdev_queue_to_priority, > > ? ? dpif_netdev_recv, > > ? ? dpif_netdev_recv_wait, > > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > > index 429cc9d..916b461 100644 > > --- a/lib/dpif-provider.h > > +++ b/lib/dpif-provider.h > > @@ -301,21 +301,11 @@ struct dpif_class { > > ? ? ?* 'dpif' can perform operations in batch faster than individually. */ > > ? ? void (*operate)(struct dpif *dpif, union dpif_op **ops, size_t n_ops); > > > > - ? ?/* Retrieves 'dpif''s "listen mask" into '*listen_mask'. ?A 1-bit of > > value > > - ? ? * 2**X set in '*listen_mask' indicates that 'dpif' will receive > > messages > > - ? ? * of the type (from "enum dpif_upcall_type") with value X when its > > 'recv' > > - ? ? * function is called. */ > > - ? ?int (*recv_get_mask)(const struct dpif *dpif, int *listen_mask); > > - > > - ? ?/* Sets 'dpif''s "listen mask" to 'listen_mask'. ?A 1-bit of value > > 2**X set > > - ? ? * in '*listen_mask' requests that 'dpif' will receive messages of the > > type > > - ? ? * (from "enum dpif_upcall_type") with value X when its 'recv' > > function is > > - ? ? * called. > > - ? ? * > > - ? ? * Turning DPIF_UC_ACTION off and then back on is allowed to change > > Netlink > > + ? ?/* Enables or disables receiving packets with dpif_recv() for 'dpif'. > > + ? ? * Turning packet receive off and then back on is allowed to change > > Netlink > > ? ? ?* PID assignments (see ->port_get_pid()). ?The client is responsible > > for > > ? ? ?* updating flows as necessary if it does this. */ > > - ? ?int (*recv_set_mask)(struct dpif *dpif, int listen_mask); > > + ? ?int (*recv_set)(struct dpif *dpif, bool enable); > > > > ? ? /* Translates OpenFlow queue ID 'queue_id' (in host byte order) into a > > ? ? ?* priority value used for setting packet priority. */ > > @@ -323,8 +313,8 @@ struct dpif_class { > > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?uint32_t *priority); > > > > ? ? /* Polls for an upcall from 'dpif'. ?If successful, stores the upcall > > into > > - ? ? * '*upcall'. ?Only upcalls of the types selected with the > > set_listen_mask > > - ? ? * member function should be received. > > + ? ? * '*upcall'. ?Should only be called if 'recv_set' has been used to > > enable > > + ? ? * receiving packets from 'dpif'. > > ? ? ?* > > ? ? ?* The caller takes ownership of the data that 'upcall' points to. > > ? ? ?* 'upcall->key' and 'upcall->actions' (if nonnull) point into data > > owned > > diff --git a/lib/dpif.c b/lib/dpif.c > > index 9cbf877..ca875b6 100644 > > --- a/lib/dpif.c > > +++ b/lib/dpif.c > > @@ -1032,54 +1032,24 @@ dpif_upcall_type_to_string(enum dpif_upcall_type > > type) > > ? ? } > > ?} > > > > -static bool OVS_UNUSED > > -is_valid_listen_mask(int listen_mask) > > -{ > > - ? ?return !(listen_mask & ~((1u << DPIF_UC_MISS) | > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? (1u << DPIF_UC_ACTION))); > > -} > > - > > -/* Retrieves 'dpif''s "listen mask" into '*listen_mask'. ?A 1-bit of value > > 2**X > > - * set in '*listen_mask' indicates that dpif_recv() will receive messages > > of > > - * the type (from "enum dpif_upcall_type") with value X. ?Returns 0 if > > - * successful, otherwise a positive errno value. */ > > -int > > -dpif_recv_get_mask(const struct dpif *dpif, int *listen_mask) > > -{ > > - ? ?int error = dpif->dpif_class->recv_get_mask(dpif, listen_mask); > > - ? ?if (error) { > > - ? ? ? ?*listen_mask = 0; > > - ? ?} > > - ? ?assert(is_valid_listen_mask(*listen_mask)); > > - ? ?log_operation(dpif, "recv_get_mask", error); > > - ? ?return error; > > -} > > - > > -/* Sets 'dpif''s "listen mask" to 'listen_mask'. ?A 1-bit of value 2**X > > set in > > - * '*listen_mask' requests that dpif_recv() will receive messages of the > > type > > - * (from "enum dpif_upcall_type") with value X. ?Returns 0 if successful, > > - * otherwise a positive errno value. > > +/* Enables or disables receiving packets with dpif_recv() on 'dpif'. > > ?Returns 0 > > + * if successful, otherwise a positive errno value. > > ?* > > - * Turning DPIF_UC_ACTION off and then back on may change the Netlink PID > > + * Turning packet receive off and then back on may change the Netlink PID > > ?* assignments returned by dpif_port_get_pid(). ?If the client does this, it > > ?* must update all of the flows that have OVS_ACTION_ATTR_USERSPACE actions > > ?* using the new PID assignment. */ > > ?int > > -dpif_recv_set_mask(struct dpif *dpif, int listen_mask) > > +dpif_recv_set(struct dpif *dpif, bool enable) > > ?{ > > - ? ?int error; > > - > > - ? ?assert(is_valid_listen_mask(listen_mask)); > > - > > - ? ?error = dpif->dpif_class->recv_set_mask(dpif, listen_mask); > > - ? ?log_operation(dpif, "recv_set_mask", error); > > + ? ?int error = dpif->dpif_class->recv_set(dpif, enable); > > + ? ?log_operation(dpif, "recv_set", error); > > ? ? return error; > > ?} > > > > ?/* Polls for an upcall from 'dpif'. ?If successful, stores the upcall into > > - * '*upcall'. ?Only upcalls of the types selected with dpif_recv_set_mask() > > - * member function will ordinarily be received (but if a message type is > > - * enabled and then later disabled, some stragglers might pop up). > > + * '*upcall'. ?Should only be called if dpif_recv_set() has been used to > > enable > > + * receiving packets on 'dpif'. > > ?* > > ?* The caller takes ownership of the data that 'upcall' points to. > > ?* 'upcall->key' and 'upcall->actions' (if nonnull) point into data owned by > > diff --git a/lib/dpif.h b/lib/dpif.h > > index 0ff2389..f2a9c48 100644 > > --- a/lib/dpif.h > > +++ b/lib/dpif.h > > @@ -245,8 +245,7 @@ struct dpif_upcall { > > ? ? uint64_t userdata; ? ? ? ? ?/* Argument to OVS_ACTION_ATTR_USERSPACE. */ > > ?}; > > > > -int dpif_recv_get_mask(const struct dpif *, int *listen_mask); > > -int dpif_recv_set_mask(struct dpif *, int listen_mask); > > +int dpif_recv_set(struct dpif *, bool enable); > > ?int dpif_recv(struct dpif *, struct dpif_upcall *); > > ?void dpif_recv_purge(struct dpif *); > > ?void dpif_recv_wait(struct dpif *); > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 14b5447..722ca9a 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -625,9 +625,7 @@ construct(struct ofproto *ofproto_, int *n_tablesp) > > ? ? dpif_flow_flush(ofproto->dpif); > > ? ? dpif_recv_purge(ofproto->dpif); > > > > - ? ?error = dpif_recv_set_mask(ofproto->dpif, > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((1u << DPIF_UC_MISS) | > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(1u << DPIF_UC_ACTION))); > > + ? ?error = dpif_recv_set(ofproto->dpif, true); > > ? ? if (error) { > > ? ? ? ? VLOG_ERR("failed to listen on datapath %s: %s", name, > > strerror(error)); > > ? ? ? ? dpif_close(ofproto->dpif); > > -- > > 1.7.2.5 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
