Re: [PATCH v2 11/11] vinyl: introduce quota consumer priorities

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Thu, Oct 11, 2018 at 10:02:50AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/10/08 14:12]:
> > 
> > Actually, there are resource types and there are consumer types.
> > I admit the fact that I mixed them may look confusing at the first
> > glance. We may introduce a seprate enum for resource types with a
> > mapping between them.
> > 
> > enum vy_quota_consumer_type {
> > 	VY_QUOTA_CONSUMER_TX = 0,
> > 	VY_QUOTA_CONSUMER_COMPACTION = 1,
> > 	vy_quota_consumer_max,
> > };
> > 
> > enum vy_quota_resource_type {
> > 	VY_QUOTA_RESOURCE_DISK = 0,
> > 	VY_QUOTA_RESOURCE_MEMORY = 1,
> > 	vy_quota_resource_max,
> > };
> > 
> > static unsigned vy_quota_consumer_mask[] = {
> > 	[VY_QUOTA_CONSUMER_TX] = (1 << VY_QUOTA_RESOURCE_DISK) |
> > 				 (1 << VY_QUOTA_RESOURCE_MEMORY),
> > 	[VY_QUOTA_CONSUMER_COMPACTION] = (1 << VY_QUOTA_RESOURCE_MEMORY),
> > };
> > 
> > struct vy_quota {
> > 	...
> > 	struct rlist wait_queue[vy_quota_consumer_max];
> > 	struct vy_rate_limit rate_limit[vy_quota_resource_max];
> > };
> 
> 
> > 
> > This would make the code more bulky though. Do we really want to
> > complicate?
> 
> I like it.

Then please see the patch:

https://www.freelists.org/post/tarantool-patches/PATCH-v2-1111-vinyl-introduce-quota-consumer-priorities,3

> > 
> > Also, quite frankly I don't quite dig the concept of 'resources' and
> > the corresponding constant names, because 'memory' rate limit may be
> > confused with memory usage, which is what vy_quota is about in the first
> > place.
> 
> Yes, vy_quota is originally about usage. I don't know why you
> insist on having rate limits in it ;) I'm ok if we put rate limits
> into a separate object, now that we have vy_regulator it can
> manage all of them.

Because I want to reuse the wait queue which was initially introduced
for putting consumers to sleep when they hit the memory limit.

Besides, rate limit value is consumed only along with quota so I'm
convinced they should be together.

> 
> > > Quota refill interval should be varying - please schedule this
> > > work in a separate patch.
> > > 
> > > 
> > > >  static void
> > > >  vy_quota_signal(struct vy_quota *q)
> > > >  {
> > > > -	if (!rlist_empty(&q->wait_queue)) {
> > > > +	/*
> > > > +	 * Wake up a consumer that has waited most no matter
> > > > +	 * whether it's high or low priority. This assures that
> > > > +	 * high priority consumers don't uncontrollably throttle
> > > > +	 * low priority ones.
> > > > +	 */
> > > > +	struct vy_quota_wait_node *oldest = NULL;
> > > > +	for (int i = 0; i < vy_quota_consumer_prio_MAX; i++) {
> > > > +		struct rlist *wq = &q->wait_queue[i];
> > > > +		if (rlist_empty(wq))
> > > > +			continue;
> > > 
> > > I still do not understand why you need a second queue and
> > > timestapms. If you need to ensure total fairness, a single queue
> > > should be enough. 
> > 
> > I need to maintain one queue per consumer type, because it's possible
> > that we may wake up consumers of one type, but not of the other.
> 
> If you do this, your fairness is strictly speaking broken, unless,
> then again, a type implies some kind of priority. By waking up a
> consumer out of (strict) order you give away a resource which may
> be needed for another consumer which (in strict single-queue
> order) is ahead of it.

I only wake up consumers out of (strict) order in case some consumers
can't proceed in the *current* interval, i.e. I only give the *surplus*
of the resource that wouldn't be consumed anyway in *this* interval.
Note, the resource is replenished on a timely basis so by dispensing the
surplus I don't deprive throttled consumers of any resource.

> 
> Frankly I still don't see why you did it this way except for the
> personal preference.
> 
> > If we used a single queue, it could occur that consumers that
> > could be woken up landed in the middle of the queue so that
> > without scanning the queue we wouldn't be able to find them.
> > Scanning a queue looks ugly.
> 
> Don't scan the queue. If you have no resources for the first
> consumer in the queue, everyone else has to wait. Make it simple
> and stupid.

Then compaction threads could occasionally be throttled by transactions
that stalled due to the disk-based rate limit.

> 
> > Think of the multi-queue design like this: there's one queue per each
> > consumer type. When a resource is replenished, we check only those
> > queues that store consumers that may proceed and choose the one that has
> > waited most.
> 
> How is that possible that some consumer can not proceed? The

Transactions waiting for disk-based quota since the last interval may
consume all quota that was added for the current interval and get
throttled. Then we wouldn't be able to wake up compaction threads that
were queued after them.

Even though you might argue that this is unlikely and this should never
happen once the load is stabilized, ignoring such a possibility makes me
feel uncomfortable. Using one wait queue per consumer with ticketing for
fairness is a simple and clear solution that assures this can't happen.
It's clear both to me and Kirill. I fail to see why you find it
difficult for understanding. You haven't given a single technical
argument against it. You just seem to not like it for some reason,
that's all...

> replenishment is instantaneous for all kinds of resources. So if
> there is at all a chance that the first (strictly first) consumer
> has insufficient resources to proceed, it means it requires more
> than is at all possible within this rate limit interval. It's
> better to prohibit this altogether.