From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 3 Oct 2018 11:49:05 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v2 02/11] vinyl: factor load regulator out of quota Message-ID: <20181003084905.m5shae5tg6pglexs@esperanza> References: <20181002181656.GA12422@chai> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181002181656.GA12422@chai> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Tue, Oct 02, 2018 at 09:16:56PM +0300, Konstantin Osipov wrote: > * Vladimir Davydov [18/09/28 21:00]: > > Turned out that throttling isn't going to be as simple as maintaining > > the write rate below the estimated dump bandwidth, because we also need > > to take into account whether compaction keeps up with dumps. Tracking > > compaction progress isn't a trivial task and mixing it in a module > > responsible for resource limiting, which vy_quota is, doesn't seem to be > > a good idea. Let's factor out the related code into a separate module > > and call it vy_regulator. Currently, the new module only keeps track of > > the write rate and the dump bandwidth and sets the memory watermark > > accordingly, but soon we will extend it to configure throttling as well. > > OK, if we keep regulator in place there is a few comments about > the contents of the patch (i.e. the renames). > > > - struct vy_quota *q = &env->quota; > > + struct vy_regulator *r = &env->regulator; > > > > - info_table_begin(h, "quota"); > > - info_append_int(h, "used", q->used); > > - info_append_int(h, "limit", q->limit); > > - info_append_int(h, "watermark", q->watermark); > > - info_append_int(h, "use_rate", q->use_rate); > > - info_append_int(h, "dump_bandwidth", q->dump_bw); > > - info_table_end(h); /* quota */ > > + info_table_begin(h, "regulator"); > > + info_append_int(h, "watermark", r->watermark); > > + info_append_int(h, "write_rate", r->write_rate); > > + info_append_int(h, "dump_bandwidth", r->dump_bw); > > I think "watermark" should be now "dump_watermark", sicne simply > "watermark" is not specific enough. Is it regulator watermark? A > regulator watermark doesn't make any sense to me. > > The rename should take place across the entire code base imho. > > > assert(mem_used_after >= mem_used_before); > > vy_quota_adjust(&env->quota, tx->write_size, > > mem_used_after - mem_used_before); > > + vy_regulator_check_watermark(&env->regulator); > > Like check_dump_watermark. > > > vy_env_quota_exceeded_cb(struct vy_quota *quota) > > { > > struct vy_env *env = container_of(quota, struct vy_env, quota); > > + vy_regulator_no_memory(&env->regulator); > > IMHO vy_regulator_quota_exceeded is a better name. Makes sense to me. I also renamed dump_bw to dump_bandwidth, because we typically don't use abbreviations in member names, and used_last to quota_used_last and pushed it to 1.10.