Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1441768 > unrolled thread

[PATCH 24/32] Task fork and exit for rdtgroup

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 24/32] Task fork and exit for rdtgroup "Fenghua Yu" <fenghua.yu@intel.com> - 2016-07-13 00:10 +0200
    Re: [PATCH 24/32] Task fork and exit for rdtgroup Thomas Gleixner <tglx@linutronix.de> - 2016-07-13 15:20 +0200
      RE: [PATCH 24/32] Task fork and exit for rdtgroup Thomas Gleixner <tglx@linutronix.de> - 2016-07-13 23:10 +0200
        RE: [PATCH 24/32] Task fork and exit for rdtgroup "Yu, Fenghua" <fenghua.yu@intel.com> - 2016-07-13 23:30 +0200

#1441768 — [PATCH 24/32] Task fork and exit for rdtgroup

From: Fenghua Yu <fenghua.yu@intel.com>

When a task is forked, it inherites its parent rdtgroup. The task
can be moved to other rdtgroup during its run time.

When the task exits, it's deleted from it's current rdtgroup's task
list.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/intel_rdt.h |  1 +
 arch/x86/kernel/cpu/intel_rdt.c  | 22 ++++++++++++++++++++++
 kernel/exit.c                    |  2 ++
 kernel/fork.c                    |  4 ++++
 4 files changed, 29 insertions(+)

diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 5aacc4a..f2298f3 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -22,6 +22,7 @@ enum resource_type {
 DECLARE_PER_CPU_READ_MOSTLY(int, cpu_l3_domain);
 DECLARE_PER_CPU_READ_MOSTLY(struct rdtgroup *, cpu_rdtgroup);
 
+extern spinlock_t rdtgroup_task_lock;
 extern struct static_key rdt_enable_key;
 void __intel_rdt_sched_in(void *dummy);
 extern bool use_rdtgroup_tasks;
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 017c833..901156d 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -812,3 +812,25 @@ static int __init intel_rdt_late_init(void)
 }
 
 late_initcall(intel_rdt_late_init);
