On 28/08/2015 18:51, "Joe Stringer" <joestrin...@nicira.com> wrote:
>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>
>
>That could even be... dare I say it... "Tested-by: ..." :-)
Oops... Fair enough, I was too eager to get the fix in.
Thanks for the comments, I'll let you handle the revalidator issues
then! :-)
>
>
>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.
>
>
>> 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);
>> }
>>
>>
>>
>>
>>> 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
>>>
>>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
