[ovs-dev] [PATCH 1/2] dpif-netdev: Purge all ukeys when reconfigure pmd.

Daniele Di Proietto diproiettod at vmware.com
Mon Aug 17 17:22:12 UTC 2015


The patch looks good to me.

I think that losing some packets when the thread are reconfigured
is unavoidable, considering that often the NIC must be stopped
to change the number of rxqs.  Does this make sense?

Related to this, I've had an offline discussion with Ethan
and we realized that ukeys are never deleted if the datapath
flow disappear for other reasons (e.g. ovs-dpctl del-flows).
Should we address these issues toghether?

Thanks

On 11/08/2015 03:47, "Alex Wang" <alexw at nicira.com> wrote:

>When dpdk configuration changes, all pmd threads are recreated
>and rx queues of each port are reloaded.  After this process,
>rx queue could be mapped to a different pmd thread other than
>the one before reconfiguration.  However, this is totally
>transparent to ofproto layer modules.  So, if the ofproto-dpif-upcall
>module still holds ukeys generated before pmd thread recreation,
>this old ukey will collide with the ukey for the new upcalls
>from same traffic flow, causing flow installation failure.
>
>To fix the bug, this commit adds a new call-back function
>in dpif layer for notifying upper layer the purging of datapath
>(e.g. pmd threads recreation in dpif-netdev).  So, the
>ofproto-dpif-upcall module can react properly with deleting
>all the ukeys and with collecting all flows' last stats.
>
>Reported-by: Ilya Maximets <i.maximets at samsung.com>
>Signed-off-by: Alex Wang <alexw at nicira.com>
>---
> lib/dpif-netdev.c             |   25 +++++++++++++++++++++++++
> lib/dpif-netlink.c            |    1 +
> lib/dpif-provider.h           |   11 ++++++++++-
> lib/dpif.c                    |    8 ++++++++
> lib/dpif.h                    |   12 ++++++++++++
> ofproto/ofproto-dpif-upcall.c |   16 ++++++++++++++++
> 6 files changed, 72 insertions(+), 1 deletion(-)
>
>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>index c144352..a54853f 100644
>--- a/lib/dpif-netdev.c
>+++ b/lib/dpif-netdev.c
>@@ -203,6 +203,11 @@ struct dp_netdev {
>     upcall_callback *upcall_cb;  /* Callback function for executing
>upcalls. */
>     void *upcall_aux;
> 
>+    /* Callback function for notifying the purging of dp flows (during
>+     * reseting pmd threads). */
>+    dp_purge_callback *dp_purge_cb;
>+    void *dp_purge_aux;
>+
>     /* Stores all 'struct dp_netdev_pmd_thread's. */
>     struct cmap poll_threads;
> 
>@@ -223,6 +228,8 @@ struct dp_netdev {
> 
> static struct dp_netdev_port *dp_netdev_lookup_port(const struct
>dp_netdev *dp,
>                                                     odp_port_t);
>+static void dp_netdev_disable_upcall(struct dp_netdev *);
>+static void dp_netdev_enable_upcall(struct dp_netdev *);
> 
> enum dp_stat_type {
>     DP_STAT_EXACT_HIT,          /* Packets that had an exact match
>(emc). */
>@@ -2440,10 +2447,18 @@ dpif_netdev_pmd_set(struct dpif *dpif, unsigned
>int n_rxqs, const char *cmask)
>         free(dp->pmd_cmask);
>         dp->pmd_cmask = cmask ? xstrdup(cmask) : NULL;
> 
>+        /* Disables upcall and notifies higher layer about the incoming
>+         * datapath purging. */
>+        dp_netdev_disable_upcall(dp);
>+        dp->dp_purge_cb(dp->dp_purge_aux);
>+
>         /* Restores the non-pmd. */
>         dp_netdev_set_nonpmd(dp);
>         /* Restores all pmd threads. */
>         dp_netdev_reset_pmd_threads(dp);
>+
>+        /* Enables upcall after pmd reconfiguration. */
>+        dp_netdev_enable_upcall(dp);
>     }
> 
>     return 0;
>@@ -3419,6 +3434,15 @@ struct dp_netdev_execute_aux {
> };
> 
> static void
>+dpif_netdev_register_dp_purge_cb(struct dpif *dpif, dp_purge_callback
>*cb,
>+                                 void *aux)
>+{
>+    struct dp_netdev *dp = get_dp_netdev(dpif);
>+    dp->dp_purge_aux = aux;
>+    dp->dp_purge_cb = cb;
>+}
>+
>+static void
> dpif_netdev_register_upcall_cb(struct dpif *dpif, upcall_callback *cb,
>                                void *aux)
> {
>@@ -3665,6 +3689,7 @@ const struct dpif_class dpif_netdev_class = {
>     NULL,                       /* recv */
>     NULL,                       /* recv_wait */
>     NULL,                       /* recv_purge */
>+    dpif_netdev_register_dp_purge_cb,
>     dpif_netdev_register_upcall_cb,
>     dpif_netdev_enable_upcall,
>     dpif_netdev_disable_upcall,
>diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>index 8884a9f..f8f3092 100644
>--- a/lib/dpif-netlink.c
>+++ b/lib/dpif-netlink.c
>@@ -2314,6 +2314,7 @@ const struct dpif_class dpif_netlink_class = {
>     dpif_netlink_recv,
>     dpif_netlink_recv_wait,
>     dpif_netlink_recv_purge,
>+    NULL,                       /* register_dp_purge_cb */
>     NULL,                       /* register_upcall_cb */
>     NULL,                       /* enable_upcall */
>     NULL,                       /* disable_upcall */
>diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>index 28ea86f..8f2690f 100644
>--- a/lib/dpif-provider.h
>+++ b/lib/dpif-provider.h
>@@ -361,13 +361,22 @@ struct dpif_class {
>      * return. */
>     void (*recv_purge)(struct dpif *dpif);
> 
>+    /* When 'dpif' is about to purge the datapath, the higher layer may
>want
>+     * to be notified so that it could try reacting accordingly (e.g.
>grabbing
>+     * all flow stats before they are gone).
>+     *
>+     * Registers an upcall callback function with 'dpif'.  This is only
>used if
>+     * if 'dpif' needs to notify the purging of datapath.  'aux' is
>passed to
>+     * the callback on invocation. */
>+    void (*register_dp_purge_cb)(struct dpif *, dp_purge_callback *,
>void *aux);
>+
>     /* For datapaths that run in userspace (i.e. dpif-netdev), threads
>polling
>      * for incoming packets can directly call upcall functions instead of
>      * offloading packet processing to separate handler threads.
>Datapaths
>      * that directly call upcall functions should use the functions
>below to
>      * to register an upcall function and enable / disable upcalls.
>      *
>-     * Registers an upcall callback function with 'dpif'. This is only
>used if
>+     * Registers an upcall callback function with 'dpif'. This is only
>used
>      * if 'dpif' directly executes upcall functions. 'aux' is passed to
>the
>      * callback on invocation. */
>     void (*register_upcall_cb)(struct dpif *, upcall_callback *, void
>*aux);
>diff --git a/lib/dpif.c b/lib/dpif.c
>index 6e26f30..9a67a24 100644
>--- a/lib/dpif.c
>+++ b/lib/dpif.c
>@@ -1346,6 +1346,14 @@ dpif_handlers_set(struct dpif *dpif, uint32_t
>n_handlers)
> }
> 
> void
>+dpif_register_dp_purge_cb(struct dpif *dpif, dp_purge_callback *cb, void
>*aux)
>+{
>+    if (dpif->dpif_class->register_dp_purge_cb) {
>+        dpif->dpif_class->register_dp_purge_cb(dpif, cb, aux);
>+    }
>+}
>+
>+void
> dpif_register_upcall_cb(struct dpif *dpif, upcall_callback *cb, void
>*aux)
> {
>     if (dpif->dpif_class->register_upcall_cb) {
>diff --git a/lib/dpif.h b/lib/dpif.h
>index ea9caf8..dca5d00 100644
>--- a/lib/dpif.h
>+++ b/lib/dpif.h
>@@ -787,6 +787,18 @@ struct dpif_upcall {
>     struct nlattr *actions;    /* Argument to OVS_ACTION_ATTR_USERSPACE.
>*/
> };
> 
>+/* A callback to notify higher layer of dpif about to be purged, so that
>+ * higher layer could try reacting to this (e.g. grabbing all flow stats
>+ * before they are gone).  This function is currently implemented only by
>+ * dpif-netdev.
>+ *
>+ * The caller only needs to provide the 'aux' pointer passed down by
>higher
>+ * layer from the dpif_register_notify_cb() function.
>+ */
>+typedef void dp_purge_callback(void *aux);
>+
>+void dpif_register_dp_purge_cb(struct dpif *, dp_purge_callback *, void
>*aux);
>+
> /* A callback to process an upcall, currently implemented only by
>dpif-netdev.
>  *
>  * The caller provides the 'packet' and 'flow' to process, the
>corresponding
>diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>index 0f2e186..6385abc 100644
>--- a/ofproto/ofproto-dpif-upcall.c
>+++ b/ofproto/ofproto-dpif-upcall.c
>@@ -306,6 +306,7 @@ static int upcall_receive(struct upcall *, const
>struct dpif_backer *,
> static void upcall_uninit(struct upcall *);
> 
> static upcall_callback upcall_cb;
>+static dp_purge_callback dp_purge_cb;
> 
> static atomic_bool enable_megaflows = ATOMIC_VAR_INIT(true);
> static atomic_bool enable_ufid = ATOMIC_VAR_INIT(true);
>@@ -358,6 +359,7 @@ udpif_create(struct dpif_backer *backer, struct dpif
>*dpif)
>     }
> 
>     dpif_register_upcall_cb(dpif, upcall_cb, udpif);
>+    dpif_register_dp_purge_cb(dpif, dp_purge_cb, udpif);
> 
>     return udpif;
> }
>@@ -1110,6 +1112,20 @@ out:
>     return error;
> }
> 
>+/* In reaction to dpif purge, purges all 'ukey's. */
>+static void
>+dp_purge_cb(void *aux)
>+{
>+    struct udpif *udpif = aux;
>+    size_t i;
>+
>+    for (i = 0; i < udpif->n_revalidators; i++) {
>+        struct revalidator *revalidator = &udpif->revalidators[i];
>+
>+        revalidator_purge(revalidator);
>+    }
>+}
>+
> static int
> process_upcall(struct udpif *udpif, struct upcall *upcall,
>                struct ofpbuf *odp_actions, struct flow_wildcards *wc)
>-- 
>1.7.9.5
>




More information about the dev mailing list