RE: [PATCH 24/32] Task fork and exit for rdtgroup
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Wednesday, July 13, 2016 2:03 PM
> On Wed, 13 Jul 2016, Yu, Fenghua wrote:
> > On Wed, July 2016, Thomas Gleixner wrote
> > > On Tue, 12 Jul 2016, Fenghua Yu wrote:
> > > > +void rdtgroup_post_fork(struct task_struct *child) {
> > > > + if (!use_rdtgroup_tasks)
> > > > + return;
> > > > +
> > > > + spin_lock_irq(_task_lock);
> > > > + if (list_empty(>rg_list)) {
> > >
> > > Why would the list be non empty after a fork?
> >
> > In this situation for a pid:
> > 1.rdtgroup_fork(): rg_list=null.
> > 2.setup_task_rg_lists(): rg_list is setup
> > 3.rdtgroup_fork(): rg_list is not empty
>
> Why would rdtgroup_fork() be called twice for a given thread?
>
> > This situation happens only during rscctrl mount time. Before mount,
> > post_fork() returns from !use_rdtgroup_tasks and doesn't set up
> > rg_list. After mount, rg_list() is always empty in post_fork(). But we need
> to check rg_list for above situation.
> >
> > Does that make sense?
>
> No, that does not make any sense at all.
>
> > Any suggestion for better soluation?
>
> The problem you have is:
>
> fork
> list_init(rg_list);
> write_lock(tasklist_lock);
>
> task becomes visible
>
> write_unlock(tasklist_lock);
>
> rdtgroup_post_fork();
> if (!use_rdtgroup_tasks)
> return;
>
> spin_lock_irq(_task_lock);
> list_add();
> spin_unlock_irq(_task_lock);
>
> I have no idea why this lock must be taken with _irq, but thats another story.
> Let's look at the mount side:
>
>spin_lock_irq(_task_lock);
>read_lock(_lock);
>
>do_each_thread(g, p) {
> WARN_ON(More magic crap happening there)
>
> spin_lock_irq(>sighand->siglock);
> list_add();
> spin_unlock_irq(>sighand->siglock);
>
> Great: You undo the irq disable of (_task_lock) above! Oh well
>
>read_unlock(_lock);
>spin_unlock_irq(_task_lock);
>
> So you need all this magic in rdtgroup_post_fork() and setup_task_rg_lists()
> just because you blindly positioned rdtgroup_post_fork() at the point where
> the cgroup_post_fork() stuff is. But you did not think a second about the
> locking rules here otherwise they would be documented somewhere.
>
> You need a read_lock(_lock) for the mount part anyway. So why
> don't you do the obvious:
>
> fork
> list_init(rg_list);
> write_lock(tasklist_lock);
>
> rdtgroup_post_fork();
> if (use_rdtgroup_tasks)
> spin_lock(_task_lock);
> list_add();
> spin_unlock(_task_lock);
>
> task becomes visible
>
> write_unlock(tasklist_lock);
>
> And reorder the lock ordering in the mount path:
>
>read_lock(_lock);
>spin_lock(_task_lock);
>
> Now using rdtgroup_task_lock to protect current->rdtgroup is horrible as
> well. You need task->sighand->siglock in the mount path anyway to prevent
> exit races. So you can simplify the whole magic to:
>
> fork
> list_init(rg_list);
> write_lock(tasklist_lock);
>
> spin_lock(>sighand->siglock);
>
> rdtgroup_post_fork();
> if (use_rdtgroup_tasks)
> list_add();
>
> spin_unlock(>sighand->siglock);
> write_unlock(tasklist_lock);
>
> That removes an extra lock/unlock operation from the fork path because
> current->sighand->siglock is taken inside of the tasklist_lock write
> current->sighand->locked
> section already.
>
> So you need protection for use_rdtgroup_task, which is a complete
> misnomer btw. (rdtgroup_active would be too obvious, right?). That
> protection is simple because you can set that flag with tasklist_lock read
> locked which you hold anyway for iterating all threads in the mount path.
>
> Aside of that you need to take tsk->sighand->siglock when you change
> tsk->rdtgroup, but that's a no-brainer and it gives you the extra
> tsk->benefit that
> you can protect such an operation against exit of the task that way by
> checking PF_EXITING under the lock. I don't see any protection against exit in
> your current implementation when a task is moved to a different partition.
>
> Please sit down and describe the complete locking and protection scheme of
> this stuff. I'm not going to figure this out from the obscure code another
> time.
>
> Thanks,
>
> tglx
Sure, I'll rethink of the locking and protection scheme for the tasks.
Thanks.
-Fenghua
RE: [PATCH 24/32] Task fork and exit for rdtgroup
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Wednesday, July 13, 2016 2:03 PM
> On Wed, 13 Jul 2016, Yu, Fenghua wrote:
> > On Wed, July 2016, Thomas Gleixner wrote
> > > On Tue, 12 Jul 2016, Fenghua Yu wrote:
> > > > +void rdtgroup_post_fork(struct task_struct *child) {
> > > > + if (!use_rdtgroup_tasks)
> > > > + return;
> > > > +
> > > > + spin_lock_irq(_task_lock);
> > > > + if (list_empty(>rg_list)) {
> > >
> > > Why would the list be non empty after a fork?
> >
> > In this situation for a pid:
> > 1.rdtgroup_fork(): rg_list=null.
> > 2.setup_task_rg_lists(): rg_list is setup
> > 3.rdtgroup_fork(): rg_list is not empty
>
> Why would rdtgroup_fork() be called twice for a given thread?
>
> > This situation happens only during rscctrl mount time. Before mount,
> > post_fork() returns from !use_rdtgroup_tasks and doesn't set up
> > rg_list. After mount, rg_list() is always empty in post_fork(). But we need
> to check rg_list for above situation.
> >
> > Does that make sense?
>
> No, that does not make any sense at all.
>
> > Any suggestion for better soluation?
>
> The problem you have is:
>
> fork
> list_init(rg_list);
> write_lock(tasklist_lock);
>
> task becomes visible
>
> write_unlock(tasklist_lock);
>
> rdtgroup_post_fork();
> if (!use_rdtgroup_tasks)
> return;
>
> spin_lock_irq(_task_lock);
> list_add();
> spin_unlock_irq(_task_lock);
>
> I have no idea why this lock must be taken with _irq, but thats another story.
> Let's look at the mount side:
>
>spin_lock_irq(_task_lock);
>read_lock(_lock);
>
>do_each_thread(g, p) {
> WARN_ON(More magic crap happening there)
>
> spin_lock_irq(>sighand->siglock);
> list_add();
> spin_unlock_irq(>sighand->siglock);
>
> Great: You undo the irq disable of (_task_lock) above! Oh well
>
>read_unlock(_lock);
>spin_unlock_irq(_task_lock);
>
> So you need all this magic in rdtgroup_post_fork() and setup_task_rg_lists()
> just because you blindly positioned rdtgroup_post_fork() at the point where
> the cgroup_post_fork() stuff is. But you did not think a second about the
> locking rules here otherwise they would be documented somewhere.
>
> You need a read_lock(_lock) for the mount part anyway. So why
> don't you do the obvious:
>
> fork
> list_init(rg_list);
> write_lock(tasklist_lock);
>
> rdtgroup_post_fork();
> if (use_rdtgroup_tasks)
> spin_lock(_task_lock);
> list_add();
> spin_unlock(_task_lock);
>
> task becomes visible
>
> write_unlock(tasklist_lock);
>
> And reorder the lock ordering in the mount path:
>
>read_lock(_lock);
>spin_lock(_task_lock);
>
> Now using rdtgroup_task_lock to protect current->rdtgroup is horrible as
> well. You need task->sighand->siglock in the mount path anyway to prevent
> exit races. So you can simplify the whole magic to:
>
> fork
> list_init(rg_list);
> write_lock(tasklist_lock);
>
> spin_lock(>sighand->siglock);
>
> rdtgroup_post_fork();
> if (use_rdtgroup_tasks)
> list_add();
>
> spin_unlock(>sighand->siglock);
> write_unlock(tasklist_lock);
>
> That removes an extra lock/unlock operation from the fork path because
> current->sighand->siglock is taken inside of the tasklist_lock write
> current->sighand->locked
> section already.
>
> So you need protection for use_rdtgroup_task, which is a complete
> misnomer btw. (rdtgroup_active would be too obvious, right?). That
> protection is simple because you can set that flag with tasklist_lock read
> locked which you hold anyway for iterating all threads in the mount path.
>
> Aside of that you need to take tsk->sighand->siglock when you change
> tsk->rdtgroup, but that's a no-brainer and it gives you the extra
> tsk->benefit that
> you can protect such an operation against exit of the task that way by
> checking PF_EXITING under the lock. I don't see any protection against exit in
> your current implementation when a task is moved to a different partition.
>
> Please sit down and describe the complete locking and protection scheme of
> this stuff. I'm not going to figure this out from the obscure code another
> time.
>
> Thanks,
>
> tglx
Sure, I'll rethink of the locking and protection scheme for the tasks.
Thanks.
-Fenghua
RE: [PATCH 24/32] Task fork and exit for rdtgroup
On Wed, 13 Jul 2016, Yu, Fenghua wrote:
> On Wed, July 2016, Thomas Gleixner wrote
> > On Tue, 12 Jul 2016, Fenghua Yu wrote:
> > > +void rdtgroup_post_fork(struct task_struct *child) {
> > > + if (!use_rdtgroup_tasks)
> > > + return;
> > > +
> > > + spin_lock_irq(_task_lock);
> > > + if (list_empty(>rg_list)) {
> >
> > Why would the list be non empty after a fork?
>
> In this situation for a pid:
> 1.rdtgroup_fork(): rg_list=null.
> 2.setup_task_rg_lists(): rg_list is setup
> 3.rdtgroup_fork(): rg_list is not empty
Why would rdtgroup_fork() be called twice for a given thread?
> This situation happens only during rscctrl mount time. Before mount,
> post_fork()
> returns from !use_rdtgroup_tasks and doesn't set up rg_list. After mount,
> rg_list()
> is always empty in post_fork(). But we need to check rg_list for above
> situation.
>
> Does that make sense?
No, that does not make any sense at all.
> Any suggestion for better soluation?
The problem you have is:
fork
list_init(rg_list);
write_lock(tasklist_lock);
task becomes visible
write_unlock(tasklist_lock);
rdtgroup_post_fork();
if (!use_rdtgroup_tasks)
return;
spin_lock_irq(_task_lock);
list_add();
spin_unlock_irq(_task_lock);
I have no idea why this lock must be taken with _irq, but thats another
story. Let's look at the mount side:
spin_lock_irq(_task_lock);
read_lock(_lock);
do_each_thread(g, p) {
WARN_ON(More magic crap happening there)
spin_lock_irq(>sighand->siglock);
list_add();
spin_unlock_irq(>sighand->siglock);
Great: You undo the irq disable of (_task_lock) above! Oh well
read_unlock(_lock);
spin_unlock_irq(_task_lock);
So you need all this magic in rdtgroup_post_fork() and setup_task_rg_lists()
just because you blindly positioned rdtgroup_post_fork() at the point where
the cgroup_post_fork() stuff is. But you did not think a second about the
locking rules here otherwise they would be documented somewhere.
You need a read_lock(_lock) for the mount part anyway. So why don't
you do the obvious:
fork
list_init(rg_list);
write_lock(tasklist_lock);
rdtgroup_post_fork();
if (use_rdtgroup_tasks)
spin_lock(_task_lock);
list_add();
spin_unlock(_task_lock);
task becomes visible
write_unlock(tasklist_lock);
And reorder the lock ordering in the mount path:
read_lock(_lock);
spin_lock(_task_lock);
Now using rdtgroup_task_lock to protect current->rdtgroup is horrible as
well. You need task->sighand->siglock in the mount path anyway to prevent exit
races. So you can simplify the whole magic to:
fork
list_init(rg_list);
write_lock(tasklist_lock);
spin_lock(>sighand->siglock);
rdtgroup_post_fork();
if (use_rdtgroup_tasks)
list_add();
spin_unlock(>sighand->siglock);
write_unlock(tasklist_lock);
That removes an extra lock/unlock operation from the fork path because
current->sighand->siglock is taken inside of the tasklist_lock write locked
section already.
So you need protection for use_rdtgroup_task, which is a complete misnomer
btw. (rdtgroup_active would be too obvious, right?). That protection is simple
because you can set that flag with tasklist_lock read locked which you hold
anyway for iterating all threads in the mount path.
Aside of that you need to take tsk->sighand->siglock when you change
tsk->rdtgroup, but that's a no-brainer and it gives you the extra benefit that
you can protect such an operation against exit of the task that way by
checking PF_EXITING under the lock. I don't see any protection against exit in
your current implementation when a task is moved to a different partition.
Please sit down and describe the complete locking and protection scheme of
this stuff. I'm not going to figure this out from the obscure code another
time.
Thanks,
tglx
RE: [PATCH 24/32] Task fork and exit for rdtgroup
On Wed, 13 Jul 2016, Yu, Fenghua wrote:
> On Wed, July 2016, Thomas Gleixner wrote
> > On Tue, 12 Jul 2016, Fenghua Yu wrote:
> > > +void rdtgroup_post_fork(struct task_struct *child) {
> > > + if (!use_rdtgroup_tasks)
> > > + return;
> > > +
> > > + spin_lock_irq(_task_lock);
> > > + if (list_empty(>rg_list)) {
> >
> > Why would the list be non empty after a fork?
>
> In this situation for a pid:
> 1.rdtgroup_fork(): rg_list=null.
> 2.setup_task_rg_lists(): rg_list is setup
> 3.rdtgroup_fork(): rg_list is not empty
Why would rdtgroup_fork() be called twice for a given thread?
> This situation happens only during rscctrl mount time. Before mount,
> post_fork()
> returns from !use_rdtgroup_tasks and doesn't set up rg_list. After mount,
> rg_list()
> is always empty in post_fork(). But we need to check rg_list for above
> situation.
>
> Does that make sense?
No, that does not make any sense at all.
> Any suggestion for better soluation?
The problem you have is:
fork
list_init(rg_list);
write_lock(tasklist_lock);
task becomes visible
write_unlock(tasklist_lock);
rdtgroup_post_fork();
if (!use_rdtgroup_tasks)
return;
spin_lock_irq(_task_lock);
list_add();
spin_unlock_irq(_task_lock);
I have no idea why this lock must be taken with _irq, but thats another
story. Let's look at the mount side:
spin_lock_irq(_task_lock);
read_lock(_lock);
do_each_thread(g, p) {
WARN_ON(More magic crap happening there)
spin_lock_irq(>sighand->siglock);
list_add();
spin_unlock_irq(>sighand->siglock);
Great: You undo the irq disable of (_task_lock) above! Oh well
read_unlock(_lock);
spin_unlock_irq(_task_lock);
So you need all this magic in rdtgroup_post_fork() and setup_task_rg_lists()
just because you blindly positioned rdtgroup_post_fork() at the point where
the cgroup_post_fork() stuff is. But you did not think a second about the
locking rules here otherwise they would be documented somewhere.
You need a read_lock(_lock) for the mount part anyway. So why don't
you do the obvious:
fork
list_init(rg_list);
write_lock(tasklist_lock);
rdtgroup_post_fork();
if (use_rdtgroup_tasks)
spin_lock(_task_lock);
list_add();
spin_unlock(_task_lock);
task becomes visible
write_unlock(tasklist_lock);
And reorder the lock ordering in the mount path:
read_lock(_lock);
spin_lock(_task_lock);
Now using rdtgroup_task_lock to protect current->rdtgroup is horrible as
well. You need task->sighand->siglock in the mount path anyway to prevent exit
races. So you can simplify the whole magic to:
fork
list_init(rg_list);
write_lock(tasklist_lock);
spin_lock(>sighand->siglock);
rdtgroup_post_fork();
if (use_rdtgroup_tasks)
list_add();
spin_unlock(>sighand->siglock);
write_unlock(tasklist_lock);
That removes an extra lock/unlock operation from the fork path because
current->sighand->siglock is taken inside of the tasklist_lock write locked
section already.
So you need protection for use_rdtgroup_task, which is a complete misnomer
btw. (rdtgroup_active would be too obvious, right?). That protection is simple
because you can set that flag with tasklist_lock read locked which you hold
anyway for iterating all threads in the mount path.
Aside of that you need to take tsk->sighand->siglock when you change
tsk->rdtgroup, but that's a no-brainer and it gives you the extra benefit that
you can protect such an operation against exit of the task that way by
checking PF_EXITING under the lock. I don't see any protection against exit in
your current implementation when a task is moved to a different partition.
Please sit down and describe the complete locking and protection scheme of
this stuff. I'm not going to figure this out from the obscure code another
time.
Thanks,
tglx
Re: [PATCH 24/32] Task fork and exit for rdtgroup
On Tue, 12 Jul 2016, Fenghua Yu wrote:
> +void rdtgroup_fork(struct task_struct *child)
> +{
> + INIT_LIST_HEAD(>rg_list);
> + child->rdtgroup = NULL;
> +}
> +
> +void rdtgroup_post_fork(struct task_struct *child)
> +{
> + if (!use_rdtgroup_tasks)
> + return;
> +
> + spin_lock_irq(_task_lock);
> + if (list_empty(>rg_list)) {
Why would the list be non empty after a fork?
> + struct rdtgroup *rdtgrp = current->rdtgroup;
> +
> + list_add_tail(>rg_list, >pset.tasks);
> + child->rdtgroup = rdtgrp;
> + atomic_inc(>pset.refcount);
> + }
> + spin_unlock_irq(_task_lock);
> +}
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 9e6e135..04346b6 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -59,6 +59,7 @@
> #include
> #include
> #include
> +#include
>
> static void __unhash_process(struct task_struct *p, bool group_dead)
> {
> @@ -757,6 +758,7 @@ void do_exit(long code)
> perf_event_exit_task(tsk);
>
> cgroup_exit(tsk);
> + rdtgroup_exit(tsk);
-ENOSUCHFUNCTION
Please provide the implementations first and then hook it up not the other way
round.
Thanks,
tglx
Re: [PATCH 24/32] Task fork and exit for rdtgroup
On Tue, 12 Jul 2016, Fenghua Yu wrote:
> +void rdtgroup_fork(struct task_struct *child)
> +{
> + INIT_LIST_HEAD(>rg_list);
> + child->rdtgroup = NULL;
> +}
> +
> +void rdtgroup_post_fork(struct task_struct *child)
> +{
> + if (!use_rdtgroup_tasks)
> + return;
> +
> + spin_lock_irq(_task_lock);
> + if (list_empty(>rg_list)) {
Why would the list be non empty after a fork?
> + struct rdtgroup *rdtgrp = current->rdtgroup;
> +
> + list_add_tail(>rg_list, >pset.tasks);
> + child->rdtgroup = rdtgrp;
> + atomic_inc(>pset.refcount);
> + }
> + spin_unlock_irq(_task_lock);
> +}
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 9e6e135..04346b6 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -59,6 +59,7 @@
> #include
> #include
> #include
> +#include
>
> static void __unhash_process(struct task_struct *p, bool group_dead)
> {
> @@ -757,6 +758,7 @@ void do_exit(long code)
> perf_event_exit_task(tsk);
>
> cgroup_exit(tsk);
> + rdtgroup_exit(tsk);
-ENOSUCHFUNCTION
Please provide the implementations first and then hook it up not the other way
round.
Thanks,
tglx
