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

Eelco Chaudron echaudro at redhat.com
Fri Oct 19 14:22:47 UTC 2018


On 18 Oct 2018, at 18:13, 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.

Was there a specific reason to go with pps and not bytes per second? 
Asking as larger packets might need more copying around vs a lot of 
small packets need more flow processing.

> Signed-off-by: Sriharsha Basavapatna 
> <sriharsha.basavapatna at broadcom.com>
> Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru at broadcom.com>
> Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru at broadcom.com>
> Reviewed-by: Sathya Perla <sathya.perla at broadcom.com>
> Reviewed-by: Simon Horman <simon.horman at netronome.com>
> Reviewed-by: Ben Pfaff <blp at ovn.org>
> ---
>  lib/dpif-provider.h           |  1 +
>  ofproto/ofproto-dpif-upcall.c | 51 
> +++++++++++++++++++++++++++++++++++
>  2 files changed, 52 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..a372d6252 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,13 @@ 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 
> */
> +    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 +1676,10 @@ 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->flow_packets = ukey->flow_backlog_packets = 0;
> +

For clarity I think we need to set the flow_pps_rate to 0 also.

>      ukey->key_recirc_id = key_recirc_id;
>      recirc_refs_init(&ukey->recircs);
>      if (xout) {
> @@ -2442,6 +2455,39 @@ 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;

It does not make sense to me why the flow_backlog_packets get added 
here, and also to the flow_packets? See also below.

> +}
> +
> +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;
> +    }
> +
> +    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;

In the help the flow_packets is described as "#pkts seen in interval" 
why do we need to add flow_backlog_packets? They are from the previous 
mode? See also comment above.

> +    ukey->flow_time = udpif->dpif->current_ms;
> +}
> +
>  static void
>  revalidate(struct revalidator *revalidator)
>  {
> @@ -2550,6 +2596,11 @@ revalidate(struct revalidator *revalidator)
>              }
>              ukey->dump_seq = dump_seq;
>
> +            if (netdev_is_offload_rebalance_policy_enabled() &&
> +                result != UKEY_DELETE) {
> +                udpif_update_flow_pps(udpif, ukey, f);
> +            }
> +
>              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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list