Hi Eelco,
Thanks for your comments, please see my response below.
On Fri, Oct 19, 2018 at 7:52 PM Eelco Chaudron <echau...@redhat.com> wrote:
>
> 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.
It's a good point, it was considered; the config parameter until patch-v4
was defined as a string with a value of "pps-rate" to enable packets-per-sec
based rebalancing. The other value that we wanted to provide was "bps-rate"
for bytes-per-second rate. But to keep it simple for now, it was changed
to a boolean.
>
> > 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>
> > Reviewed-by: Simon Horman <simon.hor...@netronome.com>
> > Reviewed-by: Ben Pfaff <b...@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.
agreed
>
> > 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.
When we switch modes (kernel->offload or vice versa), the flow stat counters
(f->stats.n_packets) get reset and start from 0 again. This leads to wrong
computation of flow pps after switching modes. To avoid this, we keep track
of packets in the previous mode at the time of switching modes. Then in the
new mode, we start saving the packets seen so far at every iteration in
ukey->flow_packets, including the total packets seen in previous mode. So
while computing the packet delta, we need to account for the backlog packets
(since we'd have saved them in prev iteration in ukey->flow_packets); thus we
add flow->backlog_packets to f->stats.n_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;
> > + }
> > +
> > + 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.
Please see my previous comment.
Thanks,
-Harsha
>
> > + 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
> > 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
