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

[Paul E. McKenney]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/