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
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
