[ovs-dev] [RFC 3/3] dpif-netlink: Introduce per-cpu upcall dispatch

Mark Gray mark.d.gray at redhat.com
Wed Jun 30 09:44:20 UTC 2021


On 08/06/2021 21:07, Flavio Leitner wrote:
> 
> Hi Mark,
> 
> This looks good to me.
> 
> Since the new scheme doesn't allow users to change the number
> of handlers, we must update ovs-vswitchd.conf.db(5) as well.

I updated this documentation

> 
> Some comments below.
> 
> On Fri, Apr 30, 2021 at 11:31:29AM -0400, Mark Gray wrote:
>> The Open vSwitch kernel module uses the upcall mechanism to send
>> packets from kernel space to user space when it misses in the kernel
>> space flow table. The upcall sends packets via a Netlink socket.
>> Currently, a Netlink socket is created for every vport. In this way,
>> there is a 1:1 mapping between a vport and a Netlink socket.
>> When a packet is received by a vport, if it needs to be sent to
>> user space, it is sent via the corresponding Netlink socket.
>>
>> This mechanism, with various iterations of the corresponding user
>> space code, has seen some limitations and issues:
>>
>> * On systems with a large number of vports, there is correspondingly
>> a large number of Netlink sockets which can limit scaling.
>> (https://bugzilla.redhat.com/show_bug.cgi?id=1526306)
>> * Packet reordering on upcalls.
>> (https://bugzilla.redhat.com/show_bug.cgi?id=1844576)
>> * A thundering herd issue.
>> (https://bugzilla.redhat.com/show_bug.cgi?id=1834444)
>>
>> This patch introduces an alternative, feature-negotiated, upcall
>> mode using a per-cpu dispatch rather than a per-vport dispatch.
>>
>> In this mode, the Netlink socket to be used for the upcall is
>> selected based on the CPU of the thread that is executing the upcall.
>> In this way, it resolves the issues above as:
>>
>> a) The number of Netlink sockets scales with the number of CPUs
>> rather than the number of vports.
>> b) Ordering per-flow is maintained as packets are distributed to
>> CPUs based on mechanisms such as RSS and flows are distributed
>> to a single user space thread.
>> c) Packets from a flow can only wake up one user space thread.
>>
>> Reported-at: https://bugzilla.redhat.com/1844576
>> Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
>> ---
>>  .../linux/compat/include/linux/openvswitch.h  |   7 +
>>  lib/dpif-netdev.c                             |   1 +
>>  lib/dpif-netlink.c                            | 405 +++++++++++++++---
>>  lib/dpif-provider.h                           |  10 +
>>  lib/dpif.c                                    |  17 +
>>  lib/dpif.h                                    |   1 +
>>  ofproto/ofproto-dpif-upcall.c                 |  51 ++-
>>  ofproto/ofproto.c                             |  12 -
>>  8 files changed, 430 insertions(+), 74 deletions(-)
>>
>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
>> index 875de20250ce..f29265df055e 100644
>> --- a/datapath/linux/compat/include/linux/openvswitch.h
>> +++ b/datapath/linux/compat/include/linux/openvswitch.h
>> @@ -89,6 +89,8 @@ enum ovs_datapath_cmd {
>>   * set on the datapath port (for OVS_ACTION_ATTR_MISS).  Only valid on
>>   * %OVS_DP_CMD_NEW requests. A value of zero indicates that upcalls should
>>   * not be sent.
>> + * OVS_DP_ATTR_PER_CPU_PIDS: Per-cpu array of PIDs for upcalls when
>> + * OVS_DP_F_DISPATCH_UPCALL_PER_CPU feature is set.
>>   * @OVS_DP_ATTR_STATS: Statistics about packets that have passed through the
>>   * datapath.  Always present in notifications.
>>   * @OVS_DP_ATTR_MEGAFLOW_STATS: Statistics about mega flow masks usage for the
>> @@ -105,6 +107,8 @@ enum ovs_datapath_attr {
>>  	OVS_DP_ATTR_MEGAFLOW_STATS,	/* struct ovs_dp_megaflow_stats */
>>  	OVS_DP_ATTR_USER_FEATURES,	/* OVS_DP_F_*  */
>>  	OVS_DP_ATTR_PAD,
>> +	OVS_DP_ATTR_PAD2,
>> +	OVS_DP_ATTR_PER_CPU_PIDS,	/* Netlink PIDS to receive upcalls */
>>  	__OVS_DP_ATTR_MAX
>>  };
>>  
>> @@ -146,6 +150,9 @@ struct ovs_vport_stats {
>>  /* Allow tc offload recirc sharing */
>>  #define OVS_DP_F_TC_RECIRC_SHARING  (1 << 2)
>>  
>> +/* Allow per-cpu dispatch of upcalls */
>> +#define OVS_DP_F_DISPATCH_UPCALL_PER_CPU (1 << 3)
>> +
>>  /* Fixed logical ports. */
>>  #define OVSP_LOCAL      ((__u32)0)
>>  
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 251788b04965..24e6911dd4ff 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -8488,6 +8488,7 @@ const struct dpif_class dpif_netdev_class = {
>>      dpif_netdev_operate,
>>      NULL,                       /* recv_set */
>>      NULL,                       /* handlers_set */
>> +    NULL,                       /* handlers_get */
> 
> That is number_handlers_required.
> 

oops. Thanks for catching this

>>      dpif_netdev_set_config,
>>      dpif_netdev_queue_to_priority,
>>      NULL,                       /* recv */
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index 2ded5fdd01b3..349897e70632 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -80,6 +80,9 @@ enum { MAX_PORTS = USHRT_MAX };
>>  #define FLOW_DUMP_MAX_BATCH 50
>>  #define OPERATE_MAX_OPS 50
>>  
>> +#define DISPATCH_MODE_PER_CPU(dpif) ((dpif)->user_features & \
>> +                                     OVS_DP_F_DISPATCH_UPCALL_PER_CPU)
>> +
> 
> Perhaps a function like this:
> static inline bool
> dpif_netlink_upcall_per_cpu(struct ... dpif) {
>     return !!((dpif)->user_features & OVS_DP_F_DISPATCH_UPCALL_PER_CPU);
> }
> 
>>  #ifndef EPOLLEXCLUSIVE
>>  #define EPOLLEXCLUSIVE (1u << 28)
>>  #endif
>> @@ -98,6 +101,8 @@ struct dpif_netlink_dp {
>>      const struct ovs_dp_stats *stats;  /* OVS_DP_ATTR_STATS. */
>>      const struct ovs_dp_megaflow_stats *megaflow_stats;
>>                                         /* OVS_DP_ATTR_MEGAFLOW_STATS.*/
>> +    const uint32_t *upcall_pids;       /* OVS_DP_ATTR_PER_CPU_PIDS */
>> +    uint32_t n_upcall_pids;
>>  };
>>  
>>  static void dpif_netlink_dp_init(struct dpif_netlink_dp *);
>> @@ -178,11 +183,16 @@ struct dpif_windows_vport_sock {
>>  #endif
>>  
>>  struct dpif_handler {
>> +    /* per-vport dispatch mode */
>>      struct epoll_event *epoll_events;
>>      int epoll_fd;                 /* epoll fd that includes channel socks. */
>>      int n_events;                 /* Num events returned by epoll_wait(). */
>>      int event_offset;             /* Offset into 'epoll_events'. */
>>  
>> +    /* per-cpu dispatch mode */
>> +    struct nl_sock *sock;         /* Each handler thread holds one netlink
>> +                                     socket. */
>> +
>>  #ifdef _WIN32
>>      /* Pool of sockets. */
>>      struct dpif_windows_vport_sock *vport_sock_pool;
>> @@ -201,6 +211,8 @@ struct dpif_netlink {
>>      struct fat_rwlock upcall_lock;
>>      struct dpif_handler *handlers;
>>      uint32_t n_handlers;           /* Num of upcall handlers. */
>> +
>> +    /* Per-vport dispatch mode */
>>      struct dpif_channel *channels; /* Array of channels for each port. */
>>      int uc_array_size;             /* Size of 'handler->channels' and */
>>                                     /* 'handler->epoll_events'. */
>> @@ -241,8 +253,12 @@ static int open_dpif(const struct dpif_netlink_dp *, struct dpif **);
>>  static uint32_t dpif_netlink_port_get_pid(const struct dpif *,
>>                                            odp_port_t port_no);
>>  static void dpif_netlink_handler_uninit(struct dpif_handler *handler);
>> -static int dpif_netlink_refresh_channels(struct dpif_netlink *,
>> -                                         uint32_t n_handlers);
>> +static int dpif_netlink_refresh_handlers_vport_dispatch(struct dpif_netlink *,
>> +                                                        uint32_t n_handlers);
>> +static void destroy_all_channels(struct dpif_netlink *);
>> +static int dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *);
>> +static void destroy_all_handlers(struct dpif_netlink *);
>> +
>>  static void dpif_netlink_vport_to_ofpbuf(const struct dpif_netlink_vport *,
>>                                           struct ofpbuf *);
>>  static int dpif_netlink_vport_from_ofpbuf(struct dpif_netlink_vport *,
>> @@ -357,11 +373,34 @@ dpif_netlink_open(const struct dpif_class *class OVS_UNUSED, const char *name,
>>          dp_request.cmd = OVS_DP_CMD_SET;
>>      }
>>  
>> +    /* The Open vSwitch kernel module has two modes for dispatching upcalls:
>> +     * per-vport and per-cpu.
>> +     *
>> +     * When dispatching upcalls per-vport, the kernel will
>> +     * send the upcall via a Netlink socket that has been selected based on the
>> +     * vport that received the packet that is causing the upcall.
>> +     *
>> +     * When dispatching upcall per-cpu, the kernel will send the upcall via
>> +     * a Netlink socket that has been selected based on the cpu that received
>> +     * the packet that is causing the upcall.
>> +     *
>> +     * First we test to see if the kernel module supports per-cpu dispatching
>> +     * (the preferred method). If it does not support per-cpu dispatching, we
>> +     * fall back to the per-vport dispatch mode.
>> +     */
>>      dp_request.user_features |= OVS_DP_F_UNALIGNED;
>> -    dp_request.user_features |= OVS_DP_F_VPORT_PIDS;
>> +    dp_request.user_features |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU;
>>      error = dpif_netlink_dp_transact(&dp_request, &dp, &buf);
>>      if (error) {
>> -        return error;
>> +        dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU;
>> +        dp_request.user_features |= OVS_DP_F_VPORT_PIDS;
>> +        error = dpif_netlink_dp_transact(&dp_request, &dp, &buf);
>> +        if (error) {
>> +            return error;
>> +        }
>> +        VLOG_INFO("Dispatch mode(per-vport)");
>> +    } else {
>> +        VLOG_INFO("Dispatch mode:(per-cpu)");
> 
> 
> It would be nice if we could expose the mode using a command.

I added `ovs-appctl dpif-netlink/dispatch-mode`

# ovs-appctl dpif-netlink/dispatch-mode
ovs-system: per-cpu dispatch mode

> 
> 
>>      }
>>  
>>      error = open_dpif(&dp, dpifp);
>> @@ -609,6 +648,24 @@ destroy_all_channels(struct dpif_netlink *dpif)
>>      dpif->uc_array_size = 0;
>>  }
>>  
>> +static void
>> +destroy_all_handlers(struct dpif_netlink *dpif)
>> +    OVS_REQ_WRLOCK(dpif->upcall_lock)
>> +{
>> +    int i = 0;
>> +
>> +    if (!dpif->handlers) {
>> +        return;
>> +    }
>> +    for (i = 0; i < dpif->n_handlers; i++) {
>> +        struct dpif_handler *handler = &dpif->handlers[i];
>> +        close_nl_sock(handler->sock);
>> +    }
>> +    free(dpif->handlers);
>> +    dpif->handlers = NULL;
>> +    dpif->n_handlers = 0;
>> +}
>> +
>>  static void
>>  dpif_netlink_close(struct dpif *dpif_)
>>  {
>> @@ -617,7 +674,11 @@ dpif_netlink_close(struct dpif *dpif_)
>>      nl_sock_destroy(dpif->port_notifier);
>>  
>>      fat_rwlock_wrlock(&dpif->upcall_lock);
>> -    destroy_all_channels(dpif);
>> +    if (DISPATCH_MODE_PER_CPU(dpif)) {
>> +        destroy_all_handlers(dpif);
>> +    } else {
>> +        destroy_all_channels(dpif);
>> +    }
>>      fat_rwlock_unlock(&dpif->upcall_lock);
>>  
>>      fat_rwlock_destroy(&dpif->upcall_lock);
>> @@ -641,11 +702,14 @@ dpif_netlink_run(struct dpif *dpif_)
>>  {
>>      struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>>  
>> -    if (dpif->refresh_channels) {
>> -        dpif->refresh_channels = false;
>> -        fat_rwlock_wrlock(&dpif->upcall_lock);
>> -        dpif_netlink_refresh_channels(dpif, dpif->n_handlers);
>> -        fat_rwlock_unlock(&dpif->upcall_lock);
>> +    if (!DISPATCH_MODE_PER_CPU(dpif)) {
>> +        if (dpif->refresh_channels) {
>> +            dpif->refresh_channels = false;
>> +            fat_rwlock_wrlock(&dpif->upcall_lock);
>> +            dpif_netlink_refresh_handlers_vport_dispatch(dpif,
>> +                                                         dpif->n_handlers);
>> +            fat_rwlock_unlock(&dpif->upcall_lock);
>> +        }
>>      }
>>      return false;
>>  }
>> @@ -681,6 +745,41 @@ dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats)
>>      return error;
>>  }
>>  
>> +static int
>> +dpif_netlink_set_handler_pids(struct dpif *dpif_, const uint32_t *upcall_pids,
>> +                              uint32_t n_upcall_pids)
>> +{
>> +    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>> +    struct dpif_netlink_dp request, reply;
>> +    struct ofpbuf *bufp;
>> +    int error;
>> +    int n_cores;
>> +
>> +    n_cores = count_cpu_cores();
>> +    ovs_assert(n_cores == n_upcall_pids);
>> +    VLOG_DBG("Dispatch mode(per-cpu): Number of CPUs is %d", n_cores);
>> +
>> +    dpif_netlink_dp_init(&request);
>> +    request.cmd = OVS_DP_CMD_SET;
>> +    request.name = dpif_->base_name;
>> +    request.dp_ifindex = dpif->dp_ifindex;
>> +    request.user_features = dpif->user_features |
>> +                            OVS_DP_F_DISPATCH_UPCALL_PER_CPU;
>> +
>> +    request.upcall_pids = upcall_pids;
>> +    request.n_upcall_pids = n_cores;
>> +
>> +    error = dpif_netlink_dp_transact(&request, &reply, &bufp);
>> +    if (!error) {
>> +        dpif->user_features = reply.user_features;
>> +        ofpbuf_delete(bufp);
>> +         if (!(dpif->user_features & OVS_DP_F_DISPATCH_UPCALL_PER_CPU)) {
>> +            return -EOPNOTSUPP;
>> +        }
>> +    }
>> +    return error;
>> +}
>> +
>>  static int
>>  dpif_netlink_set_features(struct dpif *dpif_, uint32_t new_features)
>>  {
>> @@ -741,7 +840,7 @@ get_vport_type(const struct dpif_netlink_vport *vport)
>>          return "erspan";
>>  
>>      case OVS_VPORT_TYPE_IP6ERSPAN:
>> -        return "ip6erspan"; 
>> +        return "ip6erspan";
>>  
>>      case OVS_VPORT_TYPE_IP6GRE:
>>          return "ip6gre";
>> @@ -807,10 +906,16 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, const char *name,
>>      uint32_t upcall_pids = 0;
>>      int error = 0;
>>  
>> -    if (dpif->handlers) {
>> -        error = create_nl_sock(dpif, &sock);
>> -        if (error) {
>> -            return error;
>> +    /* per-cpu dispatch mode does not require a socket per vport */
>> +    if (!DISPATCH_MODE_PER_CPU(dpif)) {
>> +        if (dpif->handlers) {
>> +            error = create_nl_sock(dpif, &sock);
>> +            if (error) {
>> +                return error;
>> +            }
>> +        }
>> +        if (sock) {
>> +            upcall_pids = nl_sock_pid(sock);
>>          }
>>      }
>>  
>> @@ -821,9 +926,6 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, const char *name,
>>      request.name = name;
>>  
>>      request.port_no = *port_nop;
>> -    if (sock) {
>> -        upcall_pids = nl_sock_pid(sock);
>> -    }
>>      request.n_upcall_pids = 1;
>>      request.upcall_pids = &upcall_pids;
>>  
>> @@ -845,19 +947,21 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, const char *name,
>>          goto exit;
>>      }
>>  
>> -    error = vport_add_channel(dpif, *port_nop, sock);
>> -    if (error) {
>> -        VLOG_INFO("%s: could not add channel for port %s",
>> -                    dpif_name(&dpif->dpif), name);
>> -
>> -        /* Delete the port. */
>> -        dpif_netlink_vport_init(&request);
>> -        request.cmd = OVS_VPORT_CMD_DEL;
>> -        request.dp_ifindex = dpif->dp_ifindex;
>> -        request.port_no = *port_nop;
>> -        dpif_netlink_vport_transact(&request, NULL, NULL);
>> -        close_nl_sock(sock);
>> -        goto exit;
>> +    if (!DISPATCH_MODE_PER_CPU(dpif)) {
>> +        error = vport_add_channel(dpif, *port_nop, sock);
>> +        if (error) {
>> +            VLOG_INFO("%s: could not add channel for port %s",
>> +                        dpif_name(&dpif->dpif), name);
>> +
>> +            /* Delete the port. */
>> +            dpif_netlink_vport_init(&request);
>> +            request.cmd = OVS_VPORT_CMD_DEL;
>> +            request.dp_ifindex = dpif->dp_ifindex;
>> +            request.port_no = *port_nop;
>> +            dpif_netlink_vport_transact(&request, NULL, NULL);
>> +            close_nl_sock(sock);
>> +            goto exit;
>> +        }
>>      }
>>  
>>  exit:
>> @@ -1115,6 +1219,11 @@ dpif_netlink_port_get_pid(const struct dpif *dpif_, odp_port_t port_no)
>>      const struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>>      uint32_t ret;
>>  
>> +    /* In per-cpu dispatch mode, vports do not have an associated PID */
>> +    if (DISPATCH_MODE_PER_CPU(dpif)) {
>> +        return 0;
>> +    }
> 
> I mentioned this in the kernel patch's review that this is used with
> userspace() action and 0 means it is 'unset'.

I resolved this and tested using `ovs-ofctl monitor br0 65534 -P
nxt_packet_in`
> 
> 
>> +
>>      fat_rwlock_rdlock(&dpif->upcall_lock);
>>      ret = dpif_netlink_port_get_pid__(dpif, port_no);
>>      fat_rwlock_unlock(&dpif->upcall_lock);
>> @@ -2326,12 +2435,51 @@ dpif_netlink_handler_uninit(struct dpif_handler *handler)
>>  }
>>  #endif
>>  
>> +static int
>> +dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif)
>> +    OVS_REQ_WRLOCK(dpif->upcall_lock)
>> +{
>> +    int handler_id;
>> +    int error = 0;
>> +    uint32_t n_handlers;
>> +    uint32_t *upcall_pids;
>> +
>> +    n_handlers = count_cpu_cores();
>> +    if (dpif->n_handlers != n_handlers) {
>> +        VLOG_DBG("Dispatch mode(per-cpu): initializing %d handlers",
>> +                   n_handlers);
>> +        destroy_all_handlers(dpif);
>> +        upcall_pids = xzalloc(n_handlers * sizeof *upcall_pids);
>> +        dpif->handlers = xzalloc(n_handlers * sizeof *dpif->handlers);
>> +        for (handler_id = 0; handler_id < n_handlers; handler_id++) {
>> +            struct dpif_handler *handler = &dpif->handlers[handler_id];
>> +            error = create_nl_sock(dpif, &handler->sock);
>> +            if (error) {
>> +                VLOG_ERR("Dispatch mode(per-cpu): Cannot create socket for"
>> +                         "handler %d", handler_id);
>> +                continue;
>> +            }
>> +            upcall_pids[handler_id] = nl_sock_pid(handler->sock);
>> +            VLOG_DBG("Dispatch mode(per-cpu): "
>> +                      "handler %d has Netlink PID of %u",
>> +                      handler_id, upcall_pids[handler_id]);
>> +        }
>> +
>> +        dpif->n_handlers = n_handlers;
>> +        error = dpif_netlink_set_handler_pids(&dpif->dpif, upcall_pids,
>> +                                              n_handlers);
>> +        free(upcall_pids);
>> +    }
>> +    return error;
>> +}
>> +
>>  /* Synchronizes 'channels' in 'dpif->handlers'  with the set of vports
>>   * currently in 'dpif' in the kernel, by adding a new set of channels for
>>   * any kernel vport that lacks one and deleting any channels that have no
>>   * backing kernel vports. */
>>  static int
>> -dpif_netlink_refresh_channels(struct dpif_netlink *dpif, uint32_t n_handlers)
>> +dpif_netlink_refresh_handlers_vport_dispatch(struct dpif_netlink *dpif,
>> +                                             uint32_t n_handlers)
>>      OVS_REQ_WRLOCK(dpif->upcall_lock)
>>  {
>>      unsigned long int *keep_channels;
>> @@ -2458,7 +2606,7 @@ dpif_netlink_refresh_channels(struct dpif_netlink *dpif, uint32_t n_handlers)
>>  }
>>  
>>  static int
>> -dpif_netlink_recv_set__(struct dpif_netlink *dpif, bool enable)
>> +dpif_netlink_recv_set_vport_dispatch(struct dpif_netlink *dpif, bool enable)
>>      OVS_REQ_WRLOCK(dpif->upcall_lock)
>>  {
>>      if ((dpif->handlers != NULL) == enable) {
>> @@ -2467,7 +2615,21 @@ dpif_netlink_recv_set__(struct dpif_netlink *dpif, bool enable)
>>          destroy_all_channels(dpif);
>>          return 0;
>>      } else {
>> -        return dpif_netlink_refresh_channels(dpif, 1);
>> +        return dpif_netlink_refresh_handlers_vport_dispatch(dpif, 1);
>> +    }
>> +}
>> +
>> +static int
>> +dpif_netlink_recv_set_cpu_dispatch(struct dpif_netlink *dpif, bool enable)
>> +    OVS_REQ_WRLOCK(dpif->upcall_lock)
>> +{
>> +    if ((dpif->handlers != NULL) == enable) {
>> +        return 0;
>> +    } else if (!enable) {
>> +        destroy_all_handlers(dpif);
>> +        return 0;
>> +    } else {
>> +        return dpif_netlink_refresh_handlers_cpu_dispatch(dpif);
>>      }
>>  }
>>  
>> @@ -2478,7 +2640,11 @@ dpif_netlink_recv_set(struct dpif *dpif_, bool enable)
>>      int error;
>>  
>>      fat_rwlock_wrlock(&dpif->upcall_lock);
>> -    error = dpif_netlink_recv_set__(dpif, enable);
>> +    if (DISPATCH_MODE_PER_CPU(dpif)) {
>> +        error = dpif_netlink_recv_set_cpu_dispatch(dpif, enable);
>> +    } else {
>> +        error = dpif_netlink_recv_set_vport_dispatch(dpif, enable);
>> +    }
>>      fat_rwlock_unlock(&dpif->upcall_lock);
>>  
>>      return error;
>> @@ -2500,13 +2666,31 @@ dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t n_handlers)
>>  
>>      fat_rwlock_wrlock(&dpif->upcall_lock);
>>      if (dpif->handlers) {
>> -        error = dpif_netlink_refresh_channels(dpif, n_handlers);
>> +        if (DISPATCH_MODE_PER_CPU(dpif)) {
>> +            error = dpif_netlink_refresh_handlers_cpu_dispatch(dpif);
>> +        } else {
>> +            error = dpif_netlink_refresh_handlers_vport_dispatch(dpif,
>> +                                                                 n_handlers);
>> +        }
>>      }
>>      fat_rwlock_unlock(&dpif->upcall_lock);
>>  
>>      return error;
>>  }
>>  
>> +static bool
>> +dpif_netlink_number_handlers_required(struct dpif *dpif_, uint32_t *n_handlers)
>> +{
>> +    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>> +
>> +    if (DISPATCH_MODE_PER_CPU(dpif)) {
>> +        *n_handlers = count_cpu_cores();
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>  static int
>>  dpif_netlink_queue_to_priority(const struct dpif *dpif OVS_UNUSED,
>>                               uint32_t queue_id, uint32_t *priority)
>> @@ -2669,8 +2853,59 @@ dpif_netlink_recv_windows(struct dpif_netlink *dpif, uint32_t handler_id,
>>  }
>>  #else
>>  static int
>> -dpif_netlink_recv__(struct dpif_netlink *dpif, uint32_t handler_id,
>> -                    struct dpif_upcall *upcall, struct ofpbuf *buf)
>> +dpif_netlink_recv_cpu_dispatch(struct dpif_netlink *dpif, uint32_t handler_id,
>> +                               struct dpif_upcall *upcall, struct ofpbuf *buf)
>> +    OVS_REQ_RDLOCK(dpif->upcall_lock)
>> +{
>> +    struct dpif_handler *handler;
>> +    int read_tries = 0;
>> +
>> +    if (!dpif->handlers || handler_id >= dpif->n_handlers) {
>> +        return EAGAIN;
>> +    }
>> +
>> +    handler = &dpif->handlers[handler_id];
>> +
>> +    for (;;) {
>> +        int dp_ifindex;
>> +        int error;
>> +
>> +        if (++read_tries > 50) {
>> +            return EAGAIN;
>> +        }
>> +        error = nl_sock_recv(handler->sock, buf, NULL, false);
>> +        if (error == ENOBUFS) {
>> +            /* ENOBUFS typically means that we've received so many
>> +                * packets that the buffer overflowed.  Try again
>> +                * immediately because there's almost certainly a packet
>> +                * waiting for us. */
>> +            report_loss(dpif, NULL, 0, handler_id);
>> +            continue;
>> +        }
>> +
>> +        if (error) {
>> +            if (error == EAGAIN) {
>> +                break;
>> +            }
>> +            return error;
>> +        }
>> +
>> +        error = parse_odp_packet(buf, upcall, &dp_ifindex);
>> +        if (!error && dp_ifindex == dpif->dp_ifindex) {
>> +            return 0;
>> +        } else if (error) {
>> +            return error;
>> +        }
>> +    }
>> +
>> +    return EAGAIN;
>> +}
>> +
>> +static int
>> +dpif_netlink_recv_vport_dispatch(struct dpif_netlink *dpif,
>> +                                 uint32_t handler_id,
>> +                                 struct dpif_upcall *upcall,
>> +                                 struct ofpbuf *buf)
>>      OVS_REQ_RDLOCK(dpif->upcall_lock)
>>  {
>>      struct dpif_handler *handler;
>> @@ -2755,18 +2990,24 @@ dpif_netlink_recv(struct dpif *dpif_, uint32_t handler_id,
>>  #ifdef _WIN32
>>      error = dpif_netlink_recv_windows(dpif, handler_id, upcall, buf);
>>  #else
>> -    error = dpif_netlink_recv__(dpif, handler_id, upcall, buf);
>> +    if (DISPATCH_MODE_PER_CPU(dpif)) {
>> +        error = dpif_netlink_recv_cpu_dispatch(dpif, handler_id, upcall, buf);
>> +    } else {
>> +        error = dpif_netlink_recv_vport_dispatch(dpif,
>> +                                                 handler_id, upcall, buf);
>> +    }
>>  #endif
>>      fat_rwlock_unlock(&dpif->upcall_lock);
>>  
>>      return error;
>>  }
>>  
>> +#ifdef _WIN32
>>  static void
>> -dpif_netlink_recv_wait__(struct dpif_netlink *dpif, uint32_t handler_id)
>> +dpif_netlink_recv_wait_windows(struct dpif_netlink *dpif, uint32_t handler_id)
>>      OVS_REQ_RDLOCK(dpif->upcall_lock)
>>  {
>> -#ifdef _WIN32
>> +
>>      uint32_t i;
>>      struct dpif_windows_vport_sock *sock_pool =
>>          dpif->handlers[handler_id].vport_sock_pool;
>> @@ -2779,13 +3020,31 @@ dpif_netlink_recv_wait__(struct dpif_netlink *dpif, uint32_t handler_id)
>>      for (i = 0; i < VPORT_SOCK_POOL_SIZE; i++) {
>>          nl_sock_wait(sock_pool[i].nl_sock, POLLIN);
>>      }
>> -#else
>> +}
>> +#endif
>> +
>> +static void
>> +dpif_netlink_recv_wait_vport_dispatch(struct dpif_netlink *dpif,
>> +                                      uint32_t handler_id)
>> +    OVS_REQ_RDLOCK(dpif->upcall_lock)
>> +{
>>      if (dpif->handlers && handler_id < dpif->n_handlers) {
>>          struct dpif_handler *handler = &dpif->handlers[handler_id];
>>  
>>          poll_fd_wait(handler->epoll_fd, POLLIN);
>>      }
>> -#endif
>> +}
>> +
>> +static void
>> +dpif_netlink_recv_wait_cpu_dispatch(struct dpif_netlink *dpif,
>> +                                    uint32_t handler_id)
>> +    OVS_REQ_RDLOCK(dpif->upcall_lock)
>> +{
>> +    if (dpif->handlers && handler_id < dpif->n_handlers) {
>> +        struct dpif_handler *handler = &dpif->handlers[handler_id];
>> +
>> +        poll_fd_wait(nl_sock_fd(handler->sock), POLLIN);
>> +    }
>>  }
>>  
>>  static void
>> @@ -2794,12 +3053,20 @@ dpif_netlink_recv_wait(struct dpif *dpif_, uint32_t handler_id)
>>      struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>>  
>>      fat_rwlock_rdlock(&dpif->upcall_lock);
>> -    dpif_netlink_recv_wait__(dpif, handler_id);
>> +#ifdef _WIN32
>> +    dpif_netlink_recv_wait_windows(dpif, handler_id);
>> +#else
>> +    if (DISPATCH_MODE_PER_CPU(dpif)) {
>> +        dpif_netlink_recv_wait_cpu_dispatch(dpif, handler_id);
>> +    } else {
>> +        dpif_netlink_recv_wait_vport_dispatch(dpif, handler_id);
>> +    }
>> +#endif
>>      fat_rwlock_unlock(&dpif->upcall_lock);
>>  }
>>  
>>  static void
>> -dpif_netlink_recv_purge__(struct dpif_netlink *dpif)
>> +dpif_netlink_recv_purge_vport_dispatch(struct dpif_netlink *dpif)
>>      OVS_REQ_WRLOCK(dpif->upcall_lock)
>>  {
>>      if (dpif->handlers) {
>> @@ -2815,13 +3082,31 @@ dpif_netlink_recv_purge__(struct dpif_netlink *dpif)
>>      }
>>  }
>>  
>> +static void
>> +dpif_netlink_recv_purge_cpu_dispatch(struct dpif_netlink *dpif)
>> +    OVS_REQ_WRLOCK(dpif->upcall_lock)
>> +{
>> +    int handler_id;
>> +
>> +    if (dpif->handlers) {
>> +        for (handler_id = 0; handler_id < dpif->n_handlers; handler_id++) {
>> +            struct dpif_handler *handler = &dpif->handlers[handler_id];
>> +            nl_sock_drain(handler->sock);
>> +        }
>> +    }
>> +}
>> +
>>  static void
>>  dpif_netlink_recv_purge(struct dpif *dpif_)
>>  {
>>      struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>>  
>>      fat_rwlock_wrlock(&dpif->upcall_lock);
>> -    dpif_netlink_recv_purge__(dpif);
>> +    if (DISPATCH_MODE_PER_CPU(dpif)) {
>> +        dpif_netlink_recv_purge_cpu_dispatch(dpif);
>> +    } else {
>> +        dpif_netlink_recv_purge_vport_dispatch(dpif);
>> +    }
>>      fat_rwlock_unlock(&dpif->upcall_lock);
>>  }
>>  
>> @@ -3974,6 +4259,7 @@ const struct dpif_class dpif_netlink_class = {
>>      dpif_netlink_operate,
>>      dpif_netlink_recv_set,
>>      dpif_netlink_handlers_set,
>> +    dpif_netlink_number_handlers_required,
>>      NULL,                       /* set_config */
>>      dpif_netlink_queue_to_priority,
>>      dpif_netlink_recv,
>> @@ -4337,6 +4623,11 @@ dpif_netlink_dp_to_ofpbuf(const struct dpif_netlink_dp *dp, struct ofpbuf *buf)
>>          nl_msg_put_u32(buf, OVS_DP_ATTR_USER_FEATURES, dp->user_features);
>>      }
>>  
>> +    if (dp->upcall_pids) {
>> +        nl_msg_put_unspec(buf, OVS_DP_ATTR_PER_CPU_PIDS, dp->upcall_pids,
>> +                          sizeof *dp->upcall_pids * dp->n_upcall_pids);
>> +    }
>> +
>>      /* Skip OVS_DP_ATTR_STATS since we never have a reason to serialize it. */
>>  }
>>  
>> @@ -4642,7 +4933,6 @@ dpif_netlink_flow_get_stats(const struct dpif_netlink_flow *flow,
>>      stats->used = flow->used ? get_32aligned_u64(flow->used) : 0;
>>      stats->tcp_flags = flow->tcp_flags ? *flow->tcp_flags : 0;
>>  }
>> -
>>  /* Logs information about a packet that was recently lost in 'ch' (in
>>   * 'dpif_'). */
>>  static void
>> @@ -4656,13 +4946,18 @@ report_loss(struct dpif_netlink *dpif, struct dpif_channel *ch, uint32_t ch_idx,
>>          return;
>>      }
>>  
>> -    ds_init(&s);
>> -    if (ch->last_poll != LLONG_MIN) {
>> -        ds_put_format(&s, " (last polled %lld ms ago)",
>> -                      time_msec() - ch->last_poll);
>> -    }
>> +    if (DISPATCH_MODE_PER_CPU(dpif)) {
>> +        VLOG_WARN("%s: lost packet on handler %u",
>> +                  dpif_name(&dpif->dpif), handler_id);
>> +    } else {
>> +        ds_init(&s);
>> +        if (ch->last_poll != LLONG_MIN) {
>> +            ds_put_format(&s, " (last polled %lld ms ago)",
>> +                        time_msec() - ch->last_poll);
>> +        }
>>  
>> -    VLOG_WARN("%s: lost packet on port channel %u of handler %u%s",
>> -              dpif_name(&dpif->dpif), ch_idx, handler_id, ds_cstr(&s));
>> -    ds_destroy(&s);
>> +        VLOG_WARN("%s: lost packet on port channel %u of handler %u%s",
>> +                dpif_name(&dpif->dpif), ch_idx, handler_id, ds_cstr(&s));
>> +        ds_destroy(&s);
>> +    }
>>  }
>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>> index b817fceac698..32711f19d1f6 100644
>> --- a/lib/dpif-provider.h
>> +++ b/lib/dpif-provider.h
>> @@ -357,6 +357,16 @@ struct dpif_class {
>>       * */
>>      int (*handlers_set)(struct dpif *dpif, uint32_t n_handlers);
>>  
>> +    /* Queries 'dpif' to see if a certain number of handlers are required by
>> +     * the implementation.
>> +     *
>> +     * If a certain number of handlers are required, returns 'true' and sets
>> +     * 'n_handlers' to that number of handler threads.
>> +     *
>> +     * If not, returns 'false'.
>> +     */
>> +    bool (*number_handlers_required)(struct dpif *dpif, uint32_t *n_handlers);
>> +
> 
> It also needs to update struct dpif_class handlers_set() documentation.
> 
> 

Good point. I updated it.

>>      /* Pass custom configuration options to the datapath.  The implementation
>>       * might postpone applying the changes until run() is called. */
>>      int (*set_config)(struct dpif *dpif, const struct smap *other_config);
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index 26e8bfb7db98..511383514d5b 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -1489,6 +1489,23 @@ dpif_handlers_set(struct dpif *dpif, uint32_t n_handlers)
>>      return error;
>>  }
>>  
>> +/* Checks if a certain number of handlers are required.
>> + *
>> + * If a certain number of handlers are required, returns 'true' and sets
>> + * 'n_handlers' to that number of handler threads.
>> + *
>> + * If not, returns 'false'
>> + */
>> +bool
>> +dpif_number_handlers_required(struct dpif *dpif, uint32_t *n_handlers) {
>> +    bool ret = false;
>> +
>> +    if (dpif->dpif_class->number_handlers_required) {
>> +        ret = dpif->dpif_class->number_handlers_required(dpif, n_handlers);
>> +    }
>> +    return ret;
>> +}
>> +
>>  void
>>  dpif_register_dp_purge_cb(struct dpif *dpif, dp_purge_callback *cb, void *aux)
>>  {
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index f9728e67393b..7c322d20e6c7 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -873,6 +873,7 @@ void dpif_register_upcall_cb(struct dpif *, upcall_callback *, void *aux);
>>  
>>  int dpif_recv_set(struct dpif *, bool enable);
>>  int dpif_handlers_set(struct dpif *, uint32_t n_handlers);
>> +bool dpif_number_handlers_required(struct dpif *, uint32_t *n_handlers);
>>  int dpif_set_config(struct dpif *, const struct smap *cfg);
>>  int dpif_port_set_config(struct dpif *, odp_port_t, const struct smap *cfg);
>>  int dpif_recv(struct dpif *, uint32_t handler_id, struct dpif_upcall *,
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 88406fea1391..45c22d20c183 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -636,24 +636,61 @@ udpif_set_threads(struct udpif *udpif, uint32_t n_handlers_,
>>                    uint32_t n_revalidators_)
>>  {
>>      ovs_assert(udpif);
>> -    ovs_assert(n_handlers_ && n_revalidators_);
>> +    uint32_t n_handlers_requested;
>> +    uint32_t n_revalidators_requested;
>> +    bool forced = false;
>> +
>> +    if (dpif_number_handlers_required(udpif->dpif, &n_handlers_requested)) {
>> +        forced = true;
>> +        if (!n_revalidators_) {
>> +            n_revalidators_requested = n_handlers_requested / 4 + 1;
>> +        } else {
>> +            n_revalidators_requested = n_revalidators_;
>> +        }
>> +    } else {
>> +        int threads = MAX(count_cpu_cores(), 2);
>> +
>> +        n_revalidators_requested = MAX(n_revalidators_, 0);
>> +        n_handlers_requested = MAX(n_handlers_, 0);
>>  
>> -    if (udpif->n_handlers != n_handlers_
>> -        || udpif->n_revalidators != n_revalidators_) {
>> +        if (!n_revalidators_requested) {
>> +            n_revalidators_requested = n_handlers_requested
>> +            ? MAX(threads - (int) n_handlers_requested, 1)
>> +            : threads / 4 + 1;
>> +        }
>> +
>> +        if (!n_handlers_requested) {
>> +            n_handlers_requested = MAX(threads -
>> +                                       (int) n_revalidators_requested, 1);
>> +        }
>> +    }
>> +
>> +    if (udpif->n_handlers != n_handlers_requested
>> +        || udpif->n_revalidators != n_revalidators_requested) {
>> +        if (forced) {
>> +            VLOG_INFO("Overriding n-handler-threads to %u, setting "
>> +                      "n-revalidator-threads to %u", n_handlers_requested,
>> +                      n_revalidators_requested);
>> +            } else {
>> +            VLOG_INFO("Setting n-handler-threads to %u, setting "
>> +                      "n-revalidator-threads to %u", n_handlers_requested,
>> +                      n_revalidators_requested);
>> +        }
>>          udpif_stop_threads(udpif, true);
>>      }
>>  
>>      if (!udpif->handlers && !udpif->revalidators) {
>> +        VLOG_INFO("Starting %u threads", n_handlers_requested +
>> +                                         n_revalidators_requested);
>>          int error;
>> -
>> -        error = dpif_handlers_set(udpif->dpif, n_handlers_);
>> +        error = dpif_handlers_set(udpif->dpif, n_handlers_requested);
>>          if (error) {
>>              VLOG_ERR("failed to configure handlers in dpif %s: %s",
>>                       dpif_name(udpif->dpif), ovs_strerror(error));
>>              return;
>>          }
>> -
>> -        udpif_start_threads(udpif, n_handlers_, n_revalidators_);
>> +        udpif_start_threads(udpif, n_handlers_requested,
>> +                            n_revalidators_requested);
>>      }
>>  }
>>  
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 69e5834e766c..1501eca9a989 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -792,20 +792,8 @@ ofproto_type_set_config(const char *datapath_type, const struct smap *cfg)
>>  void
>>  ofproto_set_threads(int n_handlers_, int n_revalidators_)
>>  {
>> -    int threads = MAX(count_cpu_cores(), 2);
>> -
>>      n_revalidators = MAX(n_revalidators_, 0);
>>      n_handlers = MAX(n_handlers_, 0);
>> -
>> -    if (!n_revalidators) {
>> -        n_revalidators = n_handlers
>> -            ? MAX(threads - (int) n_handlers, 1)
>> -            : threads / 4 + 1;
>> -    }
>> -
>> -    if (!n_handlers) {
>> -        n_handlers = MAX(threads - (int) n_revalidators, 1);
>> -    }
>>  }
>>  
>>  void
>> -- 
>> 2.27.0
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 




More information about the dev mailing list