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 <[email protected]>
> Co-authored-by: Venkat Duvvuru <[email protected]>
> Signed-off-by: Venkat Duvvuru <[email protected]>
> Reviewed-by: Sathya Perla <[email protected]>
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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
