[ovs-dev] [PATCH] dpif: Simplify the "listen mask" concept.
Ethan Jackson
ethan at nicira.com
Fri Jan 13 00:24:31 UTC 2012
Looks good.
Ethan
On Wed, Nov 30, 2011 at 15:08, Ben Pfaff <blp at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list