On 25 Oct 2018, at 16:01, Sriharsha Basavapatna wrote:
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.
Sound good for now, guess some testing can be done later to determine if
bps might be needed.
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.
Why would we need to save these values? If the mode changes a new
interval starts, so only the packets for this interval matter? Could
this because you do not reset flow_packets when calling
udpif_set_ukey_backlog_packets()?
It also might be that I’m missing somehting as it’s Friday :)
Anyhow I think it does require some documentation in the code.
+}
+
+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
