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

[Andrea Righi <righi.andrea@gmail.com>]

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
...

> > > > +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).

-Andrea