[ovs-dev] [PATCH 1/2] dpif-netdev: Purge all ukeys when reconfigure pmd.

Ilya Maximets i.maximets at samsung.com
Thu Aug 13 10:24:36 UTC 2015


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.maximets at samsung.com <mailto:i.maximets at 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.maximets at samsung.com <mailto:i.maximets at samsung.com> <mailto:i.maximets at samsung.com <mailto:i.maximets at 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.maximets at samsung.com <mailto:i.maximets at samsung.com> <mailto:i.maximets at samsung.com <mailto:i.maximets at samsung.com>>>
>     >     > Signed-off-by: Alex Wang <alexw at nicira.com <mailto:alexw at nicira.com> <mailto:alexw at nicira.com <mailto:alexw at 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.
>     >
>     >
> 
> 



More information about the dev mailing list