lkml.org 
[lkml]   [2016]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v4 15/18] x86/intel_rdt: Add tasks files
On Fri, 14 Oct 2016, Fenghua Yu wrote:
> +struct task_move_callback {
> + struct callback_head work;
> + struct rdtgroup *rdtgrp;

Please align the struct members as you did everywhere else already.

> +};
> +
> +static void move_myself(struct callback_head *head)
> +{
> + struct task_move_callback *callback;
> + struct rdtgroup *rdtgrp;
> +
> + callback = container_of(head, struct task_move_callback, work);
> + rdtgrp = callback->rdtgrp;
> +
> + /* Resource group might have been deleted before process runs */

/*
* If resource group was deleted before this task work callback
* was invoked, then assign the task to root group and free the
* resource group,
*/

> + if (atomic_dec_and_test(&rdtgrp->waitcount) &&
> + (rdtgrp->flags & RDT_DELETED)) {
> + current->closid = 0;
> + kfree(rdtgrp);
> + }
> +
> + kfree(callback);
> +}
> +
> +static int __rdtgroup_move_task(struct task_struct *tsk,
> + struct rdtgroup *rdtgrp)
> +{
> + struct task_move_callback *callback;
> + int ret;
> +
> + callback = kzalloc(sizeof(*callback), GFP_KERNEL);
> + if (!callback)
> + return -ENOMEM;
> + callback->work.func = move_myself;
> + callback->rdtgrp = rdtgrp;

Lacks a comment:

/*
* Take a refcount, so rdtgrp cannot be freed before the
* callback has been invoked
*/

> + atomic_inc(&rdtgrp->waitcount);
> + ret = task_work_add(tsk, &callback->work, true);
> + if (ret) {


Lacks a comment as well:

/*
* Task is exiting. Drop the refcount and free the callback.
* No need to check the refcount as the group cannot be
* deleted before the write function unlocks rdtgroup_mutex.
*/

For you the comment might be obvious, but I had to lookup the world and
some more.

> + atomic_dec(&rdtgrp->waitcount);
> + kfree(callback);
> + } else {
> + tsk->closid = rdtgrp->closid;
> + }
> + return ret;

> +static int rdtgroup_task_write_permission(struct task_struct *task,
> + struct kernfs_open_file *of)
> +{
> + const struct cred *cred = current_cred();
> + const struct cred *tcred = get_task_cred(task);
> + int ret = 0;
> +
> + /*
> + * even if we're attaching all tasks in the thread group, we only

Sentences start with an uppercase letter.

> + * need to check permissions on one of them.
> + */
> + if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
> + !uid_eq(cred->euid, tcred->uid) &&
> + !uid_eq(cred->euid, tcred->suid))
> + ret = -EPERM;
> +
> + put_cred(tcred);
> + return ret;
> +}
> +
> +static int rdtgroup_move_task(pid_t pid, struct rdtgroup *rdtgrp,
> + 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;

This goto is pointless as this is the only user,

rcu_read_unlock()l
return -ESRCH;

> + }
> + } else {
> + tsk = current;
> + }

> @@ -559,6 +713,13 @@ static void rmdir_all_sub(void)
> {
> struct rdtgroup *rdtgrp;
> struct list_head *l, *next;
> + struct task_struct *p;
> +
> + /* move all tasks to default resource group */
> + read_lock(&tasklist_lock);
> + for_each_process(p)
> + p->closid = 0;
> + read_unlock(&tasklist_lock);

Ok.

> + /* Don't allow if there are processes in this group */
> + read_lock(&tasklist_lock);
> + for_each_process(p) {
> + if (p->closid == rdtgrp->closid) {
> + read_unlock(&tasklist_lock);
> + rdtgroup_kn_unlock(kn);
> + return -EBUSY;
> + }
> + }
> + read_unlock(&tasklist_lock);

I wonder, whether we should simply give those tasks back to the default
group, same as we do with the cpus.

Thanks,

tglx
\
 
 \ /
  Last update: 2016-10-18 00:04    [W:0.134 / U:1.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site