[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