LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Tejun Heo <tj@kernel.org>
To: Josef Bacik <josef@toxicpanda.com>
Cc: Dennis Zhou <dennisszhou@gmail.com>, Jens Axboe <axboe@kernel.dk>,
	Johannes Weiner <hannes@cmpxchg.org>,
	kernel-team@fb.com, linux-block@vger.kernel.org,
	cgroups@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 04/15] blkcg: fix ref count issue with bio_blkcg using task_css
Date: Fri, 31 Aug 2018 16:04:20 -0700
Message-ID: <20180831230420.GB1488037@devbig004.ftw2.facebook.com> (raw)
In-Reply-To: <20180831153538.brzgcm3rgmwfy3rg@destiny>

Hello,

On Fri, Aug 31, 2018 at 11:35:39AM -0400, Josef Bacik wrote:
> > +static inline struct cgroup_subsys_state *blkcg_get_css(void)
> > +{
> > +	struct cgroup_subsys_state *css;
> > +
> > +	rcu_read_lock();
> > +
> > +	css = kthread_blkcg();
> > +	if (css) {
> > +		css_get(css);
> > +	} else {
> > +		while (true) {
> > +			css = task_css(current, io_cgrp_id);
> > +			if (likely(css_tryget(css)))
> > +				break;
> > +			cpu_relax();
> 
> Does this work?  I'm ignorant of what cpu_relax() does, but it seems if we're
> rcu_read_lock()'ed here we aren't going to queisce so if we fail to get the css
> here we just simply aren't going to get it unless we go to sleep right?  An
> honest question, because this is all magic to me, I'd like to understand how
> this isn't going to infinite loop on us if css_tryget(css) fails.

The only time css_tryget() on task_css(current, xxx) can fail is if it
races against the current thread migrating away from that cgroup and
that cgroup is now getting destroyed.  IOW,

1. For css_tryget() to fail, the cgroup must be dying.
2. The cgroup must be empty for it to be dying.
3. current must have already been migrated away to a different cgroup.

So, the above happens only when racing against css_set_move_task() -
it's seeing the old css pointer.  As the membership pointer switching
must already have happened, all it's waiting for is the new css
membership pointer to be propagated on the polling cpu, making
cpu_relax() busy loop the right thing to do.

This pattern is also used in task_get_css() and cgroup_sk_alloc().
Given that it's a bit tricky, it probably would be worthwhile to
factor out and document it.

Once Josef's other concerns are addressed, please feel free to add

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

  reply index

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-31  1:53 [PATCH 00/15] blkcg ref count refactor/cleanup + blkcg avg_lat Dennis Zhou
2018-08-31  1:53 ` [PATCH 01/15] Revert "blk-throttle: fix race between blkcg_bio_issue_check() and cgroup_rmdir()" Dennis Zhou
2018-08-31  1:53 ` [PATCH 02/15] blkcg: delay blkg destruction until after writeback has finished Dennis Zhou
2018-08-31 15:27   ` Josef Bacik
2018-08-31 20:19     ` Dennis Zhou
2018-08-31  1:53 ` [PATCH 03/15] blkcg: use tryget logic when associating a blkg with a bio Dennis Zhou
2018-08-31 15:30   ` Josef Bacik
2018-08-31 20:20     ` Dennis Zhou
2018-08-31  1:53 ` [PATCH 04/15] blkcg: fix ref count issue with bio_blkcg using task_css Dennis Zhou
2018-08-31 15:35   ` Josef Bacik
2018-08-31 23:04     ` Tejun Heo [this message]
2018-09-06 15:21     ` Dennis Zhou
2018-08-31  1:53 ` [PATCH 05/15] blkcg: update blkg_lookup_create to do locking Dennis Zhou
2018-08-31 15:37   ` Josef Bacik
2018-08-31 23:09   ` Tejun Heo
2018-08-31  1:53 ` [PATCH 06/15] blkcg: always associate a bio with a blkg Dennis Zhou
2018-08-31  9:01   ` kbuild test robot
2018-08-31 10:02   ` kbuild test robot
2018-08-31 23:16   ` Tejun Heo
2018-09-06 20:41     ` Dennis Zhou
2018-09-07  3:03   ` [LKP] [blkcg] c02c58dab2: WARNING:at_block/blk-throttle.c:#blk_throtl_bio kernel test robot
2018-08-31  1:53 ` [PATCH 07/15] blkcg: consolidate bio_issue_init and blkg association Dennis Zhou
2018-08-31  9:19   ` kbuild test robot
2018-08-31 11:11   ` kbuild test robot
2018-08-31 15:42   ` Josef Bacik
2018-09-06 20:43     ` Dennis Zhou
2018-08-31 23:45   ` Tejun Heo
2018-08-31  1:53 ` [PATCH 08/15] blkcg: associate a blkg for pages being evicted by swap Dennis Zhou
2018-08-31 15:44   ` Josef Bacik
2018-08-31 23:47   ` Tejun Heo
2018-08-31  1:53 ` [PATCH 09/15] blkcg: associate writeback bios with a blkg Dennis Zhou
2018-08-31 15:45   ` Josef Bacik
2018-08-31 23:53   ` Tejun Heo
2018-08-31  1:53 ` [PATCH 10/15] blkcg: remove bio->bi_css and instead use bio->bi_blkg Dennis Zhou
2018-08-31 15:46   ` Josef Bacik
2018-09-01  0:13   ` Tejun Heo
2018-08-31  1:53 ` [PATCH 11/15] blkcg: remove additional reference to the css Dennis Zhou
2018-09-01  0:26   ` Tejun Heo
2018-09-06 20:45     ` Dennis Zhou
2018-08-31  1:53 ` [PATCH 12/15] blkcg: cleanup and make blk_get_rl use blkg_lookup_create Dennis Zhou
2018-09-01  0:29   ` Tejun Heo
2018-09-11  2:37   ` [LKP] [blkcg] 22f657e287: general_protection_fault:#[##] kernel test robot
2018-08-31  1:53 ` [PATCH 13/15] blkcg: change blkg reference counting to use percpu_ref Dennis Zhou
2018-08-31 15:49   ` Josef Bacik
2018-09-01  0:31   ` Tejun Heo
2018-09-06 20:46     ` Dennis Zhou
2018-09-07  3:08   ` [LKP] [blkcg] 6ef69a3a0b: WARNING:suspicious_RCU_usage kernel test robot
2018-08-31  1:53 ` [PATCH 14/15] blkcg: rename blkg_try_get to blkg_tryget Dennis Zhou
2018-08-31 15:50   ` Josef Bacik
2018-09-01  0:32   ` Tejun Heo
2018-08-31  1:53 ` [PATCH 15/15] blkcg: add average latency tracking to blk-cgroup Dennis Zhou
2018-08-31 10:22   ` kbuild test robot
2018-08-31 11:38   ` kbuild test robot
2018-09-01  0:35 ` [PATCH 00/15] blkcg ref count refactor/cleanup + blkcg avg_lat Tejun Heo

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=20180831230420.GB1488037@devbig004.ftw2.facebook.com \
    --to=tj@kernel.org \
    --cc=axboe@kernel.dk \
    --cc=cgroups@vger.kernel.org \
    --cc=dennisszhou@gmail.com \
    --cc=hannes@cmpxchg.org \
    --cc=josef@toxicpanda.com \
    --cc=kernel-team@fb.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git