[ovs-dev] [PATCH v6 3/3] revalidator: Rebalance offloaded flows based on the pps rate

Simon Horman horms at verge.net.au
Fri Oct 12 09:23:31 UTC 2018


On Thu, Sep 27, 2018 at 12:06:42PM +0530, Sriharsha Basavapatna via dev wrote:
> Hi Simon,
> 
> Thanks for your review comments; please see my response inline.
> 
> On Mon, Sep 24, 2018 at 8:16 PM Simon Horman <simon.horman at netronome.com> wrote:
> >
> > Hi Sriharsha,
> >
> > thanks for your patch.
> >
> > I am pleased to see work in this area. I do however, have some questions
> > about the implementation. Please see comments inline.

...

> > > +/*
> > > + * Rebalance offloaded flows on HW netdevs that are in OOR state.
> > > + */
> > > +static void
> > > +udpif_flow_rebalance(struct udpif *udpif)
> > > +{
> > > +    struct udpif_key **sort_flows = NULL;
> > > +    size_t alloc_flow_count = 0;
> > > +    size_t total_flow_count = 0;
> > > +    int oor_netdev_count = 0;
> > > +    int offload_index = 0;
> > > +    int pending_index;
> > > +
> > > +    /* Collect flows (offloaded and pending) that reference OOR netdevs */
> > > +    for (size_t i = 0; i < N_UMAPS; i++) {
> > > +        struct udpif_key *ukey;
> > > +        struct umap *umap = &udpif->ukeys[i];
> > > +
> > > +        CMAP_FOR_EACH (ukey, cmap_node, &umap->cmap) {
> > > +            ukey_to_flow_netdevs(udpif, ukey);
> > > +            sort_flows = udpif_build_oor_flows(sort_flows, &total_flow_count,
> > > +                                               &alloc_flow_count, ukey,
> > > +                                               &oor_netdev_count);
> > > +        }
> > > +    }
> >
> > If I read the above correctly it is collecting all flows present in the
> > system into a sorted list and then operating on that list below. I am
> > concerned about memory and CPU impact of this if a large number of flows,
> > lets say 500k, are present.
> 
> It is not collecting all flows present in the system. It used to, until v4
> version of the code; Ben had a similar comment on this to combine the 2 passes
> (collecting all flows and discarding non-OOR flows) into one, saving both
> space and time. We now add a flow into sort_flows[] only if its in-netdev is
> OOR (dynamically resizing sort_flows[]). So, only a subset of the flows in the
> system, that are referencing an OOR-netdev as input port are added to
> sort_flows[] and we then sort them. Another comment from Ben was to prevent
> the entire rebalancing work by first checking whether the system has any
> out-of-resource devices. A new function netdev_any_oor(), is first invoked by
> the caller.
> 
> Thanks, -Harsha

Thanks for your response to this and my other review comments and apologies
for not following-up earlier (in this case CCing me would have helped me).

Given your explanation I think that what you have is a reasonable start
and I don't object to it given that the feature must be enabled. But
I do still have a concern about the cost of this.

As I understand things the OOR condition kicks in for an offload device
when it rejects flows due to resource contention.  I am still concerned
about the cost of this loop because in a system where the offload device
can handle a large number of flows the OOR, if it occurs due to flow table
exhaustion or near-exhaustion on the offload device, means that there
are a lot of flows present on the device.  And it also implies that the
host may already be under stressed due to a) revalidating the large number
of offloaded flows and b) forwarding packets for flows that are not
offloaded due to the OOR condition.

So I while I think there may be conditions where this works quite well.
I can envisage conditions where it does not. But I'm happy for things
to proceed as is and for such scaling problems to be left open for
future work.


More information about the dev mailing list