From a12b3eadc64573c132e3630c5602be3c9b7f7156 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 12 Jan 2012 17:09:22 -0800 Subject: [PATCH] dpif: Simplify the "listen mask" concept. 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 Signed-off-by: Ben Pfaff --- 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 317274e39..741a6f0e0 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; @@ -1040,7 +1020,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; @@ -1119,7 +1098,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; } @@ -1164,9 +1143,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; } @@ -1185,7 +1162,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; } @@ -1198,7 +1175,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; } @@ -1236,8 +1213,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 6a411fe95..e2fb816bd 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; @@ -927,18 +925,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; } @@ -953,14 +941,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; } } @@ -1300,8 +1286,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 429cc9d73..916b4612a 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 ecafac1ac..4edf54e83 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1033,54 +1033,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 0ff2389dd..f2a9c4899 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 15532e02c..6ecf71b8c 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -638,9 +638,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); -- 2.45.2