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
