Commit 04a6b516 authored by Vivek Goyal's avatar Vivek Goyal Committed by Jens Axboe

blk-throttle: Correct the placement of smp_rmb()

o I was discussing what are the variable being updated without spin lock and
  why do we need barriers and Oleg pointed out that location of smp_rmb()
  should be between read of td->limits_changed and tg->limits_changed. This
  patch fixes it.

o Following is one possible sequence of events. Say cpu0 is executing
  throtl_update_blkio_group_read_bps() and cpu1 is executing
  throtl_process_limit_change().

 cpu0                                                cpu1

 tg->limits_changed = true;
 smp_mb__before_atomic_inc();
 atomic_inc(&td->limits_changed);

                                     if (!atomic_read(&td->limits_changed))
                                             return;

                                     if (tg->limits_changed)
                                             do_something;

 If cpu0 has updated tg->limits_changed and td->limits_changed, we want to
 make sure that if update to td->limits_changed is visible on cpu1, then
 update to tg->limits_changed should also be visible.

 Oleg pointed out to ensure that we need to insert an smp_rmb() between
 td->limits_changed read and tg->limits_changed read.

o I had erroneously put smp_rmb() before atomic_read(&td->limits_changed).
  This patch fixes it.
Reported-by: default avatarOleg Nesterov <oleg@redhat.com>
Signed-off-by: default avatarVivek Goyal <vgoyal@redhat.com>
Signed-off-by: default avatarJens Axboe <jaxboe@fusionio.com>
parent d1ae8ffd
...@@ -725,26 +725,21 @@ static void throtl_process_limit_change(struct throtl_data *td) ...@@ -725,26 +725,21 @@ static void throtl_process_limit_change(struct throtl_data *td)
struct throtl_grp *tg; struct throtl_grp *tg;
struct hlist_node *pos, *n; struct hlist_node *pos, *n;
/*
* Make sure atomic_inc() effects from
* throtl_update_blkio_group_read_bps(), group of functions are
* visible.
* Is this required or smp_mb__after_atomic_inc() was suffcient
* after the atomic_inc().
*/
smp_rmb();
if (!atomic_read(&td->limits_changed)) if (!atomic_read(&td->limits_changed))
return; return;
throtl_log(td, "limit changed =%d", atomic_read(&td->limits_changed)); throtl_log(td, "limit changed =%d", atomic_read(&td->limits_changed));
hlist_for_each_entry_safe(tg, pos, n, &td->tg_list, tg_node) { /*
/* * Make sure updates from throtl_update_blkio_group_read_bps() group
* Do I need an smp_rmb() here to make sure tg->limits_changed * of functions to tg->limits_changed are visible. We do not
* update is visible. I am relying on smp_rmb() at the * want update td->limits_changed to be visible but update to
* beginning of function and not putting a new one here. * tg->limits_changed not being visible yet on this cpu. Hence
*/ * the read barrier.
*/
smp_rmb();
hlist_for_each_entry_safe(tg, pos, n, &td->tg_list, tg_node) {
if (throtl_tg_on_rr(tg) && tg->limits_changed) { if (throtl_tg_on_rr(tg) && tg->limits_changed) {
throtl_log_tg(td, tg, "limit change rbps=%llu wbps=%llu" throtl_log_tg(td, tg, "limit change rbps=%llu wbps=%llu"
" riops=%u wiops=%u", tg->bps[READ], " riops=%u wiops=%u", tg->bps[READ],
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment