Message-ID: <alpine.DEB.2.20.1609082226390.5454@nanos>
Date:   Thu, 8 Sep 2016 22:50:35 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     "H. Peter Anvin" <h.peter.anvin@...el.com>,
        Ingo Molnar <mingo@...e.hu>, Tony Luck <tony.luck@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Tejun Heo <tj@...nel.org>, Borislav Petkov <bp@...e.de>,
        Stephane Eranian <eranian@...gle.com>,
        Marcelo Tosatti <mtosatti@...hat.com>,
        David Carrillo-Cisneros <davidcc@...gle.com>,
        Shaohua Li <shli@...com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
        Sai Prakhya <sai.praneeth.prakhya@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v2 29/33] x86/intel_rdt_rdtgroup.c: Tasks iterator and
 write

On Thu, 8 Sep 2016, Fenghua Yu wrote:
>  void rdtgroup_exit(struct task_struct *tsk)
>  {
> +	if (tsk->rdtgroup) {
> +		atomic_dec(&tsk->rdtgroup->refcount);
> +		tsk->rdtgroup = NULL;

The changelog still talks about this list stuff while this patch seems to
remove it and solely rely on the tsk->rdtgroup pointer, which is a sensible
thing to do.

Writing sensible changelogs would make the life of reviewers too easy,
right?

> +	}
> +}
>  
> -	if (!list_empty(&tsk->rg_list)) {
> -		struct rdtgroup *rdtgrp = tsk->rdtgroup;
> +static void show_rdt_tasks(struct rdtgroup *r, struct seq_file *s)
> +{
> +	struct task_struct *p;
> +	struct rdtgroup *this = r;
>  
> -		list_del_init(&tsk->rg_list);
> -		tsk->rdtgroup = NULL;
> -		atomic_dec(&rdtgrp->refcount);
> +
> +	if (r == root_rdtgrp)
> +		return;

Why has the root_rdtgroup a task file in the first place?

>  static void rdtgroup_destroy_locked(struct rdtgroup *rdtgrp)
> @@ -781,8 +811,6 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
>  		goto out_unlock;
>  	}
>  
> -	INIT_LIST_HEAD(&rdtgrp->pset.tasks);
> -
>  	cpumask_clear(&rdtgrp->cpu_mask);
>  
>  	rdtgrp->root = root;
> @@ -843,7 +871,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
>  	if (!rdtgrp)
>  		return -ENODEV;
>  
> -	if (!list_empty(&rdtgrp->pset.tasks)) {
> +	if (atomic_read(&rdtgrp->refcount)) {

So you rely on rdtgrp->refcount completely now.

>  /*
>   * Forcibly remove all of subdirectories under root.
> @@ -1088,22 +1094,16 @@ void rdtgroup_fork(struct task_struct *child)
>  {
>  	struct rdtgroup *rdtgrp;
>  
> -	INIT_LIST_HEAD(&child->rg_list);
> +	child->rdtgroup = NULL;
>  	if (!rdtgroup_mounted)
>  		return;
>  
> -	mutex_lock(&rdtgroup_mutex);
> -
>  	rdtgrp = current->rdtgroup;
>  	if (!rdtgrp)
> -		goto out;
> +		return;
>  
> -	list_add_tail(&child->rg_list, &rdtgrp->pset.tasks);
>  	child->rdtgroup = rdtgrp;
>  	atomic_inc(&rdtgrp->refcount);

This lacks any form of documentation WHY this is correct and works. I asked
you last time to document the locking and serialization rules ....

>  	cpumask_copy(&root->rdtgrp.cpu_mask, cpu_online_mask);

Can't remember if I told you already, but this is racy against hotplug.

> +static int _rdtgroup_move_task(struct task_struct *tsk, struct rdtgroup *rdtgrp)
> +{
> +	if (tsk->rdtgroup)
> +		atomic_dec(&tsk->rdtgroup->refcount);
> +
> +	if (rdtgrp == root_rdtgrp)
> +		tsk->rdtgroup = NULL;
> +	else
> +		tsk->rdtgroup = rdtgrp;
> +
> +	atomic_inc(&rdtgrp->refcount);
> +
> +	return 0;
> +}
> +
> +static int rdtgroup_move_task(pid_t pid, struct rdtgroup *rdtgrp,
> +			      bool threadgroup, struct kernfs_open_file *of)
> +{
> +	struct task_struct *tsk;
> +	int ret;
> +
> +	rcu_read_lock();
> +	if (pid) {
> +		tsk = find_task_by_vpid(pid);
> +		if (!tsk) {
> +			ret = -ESRCH;
> +			goto out_unlock_rcu;
> +		}
> +	} else {
> +		tsk = current;
> +	}
> +
> +	if (threadgroup)
> +		tsk = tsk->group_leader;
> +
> +	get_task_struct(tsk);
> +	rcu_read_unlock();
> +
> +	ret = rdtgroup_procs_write_permission(tsk, of);
> +	if (!ret)
> +		_rdtgroup_move_task(tsk, rdtgrp);
> +
> +	put_task_struct(tsk);
> +	goto out_unlock_threadgroup;
> +
> +out_unlock_rcu:
> +	rcu_read_unlock();
> +out_unlock_threadgroup:
> +	return ret;
> +}
> +
> +ssize_t _rdtgroup_procs_write(struct rdtgroup *rdtgrp,
> +			   struct kernfs_open_file *of, char *buf,
> +			   size_t nbytes, loff_t off, bool threadgroup)

global visible? And what is the underscore for?

> +{
> +	pid_t pid;
> +	int ret;
> +
> +	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
> +		return -EINVAL;

Why do you evaluate the buffer inside the lock held region?

This function split is completely bogus and artificial.

> +
> +	ret = rdtgroup_move_task(pid, rdtgrp, threadgroup, of);
> +
> +	return ret ?: nbytes;
> +}
> +
> +static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
> +				  char *buf, size_t nbytes, loff_t off)
> +{
> +	struct rdtgroup *rdtgrp;
> +	int ret;
> +
> +	rdtgrp = rdtgroup_kn_lock_live(of->kn);
> +	if (!rdtgrp)
> +		return -ENODEV;
> +
> +	ret = _rdtgroup_procs_write(rdtgrp, of, buf, nbytes, off, false);
> +
> +	rdtgroup_kn_unlock(of->kn);
> +	return ret;
> +}

More sigh.

     tglx

Powered by blists - more mailing lists