Re: [PATCH RFC net-next] openvswitch: Queue upcalls to userspace in per-port round-robin order

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 3 Aug 2018 16:01:08 -0700
Ben Pfaff <blp@ovn.org> wrote:

> I think that a simple mechanism for fairness is fine.  The direction
> of extensibility that makes me anxious is how to decide what matters
> for fairness.  So far, we've talked about per-vport fairness.  That
> works pretty well for packets coming in from virtual interfaces where
> each vport represents a separate VM.

Yes, right, that's the case where we have significant issues currently.

> It does not work well if the traffic filling your queues all comes
> from a single physical port because some source of traffic is sending
> traffic at a high rate.  In that case, you'll do a lot better if you
> do fairness based on the source 5-tuple. But if you're doing network
> virtualization, then the outer source 5-tuples won't necessarily vary
> much and you'd be better off looking at the VNI and maybe some Geneve
> TLV options and maybe the inner 5-tuple...

Sure, I see what you mean now. That looks entirely doable if we
abstract the round-robin bucket selection out of the current patch.

> I would be very pleased if we could integrate a simple mechanism for
> fairness, based for now on some simple criteria like the source port,
> but thinking ahead to how we could later make it gracefully extensible
> to consider more general and possibly customizable criteria.

We could change the patch so that instead of just using the vport for
round-robin queue insertion, we generalise that and use "buckets"
instead of vports, and have a set of possible functions that are called
instead of using port_no directly in ovs_dp_upcall_queue_roundrobin(),
making this configurable via netlink, per datapath.

We could implement selection based on source port or a hash on the
source 5-tuple, and the relevant bits of
ovs_dp_upcall_queue_roundrobin() would look like this:

static int ovs_dp_upcall_queue_roundrobin(struct datapath *dp,
					  struct dp_upcall_info *upcall)
{

[...]

	list_for_each_entry(pos, head, list) {
		int bucket = dp->rr_select(pos);

		/* Count per-bucket upcalls. */
		if (dp->upcalls.count[bucket] == U8_MAX) {
			err = -ENOSPC;
			goto out_clear;
		}
		dp->upcalls.count[bucket]++;

		if (bucket == upcall->bucket) {
			/* Another upcall for the same bucket: move insertion
			 * point here, keep looking for insertion condition to
			 * be still met further on.
			 */
			find_next = true;
			here = pos;
			continue;
		}

		count = dp->upcalls.count[bucket];
		if (find_next && dp->upcalls.count[bucket] >= count) {
			/* Insertion condition met: no need to look further,
			 * unless another upcall for the same port occurs later.
			 */
			find_next = false;
			here = pos;
		}
	}

[...]

}

and implementations for dp->rr_select() would look like:

int rr_select_vport(struct dp_upcall_info *upcall)
{
	return upcall->port_no;
}

int rr_select_srcport(struct dp_upcall_info *upcall)
{
	/* look up source port from upcall->skb... */
}

And we could then easily extend this to use BPF with maps one day.

This is for clarity by the way, but I guess we should avoid indirect
calls in the final implementation. 

What do you think?

-- 
Stefano