On Sat, Mar 24, 2007 at 10:43:48PM -0700, Paul Jackson wrote:
> I'm unsure here, but this 'tsk->cpuset == cs' test feels fragile to me.
>
> How about a bit earlier in attach_task(), right at the point we overwrite the
> victim tasks cpuset pointer, we decrement the count on the old cpuset, and if
On Sat, Mar 24, 2007 at 10:43:48PM -0700, Paul Jackson wrote:
I'm unsure here, but this 'tsk-cpuset == cs' test feels fragile to me.
How about a bit earlier in attach_task(), right at the point we overwrite the
victim tasks cpuset pointer, we decrement the count on the old cpuset, and if
it
vatsa wrote:
> Not just this, continuing further we have more trouble:
>
>
> CPU0 (attach_task T1 to CS2) CPU1 (T1 is exiting)
>
>
>
> I will try to send out a patch later today to fix
Thanks!
> Agreed, but good to keep code clean isn't it? :)
Definitely.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[EMAIL PROTECTED]> 1.925.600.0401
On Sat, Mar 24, 2007 at 09:45:50PM -0700, Paul Jackson wrote:
> Nice work - thanks. Yes, both an extra cpuset count and a negative
> cpuset count are bad news, opening the door to the usual catastrophes.
>
> Would you like the honor of submitting the patch to add a task_lock
> to cpuset_exit()?
vatsa wrote:
> Now consider:
Nice work - thanks. Yes, both an extra cpuset count and a negative
cpuset count are bad news, opening the door to the usual catastrophes.
Would you like the honor of submitting the patch to add a task_lock
to cpuset_exit()? If you do, be sure to fix, or at least
On Sun, Mar 25, 2007 at 07:58:16AM +0530, Srivatsa Vaddagiri wrote:
> Not just this, continuing further we have more trouble:
>
>
> CPU0 (attach_task T1 to CS2) CPU1 (T1 is exiting)
>
On Sat, Mar 24, 2007 at 06:41:28PM -0700, Paul Jackson wrote:
> > the following code becomes racy with cpuset_exit() ...
> >
> > atomic_inc(>count);
> > rcu_assign_pointer(tsk->cpuset, cs);
> > task_unlock(tsk);
>
> eh ... so ... ?
>
> I don't know of any sequence where
vatsa wrote:
> > if (tsk->flags & PF_EXITING) {
>
> What if PF_EXITING is set after this check? If that happens then,
>
> > task_unlock(tsk);
> > mutex_unlock(_mutex);
> > put_task_struct(tsk);
> > return -ESRCH;
> >
On Sat, Mar 24, 2007 at 12:25:59PM -0700, Paul Jackson wrote:
> > P.S : cpuset.c checks for PF_EXITING twice in attach_task(), while this
> > patch seems to be checking only once. Is that fine?
>
> I think the cpuset code is ok, because, as you note, it locks the task,
> picks off the cpuset
> IMO, we need to use task_lock() in container_exit() to avoid this race.
>
> (I think this race already exists in mainline cpuset.c?)
>
> P.S : cpuset.c checks for PF_EXITING twice in attach_task(), while this
> patch seems to be checking only once. Is that fine?
I think the cpuset code is ok,
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
> +static int attach_task(struct container *cont, char *pidbuf, char **ppathbuf)
> +{
> + pid_t pid;
> + struct task_struct *tsk;
> + struct container *oldcont;
> + int retval;
> +
> + if (sscanf(pidbuf, "%d", )
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
+static int attach_task(struct container *cont, char *pidbuf, char **ppathbuf)
+{
+ pid_t pid;
+ struct task_struct *tsk;
+ struct container *oldcont;
+ int retval;
+
+ if (sscanf(pidbuf, %d, pid) != 1)
IMO, we need to use task_lock() in container_exit() to avoid this race.
(I think this race already exists in mainline cpuset.c?)
P.S : cpuset.c checks for PF_EXITING twice in attach_task(), while this
patch seems to be checking only once. Is that fine?
I think the cpuset code is ok,
On Sat, Mar 24, 2007 at 12:25:59PM -0700, Paul Jackson wrote:
P.S : cpuset.c checks for PF_EXITING twice in attach_task(), while this
patch seems to be checking only once. Is that fine?
I think the cpuset code is ok, because, as you note, it locks the task,
picks off the cpuset pointer,
vatsa wrote:
if (tsk-flags PF_EXITING) {
What if PF_EXITING is set after this check? If that happens then,
task_unlock(tsk);
mutex_unlock(callback_mutex);
put_task_struct(tsk);
return -ESRCH;
}
the
On Sat, Mar 24, 2007 at 06:41:28PM -0700, Paul Jackson wrote:
the following code becomes racy with cpuset_exit() ...
atomic_inc(cs-count);
rcu_assign_pointer(tsk-cpuset, cs);
task_unlock(tsk);
eh ... so ... ?
I don't know of any sequence where that causes
On Sun, Mar 25, 2007 at 07:58:16AM +0530, Srivatsa Vaddagiri wrote:
Not just this, continuing further we have more trouble:
CPU0 (attach_task T1 to CS2) CPU1 (T1 is exiting)
vatsa wrote:
Now consider:
Nice work - thanks. Yes, both an extra cpuset count and a negative
cpuset count are bad news, opening the door to the usual catastrophes.
Would you like the honor of submitting the patch to add a task_lock
to cpuset_exit()? If you do, be sure to fix, or at least
On Sat, Mar 24, 2007 at 09:45:50PM -0700, Paul Jackson wrote:
Nice work - thanks. Yes, both an extra cpuset count and a negative
cpuset count are bad news, opening the door to the usual catastrophes.
Would you like the honor of submitting the patch to add a task_lock
to cpuset_exit()? If
I will try to send out a patch later today to fix
Thanks!
Agreed, but good to keep code clean isn't it? :)
Definitely.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson [EMAIL PROTECTED] 1.925.600.0401
-
vatsa wrote:
Not just this, continuing further we have more trouble:
CPU0 (attach_task T1 to CS2) CPU1 (T1 is exiting)
On Thu, Mar 22, 2007 at 03:26:51PM +0530, Srivatsa Vaddagiri wrote:
> On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
> > +static void remove_dir(struct dentry *d)
> > +{
> > + struct dentry *parent = dget(d->d_parent);
>
> Don't we need to lock parent inode's mutex here?
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
> +static void remove_dir(struct dentry *d)
> +{
> + struct dentry *parent = dget(d->d_parent);
Don't we need to lock parent inode's mutex here? sysfs seems to take
that lock.
> +
> + d_delete(d);
> +
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
+static void remove_dir(struct dentry *d)
+{
+ struct dentry *parent = dget(d-d_parent);
Don't we need to lock parent inode's mutex here? sysfs seems to take
that lock.
+
+ d_delete(d);
+
On Thu, Mar 22, 2007 at 03:26:51PM +0530, Srivatsa Vaddagiri wrote:
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
+static void remove_dir(struct dentry *d)
+{
+ struct dentry *parent = dget(d-d_parent);
Don't we need to lock parent inode's mutex here? sysfs seems
On Sun, Mar 11, 2007 at 12:38:43PM -0700, Paul Jackson wrote:
> The primary reason for the cpuset double locking, as I recall, was because
> cpusets needs to access cpusets inside the memory allocator.
"needs to access cpusets" - can you be more specific?
Being able to safely walk
On Sun, Mar 11, 2007 at 12:38:43PM -0700, Paul Jackson wrote:
The primary reason for the cpuset double locking, as I recall, was because
cpusets needs to access cpusets inside the memory allocator.
needs to access cpusets - can you be more specific?
Being able to safely walk cpuset-parent
vatsa wrote:
> Yes, that way only the hierarchy hosting cpusets takes the hit of
> double-locking. cpuset_subsys->create/destroy can take this additional lock
> inside cpuset.c.
The primary reason for the cpuset double locking, as I recall, was because
cpusets needs to access cpusets inside the
vatsa wrote:
Yes, that way only the hierarchy hosting cpusets takes the hit of
double-locking. cpuset_subsys-create/destroy can take this additional lock
inside cpuset.c.
The primary reason for the cpuset double locking, as I recall, was because
cpusets needs to access cpusets inside the
On 3/8/07, Srivatsa Vaddagiri <[EMAIL PROTECTED]> wrote:
On Wed, Mar 07, 2007 at 12:50:03PM -0800, Paul Menage wrote:
> The callback mutex (which is what container_lock() actually locks) is
> also used to synchronize fork/exit against subsystem additions, in the
> event that some subsystem has
On Wed, Mar 07, 2007 at 12:50:03PM -0800, Paul Menage wrote:
> The callback mutex (which is what container_lock() actually locks) is
> also used to synchronize fork/exit against subsystem additions, in the
> event that some subsystem has registered fork or exit callbacks. We
> could probably have
On Wed, Mar 07, 2007 at 12:50:03PM -0800, Paul Menage wrote:
The callback mutex (which is what container_lock() actually locks) is
also used to synchronize fork/exit against subsystem additions, in the
event that some subsystem has registered fork or exit callbacks. We
could probably have a
On 3/8/07, Srivatsa Vaddagiri [EMAIL PROTECTED] wrote:
On Wed, Mar 07, 2007 at 12:50:03PM -0800, Paul Menage wrote:
The callback mutex (which is what container_lock() actually locks) is
also used to synchronize fork/exit against subsystem additions, in the
event that some subsystem has
On 3/7/07, Srivatsa Vaddagiri <[EMAIL PROTECTED]> wrote:
If that is the case, I think we can push container_lock entirely inside
cpuset.c and not have others exposed to this double-lock complexity.
This is possible because cpuset.c (build on top of containers) still has
cpuset->parent and
On Wed, Mar 07, 2007 at 05:51:17PM +0530, Srivatsa Vaddagiri wrote:
> If that is the case, I think we can push container_lock entirely inside
> cpuset.c and not have others exposed to this double-lock complexity.
> This is possible because cpuset.c (build on top of containers) still has
>
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
> +/**
> + * container_lock - lock out any changes to container structures
> + *
> + * The out of memory (oom) code needs to mutex_lock containers
> + * from being changed while it scans the tasklist looking for a
> + * task in an
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
+/**
+ * container_lock - lock out any changes to container structures
+ *
+ * The out of memory (oom) code needs to mutex_lock containers
+ * from being changed while it scans the tasklist looking for a
+ * task in an
On Wed, Mar 07, 2007 at 05:51:17PM +0530, Srivatsa Vaddagiri wrote:
If that is the case, I think we can push container_lock entirely inside
cpuset.c and not have others exposed to this double-lock complexity.
This is possible because cpuset.c (build on top of containers) still has
On 3/7/07, Srivatsa Vaddagiri [EMAIL PROTECTED] wrote:
If that is the case, I think we can push container_lock entirely inside
cpuset.c and not have others exposed to this double-lock complexity.
This is possible because cpuset.c (build on top of containers) still has
cpuset-parent and walking
On Tue, Feb 13, 2007 at 11:18:57AM +0530, Srivatsa Vaddagiri wrote:
> Which make me wonder why we need task_lock() at all ..I can understand
> the need for a lock like that if we are reading/updating multiple words
> in task_struct under the lock. In this case, it is used to read/write
> just one
On Tue, Feb 13, 2007 at 11:18:57AM +0530, Srivatsa Vaddagiri wrote:
Which make me wonder why we need task_lock() at all ..I can understand
the need for a lock like that if we are reading/updating multiple words
in task_struct under the lock. In this case, it is used to read/write
just one
On Mon, Feb 12, 2007 at 11:46:20AM -0800, Paul Menage wrote:
> On further reflection, this probably would be safe after all. Since we
> don't call put_container_group() in attach_task() until after
> synchronize_rcu() completes, that implies that a container_group_get()
> from the RCU section
On 2/12/07, Paul Menage <[EMAIL PROTECTED]> wrote:
reaches zero. RCU is still fine for reading the container_group
pointers, but it's no good for updating them, since by the time you
update it it may no longer be your container_group structure, and may
instead be about to be deleted as soon as
On 2/12/07, Srivatsa Vaddagiri <[EMAIL PROTECTED]> wrote:
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
> +void container_fork(struct task_struct *child)
> +{
> + task_lock(current);
Can't this be just rcu_read_lock()?
In this particular patch (which is an almost
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
> +void container_fork(struct task_struct *child)
> +{
> + task_lock(current);
Can't this be just rcu_read_lock()?
> + child->container = current->container;
> + atomic_inc(>container->count);
> +
This patch creates a generic process container system based on (and
parallel top) the cpusets code. At a coarse level it was created by
copying kernel/cpuset.c, doing s/cpuset/container/g, and stripping out any
code that was cpuset-specific rather than applicable to any process
container
This patch creates a generic process container system based on (and
parallel top) the cpusets code. At a coarse level it was created by
copying kernel/cpuset.c, doing s/cpuset/container/g, and stripping out any
code that was cpuset-specific rather than applicable to any process
container
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
+void container_fork(struct task_struct *child)
+{
+ task_lock(current);
Can't this be just rcu_read_lock()?
+ child-container = current-container;
+ atomic_inc(child-container-count);
+
On 2/12/07, Srivatsa Vaddagiri [EMAIL PROTECTED] wrote:
On Mon, Feb 12, 2007 at 12:15:22AM -0800, [EMAIL PROTECTED] wrote:
+void container_fork(struct task_struct *child)
+{
+ task_lock(current);
Can't this be just rcu_read_lock()?
In this particular patch (which is an almost verbatim
On 2/12/07, Paul Menage [EMAIL PROTECTED] wrote:
reaches zero. RCU is still fine for reading the container_group
pointers, but it's no good for updating them, since by the time you
update it it may no longer be your container_group structure, and may
instead be about to be deleted as soon as the
On Mon, Feb 12, 2007 at 11:46:20AM -0800, Paul Menage wrote:
On further reflection, this probably would be safe after all. Since we
don't call put_container_group() in attach_task() until after
synchronize_rcu() completes, that implies that a container_group_get()
from the RCU section would
52 matches
Mail list logo
