Alex, I have lost at least 32 packets while testing this patch yesterday(~15 configuration changes). Is it bad? What is worse: loosing of ~1-2 bursts of packets or a little stats about threads?
Best regards, Ilya Maximets. On 12.08.2015 23:09, Alex Wang wrote: > > > On Wed, Aug 12, 2015 at 1:10 AM, Ilya Maximets <i.maxim...@samsung.com > <mailto:i.maxim...@samsung.com>> wrote: > > Ok, but why don't just put dp->dp_purge_cb(dp->dp_purge_aux) right after > dp_netdev_destroy_all_pmds(dp) in dpif_netdev_pmd_set() ? In that case, > disabling of upcalls will not be necessary. > > > Hey Ilya, > > Calling it before 'dp_netdev_destroy_all_pmds()' can also allow the update > of 'dpif-netdev' flows' stats before they are gone. > > Also, since the pmd thread recreation change is deemed big configuration > change and enable/disable upcall is only one locking operation each, so I > think it will not matter very much. Would like to hear your concerns, > > Thanks, > Alex Wang, > > > > > P.S. The second letter of my name is 'L'. > > > Apology~ for keeping using it, > > > > > > On 11.08.2015 21:06, Alex Wang wrote: > > Thx, IIya for the review, please see my reply below, > > > > > > > > On Tue, Aug 11, 2015 at 4:41 AM, Ilya Maximets <i.maxim...@samsung.com > <mailto:i.maxim...@samsung.com> <mailto:i.maxim...@samsung.com > <mailto:i.maxim...@samsung.com>>> wrote: > > > > On 11.08.2015 05:47, Alex Wang 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> <mailto:i.maxim...@samsung.com > <mailto:i.maxim...@samsung.com>>> > > > Signed-off-by: Alex Wang <al...@nicira.com > <mailto:al...@nicira.com> <mailto: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); > > > } > > > > It is better to enable upcalls before restoring PMD threads to > prevent dropping of packets. > > > > > > > > Sorry, the comments are misleading, the "dp_netdev_reset_pmd_threads()" > > is for recreating all pmd threads (not exactly restoring). If we move > the > > "dp_netdev_enable_upcall()" before the function, then upcall will be > enabled > > before destroying all existing pmd threads, and we will have the same > issue > > again (the existing pmd threads will try installing flows/creating > ukeys before > > getting destroyed). > > > > Also, since recreating pmd threads is considered a major configuration > > change and is disruptive, I think it should be okay to resume upcall > handling > > right after all new pmd threads are created. > > > > > > > > > > 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 > > > > double 'if' here too. > > > > > > Thx, fixed it, > > > > > > > > > + * 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) > > > > > > > Best regards, Ilya Maximets. > > > > > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
