All of lore.kernel.org
 help / color / mirror / Atom feed
From: Liqueur Librazy <im@librazy.org>
To: zhangyoufu@gmail.com
Cc: axboe@kernel.dk, cgroups@vger.kernel.org,
	linux-block@vger.kernel.org, tj@kernel.org
Subject: Re: [BUG] blk-throttle panic on 32bit machine after startup
Date: Mon, 18 Oct 2021 23:22:27 +0800	[thread overview]
Message-ID: <e726813b-4f6d-3a5d-6adb-8407a9b4c7f7@librazy.org> (raw)
In-Reply-To: <CAEKhA2x1Qi3Ywaj9fzdsaChabqDSMe2m2441wReg_V=39_Cuhg@mail.gmail.com>

Hi,

Yet another colleague of the reporter here. I found that some 
precondition maybe not sound when tg->slice_end[rw] is initialized with 
0, which time_before(INITIAL_JIFFIES, 0) holds true in 32-bit Linux.

As in v5.15-rc6/block/blk-throttle.c

1. L833

	/* Determine if previously allocated or extended slice is complete or 
not */
	static bool throtl_slice_used(struct throtl_grp *tg, bool rw)
	{
		if (time_in_range(jiffies, tg->slice_start[rw], tg->slice_end[rw]))
			return false;

		return true;
	}

throtl_slice_used will always return true for a newly initialized slice.
This may be intentional behavior but not mentioned in comment.
(except when jiffies == 0, which is another topic: will 
time_in_range_open do better here?)

2. L791, in throtl_start_new_slice_with_credit

	/*
	 * Previous slice has expired. We must have trimmed it after last
	 * bio dispatch. That means since start of last slice, we never used
	 * that bandwidth. Do try to make use of that bandwidth while giving
	 * credit.
	 */
	if (time_after_eq(start, tg->slice_start[rw]))
		tg->slice_start[rw] = start;

As mentioned in my colleague Haoran Luo's reply, time_after_eq(start, 
tg->slice_start[rw]) is falsy when the jiffies had not wrapped around.
A easy solution is to add a check for tg->slice_start[rw] == 0, or we 
should initialize tg->slice_start[rw] and tg->slice_end[rw] with 
INITIAL_JIFFIES.

WARNING: multiple messages have this Message-ID (diff)
From: Liqueur Librazy <im-WO6o3BnTAZpAfugRpC6u6w@public.gmane.org>
To: zhangyoufu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Cc: axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org,
	cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-block-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Subject: Re: [BUG] blk-throttle panic on 32bit machine after startup
Date: Mon, 18 Oct 2021 23:22:27 +0800	[thread overview]
Message-ID: <e726813b-4f6d-3a5d-6adb-8407a9b4c7f7@librazy.org> (raw)
In-Reply-To: <CAEKhA2x1Qi3Ywaj9fzdsaChabqDSMe2m2441wReg_V=39_Cuhg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

Hi,

Yet another colleague of the reporter here. I found that some 
precondition maybe not sound when tg->slice_end[rw] is initialized with 
0, which time_before(INITIAL_JIFFIES, 0) holds true in 32-bit Linux.

As in v5.15-rc6/block/blk-throttle.c

1. L833

	/* Determine if previously allocated or extended slice is complete or 
not */
	static bool throtl_slice_used(struct throtl_grp *tg, bool rw)
	{
		if (time_in_range(jiffies, tg->slice_start[rw], tg->slice_end[rw]))
			return false;

		return true;
	}

throtl_slice_used will always return true for a newly initialized slice.
This may be intentional behavior but not mentioned in comment.
(except when jiffies == 0, which is another topic: will 
time_in_range_open do better here?)

2. L791, in throtl_start_new_slice_with_credit

	/*
	 * Previous slice has expired. We must have trimmed it after last
	 * bio dispatch. That means since start of last slice, we never used
	 * that bandwidth. Do try to make use of that bandwidth while giving
	 * credit.
	 */
	if (time_after_eq(start, tg->slice_start[rw]))
		tg->slice_start[rw] = start;

As mentioned in my colleague Haoran Luo's reply, time_after_eq(start, 
tg->slice_start[rw]) is falsy when the jiffies had not wrapped around.
A easy solution is to add a check for tg->slice_start[rw] == 0, or we 
should initialize tg->slice_start[rw] and tg->slice_end[rw] with 
INITIAL_JIFFIES.

  reply	other threads:[~2021-10-18 15:38 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-18  7:08 [BUG] blk-throttle panic on 32bit machine after startup Youfu Zhang
2021-10-18  7:08 ` Youfu Zhang
2021-10-18 15:22 ` Liqueur Librazy [this message]
2021-10-18 15:22   ` Liqueur Librazy
2021-10-19 17:45 ` Tejun Heo
2021-10-19 17:45   ` Tejun Heo
2021-10-21  4:26   ` Youfu Zhang
2021-10-18  8:00 Haoran Luo
2021-10-18  8:00 ` Haoran Luo
2021-10-18  9:25 Haoran Luo
2021-10-18  9:25 ` Haoran Luo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e726813b-4f6d-3a5d-6adb-8407a9b4c7f7@librazy.org \
    --to=im@librazy.org \
    --cc=axboe@kernel.dk \
    --cc=cgroups@vger.kernel.org \
    --cc=linux-block@vger.kernel.org \
    --cc=tj@kernel.org \
    --cc=zhangyoufu@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.