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

Alex Wang alexw at nicira.com
Wed Aug 26 15:52:43 UTC 2015


Sorry for the delayed reply,

I tested this patch yesterday, found that this is not right fix,

Turns out, dp_netdev_reset_pmd_threads() just creates (or restores) pmd
threads...  and we call dp_netdev_destroy_all_pmds() at the beginning of the
function.  So, when purge_cb() is called, the flows in each 'pmd' have
already
gone...  So, we did not harvest the last bit of stats,

Instead, I think the right place to call purge_cb() is between pmd thread
stop
and deletion in dp_netdev_del_pmd().  Please see the V2 patch for details,

Thanks,
Alex Wang,

On Mon, Aug 17, 2015 at 11:09 PM, Ilya Maximets <i.maximets at samsung.com>
wrote:

>
> On 17.08.2015 20:35, Alex Wang wrote:
> >
> >
> > On Mon, Aug 17, 2015 at 10:22 AM, Daniele Di Proietto <
> diproiettod at vmware.com <mailto:diproiettod at vmware.com>> wrote:
> >
> >     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?
> >
> >
> > Thx, Daniele,
> >
> > Ilya, if you are also okay, I'll push this to master.
>
> Ok. I think, it's good.
>
> Acked-by: Ilya Maximets <i.maximets at samsung.com>
>
> >
> >
> >
> >     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?
> >
> >
> > For this issue, to be more specific, the ukeys are never deleted if:
> > 1. datapath flow disappears,
> > 2. && there is no userspace changes which causes increments of reval_seq.
> >
> > In other words, once 'need_revalidate' is true, the obsolete ukeys
> should be
> > deleted at revaidator_sweep() stage.  Right?
> >
> > This sounds to me like a different issue and may require more thinking,
> so I'd
> > like to keep it a separate issue,
> >
> > Thanks,
> > Alex Wang,
> >
> >
> >     Thanks
> >
> >     On 11/08/2015 03:47, "Alex Wang" <alexw at nicira.com <mailto:
> 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 <mailto:
> i.maximets at samsung.com>>
> >     >Signed-off-by: Alex Wang <alexw at nicira.com <mailto: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