Re: [PATCH v4 15/18] x86/intel_rdt: Add tasks files
On Tue, Oct 18, 2016 at 12:01:01AM +0200, Thomas Gleixner wrote:
> > + /* Don't allow if there are processes in this group */
> > + read_lock(_lock);
> > + for_each_process(p) {
> > + if (p->closid == rdtgrp->closid) {
> > + read_unlock(_lock);
> > + rdtgroup_kn_unlock(kn);
> > + return -EBUSY;
> > + }
> > + }
> > + read_unlock(_lock);
>
> I wonder, whether we should simply give those tasks back to the default
> group, same as we do with the cpus.
Leftover inherited semantics from the cgroup version. It would
simplify the code here (one less error case to handle) if we
did drop this. I can't come up with a good reason why we'd
want to make the rmdir fail. I can imagine that a sysadmin
dealing with an application that is a fork bomb being happy
about not having to race to move things out so they can do the
rmdir.
-Tony
Re: [PATCH v4 15/18] x86/intel_rdt: Add tasks files
On Tue, Oct 18, 2016 at 12:01:01AM +0200, Thomas Gleixner wrote:
> > + /* Don't allow if there are processes in this group */
> > + read_lock(_lock);
> > + for_each_process(p) {
> > + if (p->closid == rdtgrp->closid) {
> > + read_unlock(_lock);
> > + rdtgroup_kn_unlock(kn);
> > + return -EBUSY;
> > + }
> > + }
> > + read_unlock(_lock);
>
> I wonder, whether we should simply give those tasks back to the default
> group, same as we do with the cpus.
Leftover inherited semantics from the cgroup version. It would
simplify the code here (one less error case to handle) if we
did drop this. I can't come up with a good reason why we'd
want to make the rmdir fail. I can imagine that a sysadmin
dealing with an application that is a fork bomb being happy
about not having to race to move things out so they can do the
rmdir.
-Tony
Re: [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(>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(>waitcount);
> + ret = task_work_add(tsk, >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(>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(_lock);
> + for_each_process(p)
> + p->closid = 0;
> + read_unlock(_lock);
Ok.
> + /* Don't allow if there are processes in this group */
> + read_lock(_lock);
> + for_each_process(p) {
> + if (p->closid == rdtgrp->closid) {
> + read_unlock(_lock);
> + rdtgroup_kn_unlock(kn);
> + return -EBUSY;
> + }
> + }
> + read_unlock(_lock);
I wonder, whether we should simply give those tasks back to the default
group, same as we do with the cpus.
Thanks,
tglx
Re: [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(>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(>waitcount);
> + ret = task_work_add(tsk, >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(>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(_lock);
> + for_each_process(p)
> + p->closid = 0;
> + read_unlock(_lock);
Ok.
> + /* Don't allow if there are processes in this group */
> + read_lock(_lock);
> + for_each_process(p) {
> + if (p->closid == rdtgrp->closid) {
> + read_unlock(_lock);
> + rdtgroup_kn_unlock(kn);
> + return -EBUSY;
> + }
> + }
> + read_unlock(_lock);
I wonder, whether we should simply give those tasks back to the default
group, same as we do with the cpus.
Thanks,
tglx
