[ovs-dev] [PATCH] dpif: Simplify the "listen mask" concept.

Ben Pfaff blp at nicira.com
Fri Jan 13 01:09:37 UTC 2012


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 <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