[ovs-dev] [PATCH V2 2/2] dpif-netdev: Purge all ukeys when reconfigure pmd.
ALeX Wang
ee07b291 at gmail.com
Sat Aug 29 01:18:02 UTC 2015
On 28 August 2015 at 10:51, Joe Stringer <joestringer at nicira.com> wrote:
> On 28 August 2015 at 09:41, Daniele Di Proietto <diproiettod at vmware.com>
> wrote:
> > I've tested it and it appears to correctly keep all the stats.
> >
> > Two comments inline, otherwise:
> >
> > Acked-by: Daniele Di Proietto <diproiettod at vmware.com>
>
> That could even be... dare I say it... "Tested-by: ..." :-)
>
>
> In general this patch seems fine. I'm not entirely confident what the
> worst case behaviour is though - for instance if revalidators are
> crazy busy (eg disable megaflows with port scan) and you reconfigure
> pmd threads, does it handle this gracefully? As far as I can tell,
> there is nothing stopping a revalidator from trying to delete the same
> ukey+flow at the same time. Also, typical purging like this happens
> independently by each revalidator thread where no thread can handle
> the same ukeys as another thread. This instead goes through all cmaps
> and picks out those with the current pmd id.
>
> The safest thing is to stop the world (ie, at least stop revalidators
> from processing further, if not completely disable the threads then
> re-enable when it's safe again), and an approach like that seems
> simpler.
>
>
Thx a lot for pointing this out! This is indeed an issue~~~
Based on our offlline discussion, I think I'll just destroy all
revalidators before
iterating the CMAP and restart them after iteration... and maybe modify the
pmd thread destroy functions to avoid multiple times of revalidators
recreation,
Will try and send out V3 patch~
Thanks,
Alex Wang,
>
> > On 28/08/2015 06:25, "Alex Wang" <ee07b291 at gmail.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 thread deletion in dpif-netdev). So, the
> >>ofproto-dpif-upcall module can react properly with deleting
> >>the ukeys and with collecting flows' last stats.
> >>
> >>Reported-by: Ilya Maximets <i.maximets at samsung.com>
> >>Signed-off-by: Alex Wang <ee07b291 at gmail.com>
> >>
> >>---
> >>PATCH->V2:
> >>- PATCH does not fix the problem,
> >>- call the purge_cb between pmd thread stopping and deletion.
> >> (which should be the right place)
> >>---
> >> lib/dpif-netdev.c | 23 ++++++++++++++++++++---
> >> lib/dpif-netlink.c | 1 +
> >> lib/dpif-provider.h | 11 ++++++++++-
> >> lib/dpif.c | 8 ++++++++
> >> lib/dpif.h | 13 +++++++++++++
> >> ofproto/ofproto-dpif-upcall.c | 36 ++++++++++++++++++++++++++++++++++++
> >> 6 files changed, 88 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> >>index f841876..2d187da 100644
> >>--- a/lib/dpif-netdev.c
> >>+++ b/lib/dpif-netdev.c
> >>@@ -204,6 +204,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 deletion). */
> >>+ dp_purge_callback *dp_purge_cb;
> >>+ void *dp_purge_aux;
> >>+
> >> /* Stores all 'struct dp_netdev_pmd_thread's. */
> >> struct cmap poll_threads;
> >>
> >>@@ -2851,7 +2856,7 @@ dp_netdev_destroy_pmd(struct dp_netdev_pmd_thread
> >>*pmd)
> >> /* Stops the pmd thread, removes it from the 'dp->poll_threads',
> >> * and unrefs the struct. */
> >> static void
> >>-dp_netdev_del_pmd(struct dp_netdev_pmd_thread *pmd)
> >>+dp_netdev_del_pmd(struct dp_netdev *dp, struct dp_netdev_pmd_thread
> *pmd)
> >> {
> >> /* Uninit the 'flow_cache' since there is
> >> * no actual thread uninit it for NON_PMD_CORE_ID. */
> >>@@ -2863,6 +2868,8 @@ dp_netdev_del_pmd(struct dp_netdev_pmd_thread *pmd)
> >> ovs_numa_unpin_core(pmd->core_id);
> >> xpthread_join(pmd->thread, NULL);
> >> }
> >>+ /* Purges the 'pmd''s flows after stoping the thread. */
> >>+ dp->dp_purge_cb(dp->dp_purge_aux, pmd->core_id);
> >
> > There's still the possibility a datapath is created through appctl
> > dpctl/add-dp.
> > In this case dp_purge_cb is NULL and a couple of tests fail (789, 790).
> > Adding a check should be enough:
> >
> > /* Purges the 'pmd''s flows after stopping the thread. */
> > if (dp->dp_purge_cb) {
> > dp->dp_purge_cb(dp->dp_purge_aux, pmd->core_id);
> > }
> >
> >
> >
> >
> >> cmap_remove(&pmd->dp->poll_threads, &pmd->node,
> >>hash_int(pmd->core_id, 0));
> >> dp_netdev_pmd_unref(pmd);
> >> }
> >>@@ -2874,7 +2881,7 @@ dp_netdev_destroy_all_pmds(struct dp_netdev *dp)
> >> struct dp_netdev_pmd_thread *pmd;
> >>
> >> CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
> >>- dp_netdev_del_pmd(pmd);
> >>+ dp_netdev_del_pmd(dp, pmd);
> >> }
> >> }
> >>
> >>@@ -2886,7 +2893,7 @@ dp_netdev_del_pmds_on_numa(struct dp_netdev *dp,
> >>int numa_id)
> >>
> >> CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
> >> if (pmd->numa_id == numa_id) {
> >>- dp_netdev_del_pmd(pmd);
> >>+ dp_netdev_del_pmd(dp, pmd);
> >> }
> >> }
> >> }
> >>@@ -3391,6 +3398,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)
> >> {
> >>@@ -3637,6 +3653,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 b8c27d8..ffeb124 100644
> >>--- a/lib/dpif-netlink.c
> >>+++ b/lib/dpif-netlink.c
> >>@@ -2309,6 +2309,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..5415897 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 '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..1b8a7b9 100644
> >>--- a/lib/dpif.h
> >>+++ b/lib/dpif.h
> >>@@ -787,6 +787,19 @@ 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 needs to provide the 'aux' pointer passed down by higher
> >>+ * layer from the dpif_register_notify_cb() function and the 'pmd_id' of
> >>+ * the polling thread.
> >>+ */
> >>+ typedef void dp_purge_callback(void *aux, unsigned pmd_id);
> >>+
> >>+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 419fd1a..5795c19 100644
> >>--- a/ofproto/ofproto-dpif-upcall.c
> >>+++ b/ofproto/ofproto-dpif-upcall.c
> >>@@ -316,6 +316,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);
> >>@@ -368,6 +369,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;
> >> }
> >>@@ -2195,6 +2197,40 @@ revalidator_purge(struct revalidator *revalidator)
> >> {
> >> revalidator_sweep__(revalidator, true);
> >> }
> >>+
> >>+/* In reaction to dpif purge, purges all 'ukey's with same 'pmd_id'. */
> >>+static void
> >>+dp_purge_cb(void *aux, unsigned pmd_id)
> >>+{
> >>+ struct udpif *udpif = aux;
> >>+ size_t i;
> >>+
> >>+ for (i = 0; i < N_UMAPS; i++) {
> >>+ uint64_t odp_actions_stub[1024 / 8];
> >>+ struct ofpbuf odp_actions =
> >>OFPBUF_STUB_INITIALIZER(odp_actions_stub);
> >
> > I think these variables above are unnecessary.
> >
> >>+ struct ukey_op ops[REVALIDATE_MAX_BATCH];
> >>+ struct udpif_key *ukey;
> >>+ struct umap *umap = &udpif->ukeys[i];
> >>+ size_t n_ops = 0;
> >>+
> >>+ CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) {
> >>+ if (ukey->pmd_id == pmd_id) {
> >>+ delete_op_init(udpif, &ops[n_ops++], ukey);
> >>+ if (n_ops == REVALIDATE_MAX_BATCH) {
> >>+ push_ukey_ops(udpif, umap, ops, n_ops);
> >>+ n_ops = 0;
> >>+ }
> >>+ }
> >>+ }
> >>+
> >>+ if (n_ops) {
> >>+ push_ukey_ops(udpif, umap, ops, n_ops);
> >>+ }
> >>+
> >>+ ofpbuf_uninit(&odp_actions);
> >>+ ovsrcu_quiesce();
> >>+ }
> >>+}
> >> ?
> >> static void
> >> upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> >>--
> >>1.9.1
> >>
> >
>
--
Alex Wang,
Open vSwitch developer
More information about the dev
mailing list