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
