diff mbox series

[ovs-dev,v8,2/3] revalidator: Gather packets-per-second rate of flows

Message ID 20181018161314.27854-3-sriharsha.basavapatna@broadcom.com
State Deferred
Headers show
Series Support dynamic rebalancing of offloaded flows | expand

Commit Message

Li,Rongqing via dev Oct. 18, 2018, 4:13 p.m. UTC
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.basavapatna@broadcom.com>
Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
Reviewed-by: Sathya Perla <sathya.perla@broadcom.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Ben Pfaff <blp@ovn.org>
---
 lib/dpif-provider.h           |  1 +
 ofproto/ofproto-dpif-upcall.c | 51 +++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

Comments

0-day Robot Oct. 18, 2018, 5 p.m. UTC | #1
Bleep bloop.  Greetings Sriharsha Basavapatna via dev, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Sriharsha Basavapatna via dev <ovs-dev@openvswitch.org> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Lines checked: 132, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Eelco Chaudron Oct. 19, 2018, 2:22 p.m. UTC | #2
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.

> Signed-off-by: Sriharsha Basavapatna 
> <sriharsha.basavapatna@broadcom.com>
> Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> Reviewed-by: Sathya Perla <sathya.perla@broadcom.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> Reviewed-by: Ben Pfaff <blp@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.

>      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.

> +}
> +
> +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.

> +    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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Li,Rongqing via dev Oct. 25, 2018, 2:01 p.m. UTC | #3
Hi Eelco,

Thanks for your comments, please see my response below.
On Fri, Oct 19, 2018 at 7:52 PM Eelco Chaudron <echaudro@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.basavapatna@broadcom.com>
> > Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> > Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> > Reviewed-by: Sathya Perla <sathya.perla@broadcom.com>
> > Reviewed-by: Simon Horman <simon.horman@netronome.com>
> > Reviewed-by: Ben Pfaff <blp@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
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eelco Chaudron Oct. 26, 2018, 9:22 a.m. UTC | #4
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@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@broadcom.com>
>>> Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
>>> Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
>>> Reviewed-by: Sathya Perla <sathya.perla@broadcom.com>
>>> Reviewed-by: Simon Horman <simon.horman@netronome.com>
>>> Reviewed-by: Ben Pfaff <blp@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@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

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;
+
     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;
+}
+
+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;
+    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,