On Tue, Aug 02, 2022 at 10:04:10PM +0800, Yu Kuai wrote:
...
> +static void __tg_update_skipped(struct throtl_grp *tg, bool rw)
> +{
> + unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
> + u64 bps_limit = tg_bps_limit(tg, rw);
> + u32 iops_limit = tg_iops_limit(tg, rw);
> +
> + /*
> + * If config is updated while bios are still throttled, calculate and
> + * accumulate how many bytes/io are waited across changes. And
> + * bytes/io_skipped will be used to calculate new wait time under new
> + * configuration.
> + *
> + * Following calculation won't overflow as long as bios that are
> + * dispatched later won't preempt already throttled bios. Even if such
> + * overflow do happen, there should be no problem because unsigned is
> + * used here, and bytes_skipped/io_skipped will be updated correctly.
> + */
Would it be easier if the fields were signed? It's fragile and odd to
explain "these are unsigned but if they underflow they behave just like
signed when added" when they can just be signed. Also, I have a hard time
understand what "preempt" means above.
> + if (bps_limit != U64_MAX)
> + tg->bytes_skipped[rw] +=
> + calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
> + tg->bytes_disp[rw];
> + if (iops_limit != UINT_MAX)
> + tg->io_skipped[rw] +=
> + calculate_io_allowed(iops_limit, jiffy_elapsed) -
> + tg->io_disp[rw];
So, this is calculating the budgets to carry over. Can we name them
accordingly? I don't know what "skipped" means.
> @@ -115,6 +115,17 @@ struct throtl_grp {
> uint64_t bytes_disp[2];
> /* Number of bio's dispatched in current slice */
> unsigned int io_disp[2];
> + /*
> + * The following two fields are updated when new configuration is
> + * submitted while some bios are still throttled, they record how many
> + * bytes/io are waited already in previous configuration, and they will
> + * be used to calculate wait time under new configuration.
> + *
> + * Number of bytes will be skipped in current slice
> + */
> + uint64_t bytes_skipped[2];
> + /* Number of bio will be skipped in current slice */
> + unsigned int io_skipped[2];
So, the code seems to make sense but the field names and comments don't
really, at least to me. I can't find an intuitive understanding of what's
being skipped. Can you please take another stab at making this more
understandable?
Thanks.
--
tejun
