Hi Simon,
Thanks for your review comments; please see my response inline.
On Mon, Sep 24, 2018 at 8:07 PM Simon Horman <simon.hor...@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.basavapa...@broadcom.com>
> > Co-authored-by: Venkat Duvvuru <venkatkumar.duvv...@broadcom.com>
> > Signed-off-by: Venkat Duvvuru <venkatkumar.duvv...@broadcom.com>
> > Reviewed-by: Sathya Perla <sathya.pe...@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
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
