From: Oleg Nesterov <oleg@redhat.com>
To: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: peterz@infradead.org, linux-kernel@vger.kernel.org,
josh@joshtriplett.org, rostedt@goodmis.org,
mathieu.desnoyers@efficios.com, jiangshanlai@gmail.com
Subject: Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c
Date: Mon, 22 Oct 2018 17:24:07 +0200 [thread overview]
Message-ID: <20181022152406.GA7257@redhat.com> (raw)
In-Reply-To: <20181022145241.GA7488@linux.ibm.com>
On 10/22, Paul E. McKenney wrote:
>
> The sync.c file has a number of calls to BUG_ON(), which panics the
> kernel, which is not a good
Agreed.
I added these BUG_ON's for documentation when I was prototyping this code,
perhaps we can simply remove them.
> @@ -125,12 +125,12 @@ void rcu_sync_enter(struct rcu_sync *rsp)
> rsp->gp_state = GP_PENDING;
> spin_unlock_irq(&rsp->rss_lock);
>
> - BUG_ON(need_wait && need_sync);
> -
> if (need_sync) {
> gp_ops[rsp->gp_type].sync();
> rsp->gp_state = GP_PASSED;
> wake_up_all(&rsp->gp_wait);
> + if (WARN_ON_ONCE(need_wait))
> + wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
This wait_event(gp_state == GP_PASSED) is pointless, note that this branch
does gp_state = GP_PASSED 2 lines above.
And if we add WARN_ON_ONCE(need_wait), then we should probably also add
WARN_ON_ONCE(need_sync) into the next "if (need_wait)" branch just for
symmetry.
So I'd suggest to either turn that BUG_ON(need_wait && need_sync) above
into WARN_ON_ONCE(wait && sync) or simply remove it.
Again, the only purpose of this BUG_ON() is to explain to the reader that
it is not (must not be) possible that, say, gp_state == GP_IDLE while
gp_count != 0.
----------------------------------------------------------------------------
Damn.
This suddenly reminds me that I rewrote this code completely, and you even
reviewed the new implementation and (iirc) acked it!
However, I failed to force myself to rewrite the comments, and that is why
I didn't send the "official" patch :/
May be some time...
Oleg.
next prev parent reply other threads:[~2018-10-22 15:24 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-22 14:52 [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c Paul E. McKenney
2018-10-22 15:24 ` Oleg Nesterov [this message]
2018-10-22 15:56 ` Paul E. McKenney
2018-10-22 16:14 ` Oleg Nesterov
2018-10-30 17:55 ` Paul E. McKenney
2018-10-31 17:26 ` Oleg Nesterov
2018-10-31 17:33 ` Paul E. McKenney
2018-11-01 14:12 ` Oleg Nesterov
2018-11-01 14:42 ` Paul E. McKenney
2018-11-01 15:45 ` Oleg Nesterov
2018-11-01 17:03 ` Paul E. McKenney
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=20181022152406.GA7257@redhat.com \
--to=oleg@redhat.com \
--cc=jiangshanlai@gmail.com \
--cc=josh@joshtriplett.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=paulmck@linux.ibm.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).