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 <[email protected]> > Co-authored-by: Venkat Duvvuru <[email protected]> > Signed-off-by: Venkat Duvvuru <[email protected]> > Reviewed-by: Sathya Perla <[email protected]> > --- > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
