Thx a lot for the quick review,
On 28 August 2015 at 09:41, Daniele Di Proietto <diproiet...@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 <diproiet...@vmware.com>
>
> Thanks for fixing this Alex!
>
> On 28/08/2015 06:25, "Alex Wang" <ee07b...@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.maxim...@samsung.com>
> >Signed-off-by: Alex Wang <ee07b...@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);
> }
>
>
Thx a lot for pointing this out~! fixed
>
> > 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.
>
Thx, removed,
>
> >+ 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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
