[ovs-dev] [PATCH v8 2/3] revalidator: Gather packets-per-second rate of flows
Eelco Chaudron
echaudro at redhat.com
Fri Oct 26 09:22:10 UTC 2018
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 <echaudro at 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.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>
>>> Reviewed-by: Simon Horman <simon.horman at netronome.com>
>>> Reviewed-by: Ben Pfaff <blp at 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
>>> dev at openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list