+
+void rdtgroup_fork(struct task_struct *child)
+{
+	INIT_LIST_HEAD(&child->rg_list);
+	child->rdtgroup = NULL;
+}
+
+void rdtgroup_post_fork(struct task_struct *child)
+{
+	if (!use_rdtgroup_tasks)
+		return;
+
+	spin_lock_irq(&rdtgroup_task_lock);
+	if (list_empty(&child->rg_list)) {
+		struct rdtgroup *rdtgrp = current->rdtgroup;
+
+		list_add_tail(&child->rg_list, &rdtgrp->pset.tasks);
+		child->rdtgroup = rdtgrp;
+		atomic_inc(&rdtgrp->pset.refcount);
+	}
+	spin_unlock_irq(&rdtgroup_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 <asm/unistd.h>
 #include <asm/pgtable.h>
 #include <asm/mmu_context.h>
+#include <asm/intel_rdt.h>
 
 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);
 
 	/*
 	 * FIXME: do that only when needed, using sched_exit tracepoint
diff --git a/kernel/fork.c b/kernel/fork.c
index 4a7ec0c..d9bb373 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -84,6 +84,8 @@
 #include <asm/cacheflush.h>
 #include <asm/tlbflush.h>
 
+#include <asm/intel_rdt.h>
+
 #include <trace/events/sched.h>
 
 #define CREATE_TRACE_POINTS
@@ -1408,6 +1410,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	p->audit_context = NULL;
 	threadgroup_change_begin(current);
 	cgroup_fork(p);
+	rdtgroup_fork(p);
 #ifdef CONFIG_NUMA
 	p->mempolicy = mpol_dup(p->mempolicy);
 	if (IS_ERR(p->mempolicy)) {
@@ -1647,6 +1650,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 
 	proc_fork_connector(p);
 	cgroup_post_fork(p);
+	rdtgroup_post_fork(p);
 	threadgroup_change_end(current);
 	perf_event_fork(p);
 
-- 
2.5.0

[toc] | [next] | [standalone]

#1442446

On Tue, 12 Jul 2016, Fenghua Yu wrote:
> +void rdtgroup_fork(struct task_struct *child)
> +{
> +	INIT_LIST_HEAD(&child->rg_list);
> +	child->rdtgroup = NULL;
> +}
> +
> +void rdtgroup_post_fork(struct task_struct *child)
> +{
> +	if (!use_rdtgroup_tasks)
> +		return;
> +
> +	spin_lock_irq(&rdtgroup_task_lock);
> +	if (list_empty(&child->rg_list)) {

Why would the list be non empty after a fork?

> +		struct rdtgroup *rdtgrp = current->rdtgroup;
> +
> +		list_add_tail(&child->rg_list, &rdtgrp->pset.tasks);
> +		child->rdtgroup = rdtgrp;
> +		atomic_inc(&rdtgrp->pset.refcount);
> +	}
> +	spin_unlock_irq(&rdtgroup_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 <asm/unistd.h>
>  #include <asm/pgtable.h>
>  #include <asm/mmu_context.h>
> +#include <asm/intel_rdt.h>
>  
>  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

[toc] | [prev] | [next] | [standalone]

#1442857

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(&rdtgroup_task_lock);
> > > +	if (list_empty(&child->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(&rdtgroup_task_lock);
  	    list_add();
	    spin_unlock_irq(&rdtgroup_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(&rdtgroup_task_lock);
       read_lock(&tasklist_lock);
       
       do_each_thread(g, p) { 
       	   WARN_ON(More magic crap happening there)

	   spin_lock_irq(&p->sighand->siglock);
	   list_add();
	   spin_unlock_irq(&p->sighand->siglock);
		      ^^^^ 
Great: You undo the irq disable of (&rdtgroup_task_lock) above! Oh well....

       read_unlock(&tasklist_lock);
       spin_unlock_irq(&rdtgroup_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(&tasklist_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(&rdtgroup_task_lock);
	        list_add();
	        spin_unlock(&rdtgroup_task_lock);

        task becomes visible

	write_unlock(tasklist_lock);

And reorder the lock ordering in the mount path:

       read_lock(&tasklist_lock);
       spin_lock(&rdtgroup_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(&current->sighand->siglock);

	rdtgroup_post_fork();
	    if (use_rdtgroup_tasks)
	        list_add();

	spin_unlock(&current->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

[toc] | [prev] | [next] | [standalone]

#1442877

> From: Thomas Gleixner [mailto:tglx@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(&rdtgroup_task_lock);
> > > > +	if (list_empty(&child->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(&rdtgroup_task_lock);
>   	    list_add();
> 	    spin_unlock_irq(&rdtgroup_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(&rdtgroup_task_lock);
>        read_lock(&tasklist_lock);
> 
>        do_each_thread(g, p) {
>        	   WARN_ON(More magic crap happening there)
> 
> 	   spin_lock_irq(&p->sighand->siglock);
> 	   list_add();
> 	   spin_unlock_irq(&p->sighand->siglock);
> 		      ^^^^
> Great: You undo the irq disable of (&rdtgroup_task_lock) above! Oh well....
> 
>        read_unlock(&tasklist_lock);
>        spin_unlock_irq(&rdtgroup_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(&tasklist_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(&rdtgroup_task_lock);
> 	        list_add();
> 	        spin_unlock(&rdtgroup_task_lock);
> 
>         task becomes visible
> 
> 	write_unlock(tasklist_lock);
> 
> And reorder the lock ordering in the mount path:
> 
>        read_lock(&tasklist_lock);
>        spin_lock(&rdtgroup_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(&current->sighand->siglock);
> 
> 	rdtgroup_post_fork();
> 	    if (use_rdtgroup_tasks)
> 	        list_add();
> 
> 	spin_unlock(&current->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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web