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

[Konstantin Osipov <kostja@tarantool.org>]

* 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.
> 
> 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.

> > 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.

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.

> 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
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.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov