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.maxim...@samsung.com> wrote: > > On 17.08.2015 20:35, Alex Wang wrote: > > > > > > On Mon, Aug 17, 2015 at 10:22 AM, Daniele Di Proietto < > diproiet...@vmware.com <mailto:diproiet...@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.maxim...@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" <al...@nicira.com <mailto: > al...@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.maxim...@samsung.com <mailto: > i.maxim...@samsung.com>> > > >Signed-off-by: Alex Wang <al...@nicira.com <mailto:al...@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 > > > > > > > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
