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

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>
---
 lib/dpif-provider.h           |   1 +
 ofproto/ofproto-dpif-upcall.c | 152 ++++++++++++++++++++++++++++++++++
 2 files changed, 153 insertions(+)

On Thu, Jul 12, 2018 at 12:59:32PM +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.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>

Thanks for the patch.  I have some comments.

This code takes references to netdevs and stores them in udpif_key, but
it never releases them.

This code uses "float" but usually that's slower than "double" on modern
hardware.  If floating point is needed, I recommend double.  What is
your reasoning for using floating point, by the way?  Especially given
that the flow time delta is being rounded down to an integer number of
seconds, I don't yet see the need.

dpif_flow_to_netdevs() seems to want to treat flows with multiple
outputs differently, but I don't think it does because it breaks out of
the loop as soon as it finds the first output.

The variable 'forward' in dpif_flow_to_netdevs() doesn't seem needed
because forward == true is the same as out_port != ODPP_NONE.

In dpif_flow_to_netdevs(), the assignments in this 'if' statement seem
redundant with those at the top of the function:

            if (forward) {
                ukey->out_netdev = NULL;
                ukey->in_netdev = NULL;
                return;
            }

In dpif_flow_to_netdevs(), it seems that one could use
nl_attr_get_odp_port() here:
            in_port = *(odp_port_t *)nl_attr_get(k);

The 'type' var in the second NL_ATTR_FOR_EACH in dpif_flow_to_netdevs()
could be declared as the more specific type enum ovs_key_attr.

In udpif_update_flow_pps(), I would omit the inner parentheses in both
of these places:

    if ((udpif->dpif->current_ms - ukey->flow_time) <= OFFL_REBAL_INTVL_MSEC) {
        return;
    }
    if ((f->stats.n_packets + ukey->flow_backlog_packets) <
        ukey->flow_packets) {
        return;
    }

It's not clear to me that udpif_update_flow_pps() should do nothing in
the cases where it bails out.  Is that carefully thought out?

The cast in udpif_flow_packet_delta() is not useful.

The temporary dpif_flow used in udpif_update_flow_pps() ->
udpif_key_to_flow_netdevs() -> dpif_flow_to_netdevs() is curious.  I am
not sure why the ukey isn't used directly.  There are no other callers.

Thanks,

Ben.

On Fri, Aug 31, 2018 at 11:22 PM, Ben Pfaff <blp@ovn.org> wrote:
> On Thu, Jul 12, 2018 at 12:59:32PM +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.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>
>
> Thanks for the patch.  I have some comments.

Thanks for your review comments. Please see my response inline.
>
> This code takes references to netdevs and stores them in udpif_key, but
> it never releases them.

Thanks for catching this; fixed it.
>
> This code uses "float" but usually that's slower than "double" on modern
> hardware.  If floating point is needed, I recommend double.  What is
> your reasoning for using floating point, by the way?  Especially given
> that the flow time delta is being rounded down to an integer number of
> seconds, I don't yet see the need.

The rate computation originally was using per millisec rate (ppms) and
the float type was needed to distinguish flows with low ppms rate (< 1000
per sec or < 1 per ms). Like you pointed out we don't need it now that
we are using per-second rate. I've changed it to an uint64.
>
> dpif_flow_to_netdevs() seems to want to treat flows with multiple
> outputs differently, but I don't think it does because it breaks out of
> the loop as soon as it finds the first output.

We just want to check that the flow as an output action (and port).
That's the reason why it breaks out as soon as it finds the first output
port. I've updated comments explaining it.
>
> The variable 'forward' in dpif_flow_to_netdevs() doesn't seem needed
> because forward == true is the same as out_port != ODPP_NONE.

removed this code.
>
> In dpif_flow_to_netdevs(), the assignments in this 'if' statement seem
> redundant with those at the top of the function:
>
>             if (forward) {
>                 ukey->out_netdev = NULL;
>                 ukey->in_netdev = NULL;
>                 return;
>             }

removed.
>
> In dpif_flow_to_netdevs(), it seems that one could use
> nl_attr_get_odp_port() here:
>             in_port = *(odp_port_t *)nl_attr_get(k);

done
>
> The 'type' var in the second NL_ATTR_FOR_EACH in dpif_flow_to_netdevs()
> could be declared as the more specific type enum ovs_key_attr.

done
>
> In udpif_update_flow_pps(), I would omit the inner parentheses in both
> of these places:
>
>     if ((udpif->dpif->current_ms - ukey->flow_time) <= OFFL_REBAL_INTVL_MSEC) {
>         return;
>     }
>     if ((f->stats.n_packets + ukey->flow_backlog_packets) <
>         ukey->flow_packets) {
>         return;
>     }

done
>
> It's not clear to me that udpif_update_flow_pps() should do nothing in
> the cases where it bails out.  Is that carefully thought out?

Yes, the first one is needed; updated comments. The second one should have
been an assert(), changed it.
>
> The cast in udpif_flow_packet_delta() is not useful.

done
>
> The temporary dpif_flow used in udpif_update_flow_pps() ->
> udpif_key_to_flow_netdevs() -> dpif_flow_to_netdevs() is curious.  I am
> not sure why the ukey isn't used directly.  There are no other callers.

Thanks for suggesting this. Removed tmp dpif_flow and started using ukey
directly; simplified it into a single routine - ukey_to_flow_netdevs().
>
> Thanks,
>
> Ben.

Thanks,
-Harsha