[ovs-dev] [PATCH v6 2/3] revalidator: Gather packets-per-second rate of flows
Simon Horman
simon.horman at netronome.com
Mon Sep 24 14:37:17 UTC 2018
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 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>
> ---
> 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list