linux-kernel - Re: [PATCH 4/7] io-throttle controller infrastructure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 21 Apr 2009 07:03:23 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Andrea Righi <righi.andrea@...il.com>
Cc:	Paul Menage <menage@...gle.com>,
	Balbir Singh <balbir@...ux.vnet.ibm.com>,
	Gui Jianfeng <guijianfeng@...fujitsu.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	agk@...rceware.org, akpm@...ux-foundation.org, axboe@...nel.dk,
	baramsori72@...il.com, Carl Henrik Lunde <chlunde@...g.uio.no>,
	dave@...ux.vnet.ibm.com, Divyesh Shah <dpshah@...gle.com>,
	eric.rannaud@...il.com, fernando@....ntt.co.jp,
	Hirokazu Takahashi <taka@...inux.co.jp>,
	Li Zefan <lizf@...fujitsu.com>, matt@...ehost.com,
	dradford@...ehost.com, ngupta@...gle.com, randy.dunlap@...cle.com,
	roberto@...it.it, Ryo Tsuruta <ryov@...inux.co.jp>,
	Satoshi UCHIDA <s-uchida@...jp.nec.com>,
	subrata@...ux.vnet.ibm.com, yoshikawa.takuya@....ntt.co.jp,
	Nauman Rafique <nauman@...gle.com>, fchecconi@...il.com,
	paolo.valente@...more.it, containers@...ts.linux-foundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] io-throttle controller infrastructure

On Tue, Apr 21, 2009 at 02:58:30PM +0200, Andrea Righi wrote:
> On Mon, Apr 20, 2009 at 09:15:24PM -0700, Paul E. McKenney wrote:
> > > > How does the above lock relate to the iot->lock called out in the comment
> > > > headers in the earlier functions?  Hmmm...  Come to think of it, I don't
> > > > see an acquisition of iot->lock anywhere.
> > > > 
> > > > So, what is the story here?
> > > 
> > > As said before, only the comment in struct iothrottle is correct, we use
> > > cgroup_lock() to protect iot->list, so there's no need to introduce
> > > another lock inside struct iothrottle.
> > > 
> > > And the other comments about iot->lock must be fixed.
> > 
> > Sounds good!
> > 
> > So this code is compiled into the kernel only when cgroups are defined,
> > correct?  Otherwise, cgroup_lock() seems to be an empty function.
> 
> Right, from init/Kconfig:
> 
> config CGROUP_IO_THROTTLE
>         bool "Enable cgroup I/O throttling"
>         depends on CGROUPS && RESOURCE_COUNTERS && EXPERIMENTAL
> ...

Fair enough!

> > > > > +int iothrottle_set_page_owner(struct page *page, struct mm_struct *mm)
> > > > > +{
> > > > > +	struct iothrottle *iot;
> > > > > +	unsigned short id = 0;
> > > > > +
> > > > > +	if (iothrottle_disabled())
> > > > > +		return 0;
> > > > > +	if (!mm)
> > > > > +		goto out;
> > > > > +	rcu_read_lock();
> > > > > +	iot = task_to_iothrottle(rcu_dereference(mm->owner));
> > > > 
> > > > Given that task_to_iothrottle() calls task_subsys_state(), which contains
> > > > an rcu_dereference(), why is the rcu_dereference() above required?
> > > > (There might well be a good reason, just cannot see it right offhand.)
> > > 
> > > The first rcu_dereference() is required to safely get a task_struct from
> > > mm_struct. The second rcu_dereference() inside task_to_iothrottle() is
> > > required to safely get the struct iothrottle from task_struct.
> > 
> > Why not put the rcu_dereference() down inside task_to_iothrottle()?
> 
> mmmh... it is needed only when task_struct is taken from mm->owner,
> task_to_iothrottle(current) for example works fine without the
> rcu_dereference(current).

OK...  But please note that rcu_dereference() is extremely lightweight,
a couple orders of magnitude cheaper than an uncontended lock.  So there
is almost no penalty for using it on the task_to_iothrottle() path.

							Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/