[ovs-dev,v6,2/3] revalidator: Gather packets-per-second rate of flows

This is the second patch in the patch-set to support dynamic rebalancing
of offloaded flows.

The packets-per-second (pps) rate for each flow is computed in the context
of revalidator threads when the flow stats are retrieved. The pps-rate is
computed only after a flow is revalidated and is not scheduled for
deletion. The parameters used to compute pps and the pps itself are saved
in udpif_key since they need to be persisted across iterations of
rebalancing.

Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
Reviewed-by: Sathya Perla <sathya.perla@broadcom.com>
---
 lib/dpif-provider.h           |  1 +
 ofproto/ofproto-dpif-upcall.c | 56 +++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

Bleep bloop.  Greetings Sriharsha Basavapatna via dev, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Sriharsha Basavapatna via dev <ovs-dev@openvswitch.org> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Lines checked: 135, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot

Hi Sriharsha,

thanks for your patch.

On Sat, Sep 15, 2018 at 09:40:09PM +0530, Sriharsha Basavapatna via dev wrote:
> This is the second patch in the patch-set to support dynamic rebalancing
> of offloaded flows.
> 
> The packets-per-second (pps) rate for each flow is computed in the context
> of revalidator threads when the flow stats are retrieved. The pps-rate is
> computed only after a flow is revalidated and is not scheduled for
> deletion. The parameters used to compute pps and the pps itself are saved
> in udpif_key since they need to be persisted across iterations of
> rebalancing.
> 
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> Reviewed-by: Sathya Perla <sathya.perla@broadcom.com>
> ---
>  lib/dpif-provider.h           |  1 +
>  ofproto/ofproto-dpif-upcall.c | 56 +++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 873b6e3d4..7a71f5c0a 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -39,6 +39,7 @@ struct dpif {
>      char *full_name;
>      uint8_t netflow_engine_type;
>      uint8_t netflow_engine_id;
> +    long long int current_ms;
>  };
>  
>  void dpif_init(struct dpif *, const struct dpif_class *, const char *name,
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 62222079f..9a36dca74 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -42,6 +42,8 @@
>  #include "tunnel.h"
>  #include "unixctl.h"
>  #include "openvswitch/vlog.h"
> +#include "lib/dpif-provider.h"
> +#include "lib/netdev-provider.h"
>  
>  #define MAX_QUEUE_LENGTH 512
>  #define UPCALL_MAX_BATCH 64
> @@ -304,6 +306,15 @@ struct udpif_key {
>  
>      uint32_t key_recirc_id;   /* Non-zero if reference is held by the ukey. */
>      struct recirc_refs recircs;  /* Action recirc IDs with references held. */
> +
> +#define OFFL_REBAL_INTVL_MSEC  3000	/* dynamic offload rebalance freq */

I'd be interested to here what considerations were taken
into account when selecting 3000ms.

> +    struct netdev *in_netdev;		/* in_odp_port's netdev */
> +    struct netdev *out_netdev;		/* out_odp_port's netdev */

in_netdev, out_netdev and possibly other fields are do not
appear to be used in this patch. I'd prefer if fields were
added in the patch where they are first used.

> +    bool offloaded;			/* True if flow is offloaded */
> +    uint64_t flow_pps_rate;		/* Packets-Per-Second rate */
> +    long long int flow_time;		/* last pps update time */
> +    uint64_t flow_packets;		/* #pkts seen in interval */
> +    uint64_t flow_backlog_packets;	/* prev-mode #pkts (offl or kernel) */
>  };
>  
>  /* Datapath operation with optional ukey attached. */
> @@ -1667,6 +1678,11 @@ ukey_create__(const struct nlattr *key, size_t key_len,
>      ukey->stats.used = used;
>      ukey->xcache = NULL;
>  
> +    ukey->offloaded = false;
> +    ukey->flow_time = 0;
> +    ukey->in_netdev = ukey->out_netdev = NULL;
> +    ukey->flow_packets = ukey->flow_backlog_packets = 0;
> +
>      ukey->key_recirc_id = key_recirc_id;
>      recirc_refs_init(&ukey->recircs);
>      if (xout) {
> @@ -2442,6 +2458,42 @@ reval_op_init(struct ukey_op *op, enum reval_result result,
>      }
>  }
>  
> +static uint64_t
> +udpif_flow_packet_delta(struct udpif_key *ukey, const struct dpif_flow *f)
> +{
> +    return f->stats.n_packets + ukey->flow_backlog_packets -
> +                ukey->flow_packets;
> +}
> +
> +static long long int
> +udpif_flow_time_delta(struct udpif *udpif, struct udpif_key *ukey)
> +{
> +    return (udpif->dpif->current_ms - ukey->flow_time) / 1000;
> +}
> +
> +/* Gather pps-rate for the given dpif_flow and save it in its ukey */
> +static void
> +udpif_update_flow_pps(struct udpif *udpif, struct udpif_key *ukey,
> +                      const struct dpif_flow *f)
> +{
> +    uint64_t pps;
> +
> +    /* Update pps-rate only when we are close to rebalance interval */
> +    if (udpif->dpif->current_ms - ukey->flow_time < OFFL_REBAL_INTVL_MSEC) {
> +        return;
> +    }
> +
> +    ovs_assert(f->stats.n_packets + ukey->flow_backlog_packets >=
> +               ukey->flow_packets);

Assert seems rather strong for tripping up data collection for to a heuristic.

> +
> +    ukey->offloaded = f->attrs.offloaded;
> +    pps = udpif_flow_packet_delta(ukey, f) /
> +                    udpif_flow_time_delta(udpif, ukey);
> +    ukey->flow_pps_rate = pps;
> +    ukey->flow_packets = ukey->flow_backlog_packets + f->stats.n_packets;
> +    ukey->flow_time = udpif->dpif->current_ms;
> +}
> +
>  static void
>  revalidate(struct revalidator *revalidator)
>  {
> @@ -2550,6 +2602,10 @@ revalidate(struct revalidator *revalidator)
>              }
>              ukey->dump_seq = dump_seq;
>  
> +            if (result != UKEY_DELETE) {
> +                udpif_update_flow_pps(udpif, ukey, f);

I am concerned about the cost of this for cases where the heuristic
is not enabled. Perhaps that is addressed in a follow-up patch. In
any case I think it would be best addressed in this patch.

> +            }
> +
>              if (result != UKEY_KEEP) {
>                  /* Takes ownership of 'recircs'. */
>                  reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
> -- 
> 2.18.0.rc1.1.g6f333ff
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Hi Simon,

Thanks for your review comments; please see my response inline.

On Mon, Sep 24, 2018 at 8:07 PM Simon Horman <simon.horman@netronome.com> wrote:
>
> Hi Sriharsha,
>
> thanks for your patch.
>
> On Sat, Sep 15, 2018 at 09:40:09PM +0530, Sriharsha Basavapatna via dev wrote:
> > This is the second patch in the patch-set to support dynamic rebalancing
> > of offloaded flows.
> >
> > The packets-per-second (pps) rate for each flow is computed in the context
> > of revalidator threads when the flow stats are retrieved. The pps-rate is
> > computed only after a flow is revalidated and is not scheduled for
> > deletion. The parameters used to compute pps and the pps itself are saved
> > in udpif_key since they need to be persisted across iterations of
> > rebalancing.
> >
> > Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> > Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> > Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> > Reviewed-by: Sathya Perla <sathya.perla@broadcom.com>
> > ---
> >  lib/dpif-provider.h           |  1 +
> >  ofproto/ofproto-dpif-upcall.c | 56 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 57 insertions(+)
> >
> > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> > index 873b6e3d4..7a71f5c0a 100644
> > --- a/lib/dpif-provider.h
> > +++ b/lib/dpif-provider.h
> > @@ -39,6 +39,7 @@ struct dpif {
> >      char *full_name;
> >      uint8_t netflow_engine_type;
> >      uint8_t netflow_engine_id;
> > +    long long int current_ms;
> >  };
> >
> >  void dpif_init(struct dpif *, const struct dpif_class *, const char *name,
> > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> > index 62222079f..9a36dca74 100644
> > --- a/ofproto/ofproto-dpif-upcall.c
> > +++ b/ofproto/ofproto-dpif-upcall.c
> > @@ -42,6 +42,8 @@
> >  #include "tunnel.h"
> >  #include "unixctl.h"
> >  #include "openvswitch/vlog.h"
> > +#include "lib/dpif-provider.h"
> > +#include "lib/netdev-provider.h"
> >
> >  #define MAX_QUEUE_LENGTH 512
> >  #define UPCALL_MAX_BATCH 64
> > @@ -304,6 +306,15 @@ struct udpif_key {
> >
> >      uint32_t key_recirc_id;   /* Non-zero if reference is held by the ukey. */
> >      struct recirc_refs recircs;  /* Action recirc IDs with references held. */
> > +
> > +#define OFFL_REBAL_INTVL_MSEC  3000  /* dynamic offload rebalance freq */
>
> I'd be interested to here what considerations were taken
> into account when selecting 3000ms.

Since we compute per-second packet rate (pps) and also because revalidation
runs at 500 msec, we need to have the rebalancing duration > 1 second. We also
want to provide fast response to flows with higher pps-rate that might be
pending. Hence we are using 3 seconds interval. If this seems aggressive, we
can increase it; let me know your thoughts.

>
> > +    struct netdev *in_netdev;                /* in_odp_port's netdev */
> > +    struct netdev *out_netdev;               /* out_odp_port's netdev */
>
> in_netdev, out_netdev and possibly other fields are do not
> appear to be used in this patch. I'd prefer if fields were
> added in the patch where they are first used.

done.
>
> > +    bool offloaded;                  /* True if flow is offloaded */
> > +    uint64_t flow_pps_rate;          /* Packets-Per-Second rate */
> > +    long long int flow_time;         /* last pps update time */
> > +    uint64_t flow_packets;           /* #pkts seen in interval */
> > +    uint64_t flow_backlog_packets;   /* prev-mode #pkts (offl or kernel) */
> >  };
> >
> >  /* Datapath operation with optional ukey attached. */
> > @@ -1667,6 +1678,11 @@ ukey_create__(const struct nlattr *key, size_t key_len,
> >      ukey->stats.used = used;
> >      ukey->xcache = NULL;
> >
> > +    ukey->offloaded = false;
> > +    ukey->flow_time = 0;
> > +    ukey->in_netdev = ukey->out_netdev = NULL;
> > +    ukey->flow_packets = ukey->flow_backlog_packets = 0;
> > +
> >      ukey->key_recirc_id = key_recirc_id;
> >      recirc_refs_init(&ukey->recircs);
> >      if (xout) {
> > @@ -2442,6 +2458,42 @@ reval_op_init(struct ukey_op *op, enum reval_result result,
> >      }
> >  }
> >
> > +static uint64_t
> > +udpif_flow_packet_delta(struct udpif_key *ukey, const struct dpif_flow *f)
> > +{
> > +    return f->stats.n_packets + ukey->flow_backlog_packets -
> > +                ukey->flow_packets;
> > +}
> > +
> > +static long long int
> > +udpif_flow_time_delta(struct udpif *udpif, struct udpif_key *ukey)
> > +{
> > +    return (udpif->dpif->current_ms - ukey->flow_time) / 1000;
> > +}
> > +
> > +/* Gather pps-rate for the given dpif_flow and save it in its ukey */
> > +static void
> > +udpif_update_flow_pps(struct udpif *udpif, struct udpif_key *ukey,
> > +                      const struct dpif_flow *f)
> > +{
> > +    uint64_t pps;
> > +
> > +    /* Update pps-rate only when we are close to rebalance interval */
> > +    if (udpif->dpif->current_ms - ukey->flow_time < OFFL_REBAL_INTVL_MSEC) {
> > +        return;
> > +    }
> > +
> > +    ovs_assert(f->stats.n_packets + ukey->flow_backlog_packets >=
> > +               ukey->flow_packets);
>
> Assert seems rather strong for tripping up data collection for to a heuristic.

Removed.
>
> > +
> > +    ukey->offloaded = f->attrs.offloaded;
> > +    pps = udpif_flow_packet_delta(ukey, f) /
> > +                    udpif_flow_time_delta(udpif, ukey);
> > +    ukey->flow_pps_rate = pps;
> > +    ukey->flow_packets = ukey->flow_backlog_packets + f->stats.n_packets;
> > +    ukey->flow_time = udpif->dpif->current_ms;
> > +}
> > +
> >  static void
> >  revalidate(struct revalidator *revalidator)
> >  {
> > @@ -2550,6 +2602,10 @@ revalidate(struct revalidator *revalidator)
> >              }
> >              ukey->dump_seq = dump_seq;
> >
> > +            if (result != UKEY_DELETE) {
> > +                udpif_update_flow_pps(udpif, ukey, f);
>
> I am concerned about the cost of this for cases where the heuristic
> is not enabled. Perhaps that is addressed in a follow-up patch. In
> any case I think it would be best addressed in this patch.

Added config parameter check to this patch.

Thanks,
-Harsha
>
> > +            }
> > +
> >              if (result != UKEY_KEEP) {
> >                  /* Takes ownership of 'recircs'. */
> >                  reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
> > --
> > 2.18.0.rc1.1.g6f333ff
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >