lkml.org 
[lkml]   [2015]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v13 4/5] cgroup: allow a cgroup subsystem to reject a fork
    Hello, Aleksa.

    Looks pretty good to me in general. Some minor comments below.

    On Sat, Jun 06, 2015 at 10:02:17AM +1000, Aleksa Sarai wrote:

    > diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
    > index a593e29..17d0046 100644
    > --- a/include/linux/cgroup.h
    > +++ b/include/linux/cgroup.h
    > @@ -62,9 +62,15 @@ int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
    > struct pid *pid, struct task_struct *tsk);

    > void cgroup_fork(struct task_struct *p);
    > -void cgroup_post_fork(struct task_struct *p);
    > +extern int cgroup_can_fork(struct task_struct *p,
    > + void *ss_priv[CGROUP_CANFORK_COUNT]);
    > +extern void cgroup_cancel_fork(struct task_struct *p,
    > + void *ss_priv[CGROUP_CANFORK_COUNT]);
    > +extern void cgroup_post_fork(struct task_struct *p,
    > + void *old_ss_priv[CGROUP_CANFORK_COUNT]);
    > void cgroup_exit(struct task_struct *p);
    >
    > +

    Is this blank line intentional?

    > int cgroup_init_early(void);
    > int cgroup_init(void);
    ...
    > @@ -4924,6 +4927,7 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early)
    >
    > have_fork_callback |= (bool)ss->fork << ss->id;
    > have_exit_callback |= (bool)ss->exit << ss->id;
    > + have_canfork_callback |= (bool)ss->can_fork << ss->id;

    Hmmm.... do we still need this mask? We're already restricting
    iteration pretty heavily. I'd even suggest dropping both
    have_fork_callback and have_exit_callback too and just put them inside
    CGROUP_FORK_EXIT_START / STOP although that doesn't belong in this
    patchset.

    ...
    > +static void *subsys_canfork_priv(void *ss_priv[CGROUP_CANFORK_COUNT], int i)
    > +{
    > + void **private;
    > + if ((private = subsys_canfork_priv_p(ss_priv, i)) != NULL)
    > + return *private;
    > + return NULL;
    > +}

    void **private = subsys_canfork...;

    if (private)
    return *private;
    return NULL;

    or even just

    return private ? *private : NULL;

    We conventionally don't put assignments in if conditionals.

    > +void cgroup_cancel_fork(struct task_struct *child,
    > + void *ss_priv[CGROUP_CANFORK_COUNT])
    > +{
    > + struct cgroup_subsys *ss;
    > + int i;
    > +
    > + for_each_subsys(ss, i)
    > + if(ss->cancel_fork)
    ^
    space

    > + ss->cancel_fork(child, subsys_canfork_priv(ss_priv, i));
    > +}
    > +
    > +/**
    > * cgroup_post_fork - called on a new task after adding it to the task list
    > * @child: the task in question
    > *

    Thanks.

    --
    tejun


    \
     
     \ /
      Last update: 2015-06-09 07:21    [W:2.690 / U:0.212 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site