* Re: [PATCH UPDATED 03/10] threadgroup: extend threadgroup_lock() to cover exit and exec
From: Tejun Heo @ 2011-11-27 19:21 UTC (permalink / raw)
To: Linus Torvalds
Cc: akpm, fweisbec, containers, lizf, linux-kernel, oleg, linux-pm,
paul, kamezawa.hiroyu
In-Reply-To: <CA+55aFxq1wztMzYhKaY5RHazLBDz4pSXUgiGzTj2wA6EJcDbAw@mail.gmail.com>
Hello, Linus.
On Thu, Nov 24, 2011 at 08:02:18PM -0800, Linus Torvalds wrote:
> - do you even *need* two separate locks at all? If you extend the
> threadgroup_fork_lock to cover exec/exit too (and rename it), then why
> does that separate cred_guard_mutex make sense at all?
That was mostly choosing the path of least resistance as both locks
already existed and exec exclusion was added later on, but yeah there
isn't much to lose by merging those two locks (do_exit nesting inside
exec will need some care tho). I'll try to unify the two locks.
> Taking two locks for the common exit case seems disgusting. What do
> the separate locks buy us?
A bit confused. threadgroup_change_begin() only locks
signal->group_rwsem. ie. fork/exit is protected by
signal->group_rwsem. exec is protected by signal->cred_guard_mutex.
Grabbing both locks gives stable thread group.
> - could we possible avoid the lock(s) entirely for the
> single-threaded case? The fact that ptrace wants to serialize makes me
> say "maybe we can't avoid it", but I thought I'd ask. Even if we do
> need/want two separate locks, do we really want to take them both for
> the case that shouldn't really need even a single one?
Maybe we can avoid grabbing a lock on exec and exit if the task
belongs to single task process but optimizations like that can be very
fragile. I think it would be better to merge the two locks and keep
the locking unconditional.
> Hmm? Simplifying the locking rules is always a good idea, and I think
> this seems to make some of it more straightforward, but at the same
> time I shudder when I look at some of the patches in the series that
> nest locking three locks deep. Ugh.
cgroup_root_mutex separation is to avoid lock recursion via
cgroup_show_options and can probably be done in lighter handed way.
As I'm still getting used to cgroup code, I wanted to resolve the
locking order without changing much else.
Anyways, I'll send separated patch series dealing with fork/exec/exit
synchronization.
Thank you.
--
tejun
^ permalink raw reply
* Re: [PATCH UPDATED 03/10] threadgroup: extend threadgroup_lock() to cover exit and exec
From: Frederic Weisbecker @ 2011-11-25 14:01 UTC (permalink / raw)
To: Tejun Heo
Cc: akpm, paul, lizf, linux-kernel, oleg, linux-pm, Linus Torvalds,
containers, kamezawa.hiroyu
In-Reply-To: <20111124225054.GA14828@google.com>
On Thu, Nov 24, 2011 at 02:50:54PM -0800, Tejun Heo wrote:
> threadgroup_lock() protected only protected against new addition to
> the threadgroup, which was inherently somewhat incomplete and
> problematic for its only user cgroup. On-going migration could race
> against exec and exit leading to interesting problems - the symmetry
> between various attach methods, task exiting during method execution,
> ->exit() racing against attach methods, migrating task switching basic
> properties during exec and so on.
>
> This patch extends threadgroup_lock() such that it protects against
> all three threadgroup altering operations - fork, exit and exec. For
> exit, threadgroup_change_begin/end() calls are added to exit path.
> For exec, threadgroup_[un]lock() are updated to also grab and release
> cred_guard_mutex.
>
> With this change, threadgroup_lock() guarantees that the target
> threadgroup will remain stable - no new task will be added, no new
> PF_EXITING will be set and exec won't happen.
>
> The next patch will update cgroup so that it can take full advantage
> of this change.
>
> -v2: beefed up comment as suggested by Frederic.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Acked-by: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Paul Menage <paul@paulmenage.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>
> Linus, this is something being scheduled for the next merge window.
> It extends threadgroup locking which cgroup used to do to exclude only
> fork path so that it includes exit and exec paths. threadgroup
> locking is used by cgroup to implement process-scope cgroup migration.
>
> Migration happens in multiple steps, at each of which the matching
> method of each cgroup plugin is called. When a whole process is being
> migrated, methods being called at those different steps expect to see
> consistent image of the thread group.
>
> cgroup currently only locks out addition of new tasks into the thread
> group and holds extra ref to member tasks. This mandates all cgroup
> plugins to deal with tasks being torn down and exec morphing the
> threadgroup. This patch extends the scope of threadgroup locking such
> that the thread group is guaranteed to be stable (no new task, tasks
> are either live or dead and no exec morphing) while locked.
>
> This is part of changes to clean up cgroup methods and iron out corner
> case fuzziness and should make difficult-to-reproduce race conditions
> less likely and cgroup plugins easier to implement and verify.
>
> The synchronization is strictly per-threadgroup and goes away if
> cgroup is not configured.
>
> The whole series is avilable at
>
> http://thread.gmane.org/gmane.linux.kernel.containers/21716
> git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.3
>
> Thank you.
>
> include/linux/sched.h | 47 +++++++++++++++++++++++++++++++++++++++++------
> kernel/exit.c | 19 +++++++++++++++----
> 2 files changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index c5acbce..481c5ed 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -635,11 +635,12 @@ struct signal_struct {
> #endif
> #ifdef CONFIG_CGROUPS
> /*
> - * The group_rwsem prevents threads from forking with
> - * CLONE_THREAD while held for writing. Use this for fork-sensitive
> - * threadgroup-wide operations. It's taken for reading in fork.c in
> - * copy_process().
> - * Currently only needed write-side by cgroups.
> + * group_rwsem prevents new tasks from entering the threadgroup and
> + * member tasks from exiting. fork and exit paths are protected
> + * with this rwsem using threadgroup_change_begin/end(). Users
> + * which require threadgroup to remain stable should use
> + * threadgroup_[un]lock() which also takes care of exec path.
> + * Currently, cgroup is the only user.
> */
> struct rw_semaphore group_rwsem;
> #endif
> @@ -2374,7 +2375,6 @@ static inline void unlock_task_sighand(struct task_struct *tsk,
> spin_unlock_irqrestore(&tsk->sighand->siglock, *flags);
> }
>
> -/* See the declaration of group_rwsem in signal_struct. */
> #ifdef CONFIG_CGROUPS
> static inline void threadgroup_change_begin(struct task_struct *tsk)
> {
> @@ -2384,13 +2384,48 @@ static inline void threadgroup_change_end(struct task_struct *tsk)
> {
> up_read(&tsk->signal->group_rwsem);
> }
> +
> +/**
> + * threadgroup_lock - lock threadgroup
> + * @tsk: member task of the threadgroup to lock
> + *
> + * Lock the threadgroup @tsk belongs to. No new task is allowed to enter
> + * and member tasks aren't allowed to exit (as indicated by PF_EXITING) or
> + * perform exec. This is useful for cases where the threadgroup needs to
> + * stay stable across blockable operations.
> + *
> + * fork and exit explicitly call threadgroup_change_{begin|end}() for
> + * synchronization. This excludes most of do_exit() to ensure that, while
> + * locked, tasks belonging to a locked group are not in the process of
> + * deconstruction - they're either alive or dead.
> + *
> + * During exec, a task goes and puts its thread group through unusual
> + * changes. After de-threading, exclusive access is assumed to resources
> + * which are usually shared by tasks in the same group - e.g. sighand may
> + * be replaced with a new one. Also, the exec'ing task takes over group
> + * leader role including its pid. Exclude these changes while locked by
> + * grabbing cred_guard_mutex which is used to synchronize exec path.
> + */
> static inline void threadgroup_lock(struct task_struct *tsk)
> {
> + /*
> + * exec uses exit for de-threading nesting group_rwsem inside
> + * cred_guard_mutex. Grab cred_guard_mutex first.
> + */
> + mutex_lock(&tsk->signal->cred_guard_mutex);
> down_write(&tsk->signal->group_rwsem);
> }
> +
> +/**
> + * threadgroup_unlock - unlock threadgroup
> + * @tsk: member task of the threadgroup to unlock
> + *
> + * Reverse threadgroup_lock().
> + */
> static inline void threadgroup_unlock(struct task_struct *tsk)
> {
> up_write(&tsk->signal->group_rwsem);
> + mutex_unlock(&tsk->signal->cred_guard_mutex);
> }
> #else
> static inline void threadgroup_change_begin(struct task_struct *tsk) {}
> diff --git a/kernel/exit.c b/kernel/exit.c
> index d0b7d98..b2cb562 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -936,6 +936,14 @@ NORET_TYPE void do_exit(long code)
> schedule();
> }
>
> + /*
> + * @tsk's threadgroup is going through changes - lock out users
> + * which expect stable threadgroup. Do this before actually
> + * starting tearing down @tsk so that locked threadgroup has either
> + * alive or dead tasks, not something inbetween.
> + */
> + threadgroup_change_begin(tsk);
> +
I still wonder why there is a so big coverage of this lock. I mean
why is it called right before exit_irq_thread() and released so late.
All we want is to lock cgroup_exit() I think, after which tasks can't be
migrated.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] PM: HIBERNATION: skip the swap size check if the snapshot image size is anticipative
From: Barry Song @ 2011-11-25 13:42 UTC (permalink / raw)
To: Matthieu CASTET
Cc: Xiangzhen Ye, linux-kernel@vger.kernel.org,
workgroup.linux@csr.com, linux-arm-kernel@lists.infradead.org,
Barry Song, linux-pm@lists.linux-foundation.org
In-Reply-To: <4ECF6C36.8000702@parrot.com>
Hi Matthieu,
2011/11/25 Matthieu CASTET <matthieu.castet@parrot.com>
>
> Hi,
>
> but did embedded system will really use swap interface ?
if you self-define the hibernation flow, you don't need swap. but for
embedded system, we actually can still use the standard flow, swapon
before hibernation, and swapoff after restore. so the swap partition
only works as a partition to save used pages based on swap format. it
is not a runtime swap partition at all.
>
> Often they don't have swap and can already save suspend image in disk/nand :
> - SNAPSHOT_CREATE_IMAGE
> - read it from /dev/snapshot and write it to a file.
>
> The only problem with the current interface, is that you can't SNAPSHOT_UNFREEZE
> before reading the image.
> But this is need for some filesystem that have thread.
>
>
> For example we have a demo where we save the suspend image in a ubi volume and
> restore it from bootloader.
yes. we can definitely resume snapshot from bootloader directly, that
depends on your self-defined hibernation flow. if that works well, it
will save about 0.5s kernel boot time for a typical cortex-a9 ARM 1Ghz
system.
but you still need to take care you only compress and save used pages
but not all pages. sync, drop page caches and kill non-system
processes will help much to decrease hibernation image size. and
standard hibernation flow will only save used pages, that is very
important for decreasing snapshot size and restore faster.
i am happy to hear you have a demo. do you have a website to show your
demo and techinical details?
>
>
> Matthieu
>
-barry
^ permalink raw reply
* Re: [PATCH v2] PM: HIBERNATION: skip the swap size check if the snapshot image size is anticipative
From: Matthieu CASTET @ 2011-11-25 10:21 UTC (permalink / raw)
To: Barry Song
Cc: Xiangzhen Ye, linux-kernel@vger.kernel.org,
workgroup.linux@csr.com, Barry Song,
linux-pm@lists.linux-foundation.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1322208422-12346-1-git-send-email-Barry.Song@csr.com>
Hi,
but did embedded system will really use swap interface ?
Often they don't have swap and can already save suspend image in disk/nand :
- SNAPSHOT_CREATE_IMAGE
- read it from /dev/snapshot and write it to a file.
The only problem with the current interface, is that you can't SNAPSHOT_UNFREEZE
before reading the image.
But this is need for some filesystem that have thread.
For example we have a demo where we save the suspend image in a ubi volume and
restore it from bootloader.
Matthieu
Barry Song a écrit :
> From: Barry Song <baohua.song@csr.com>
>
> Current swsusp requires swap partitions even larger than real saved pages
> based on the worst compression ratio:
> but for an embedded system, which has limited storage space, then it might
> can't give the large partition to save snapshot.
> In the another way, some embedded systems can definitely know the most size
> needed for snapshot since they run some specific application lists.
> So this patch provides the possibility for users to tell kernel even
> the system has a little snapshot partition, but it is still enough.
> For example, if the system need to save 120MB memory, origin swsusp will require
> a 130MB partition to save snapshot. but if users know 30MB is enough for them(
> compressed image will be less than 30MB), they just make a 30MB partition by
> echo 0 > /sys/power/check_swap_size
>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> Cc: Xiangzhen Ye <Xiangzhen.Ye@csr.com>
> ---
> -v2:drop swap_enough bootargs and use /sys/power/check_swap_size node
>
> Documentation/power/interface.txt | 5 +++++
> kernel/power/hibernate.c | 22 ++++++++++++++++++++++
> kernel/power/power.h | 2 ++
> kernel/power/swap.c | 9 +++++++++
> 4 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/power/interface.txt b/Documentation/power/interface.txt
> index c537834..5e205f0 100644
> --- a/Documentation/power/interface.txt
> +++ b/Documentation/power/interface.txt
> @@ -47,6 +47,11 @@ Writing to this file will accept one of
> 'testproc'
> 'test'
>
> +/sys/power/check_swap_size controls whether we can skip checking the swap
> +partition size by worst compression ratio. If users know the swap partition
> +is enough for compressed snapshot, write 0 to /sys/power/check_swap_size.
> +It is useful for an embedded system with known running softwares.
> +
> /sys/power/image_size controls the size of the image created by
> the suspend-to-disk mechanism. It can be written a string
> representing a non-negative integer that will be used as an upper
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 1c53f7f..5552473 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -1024,11 +1024,33 @@ static ssize_t reserved_size_store(struct kobject *kobj,
>
> power_attr(reserved_size);
>
> +static ssize_t check_swap_size_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%d\n", check_swap_size);
> +}
> +
> +static ssize_t check_swap_size_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t n)
> +{
> + int check_size;
> +
> + if (sscanf(buf, "%d", &check_size) == 1) {
> + check_swap_size = check_size;
> + return n;
> + }
> +
> + return -EINVAL;
> +}
> +
> +power_attr(check_swap_size);
> +
> static struct attribute * g[] = {
> &disk_attr.attr,
> &resume_attr.attr,
> &image_size_attr.attr,
> &reserved_size_attr.attr,
> + &check_swap_size_attr.attr,
> NULL,
> };
>
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 23a2db1..4f0fa78 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -74,6 +74,8 @@ static struct kobj_attribute _name##_attr = { \
>
> /* Preferred image size in bytes (default 500 MB) */
> extern unsigned long image_size;
> +/* If 0, skip checking whether the swap size is enough for compressed snapshot */
> +extern int check_swap_size;
> /* Size of memory reserved for drivers (default SPARE_PAGES x PAGE_SIZE) */
> extern unsigned long reserved_size;
> extern int in_suspend;
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 11a594c..db90195 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -37,6 +37,12 @@
> #define HIBERNATE_SIG "S1SUSPEND"
>
> /*
> + * if users know swap partitions are enough for compressed snapshots,
> + * echo 0 > /sys/power/check_swap_size
> + */
> +int check_swap_size = 1;
> +
> +/*
> * The swap map is a data structure used for keeping track of each page
> * written to a swap partition. It consists of many swap_map_page
> * structures that contain each an array of MAP_PAGE_ENTRIES swap entries.
> @@ -772,6 +778,9 @@ static int enough_swap(unsigned int nr_pages, unsigned int flags)
> unsigned int free_swap = count_swap_pages(root_swap, 1);
> unsigned int required;
>
> + if (!check_swap_size)
> + return 1;
> +
> pr_debug("PM: Free swap pages: %u\n", free_swap);
>
> required = PAGES_FOR_IO + ((flags & SF_NOCOMPRESS_MODE) ?
^ permalink raw reply
* [PATCH v2] PM: HIBERNATION: skip the swap size check if the snapshot image size is anticipative
From: Barry Song @ 2011-11-25 8:07 UTC (permalink / raw)
To: pavel, rjw, linux-pm
Cc: workgroup.linux, Xiangzhen Ye, linux-kernel, linux-arm-kernel,
Barry Song
From: Barry Song <baohua.song@csr.com>
Current swsusp requires swap partitions even larger than real saved pages
based on the worst compression ratio:
but for an embedded system, which has limited storage space, then it might
can't give the large partition to save snapshot.
In the another way, some embedded systems can definitely know the most size
needed for snapshot since they run some specific application lists.
So this patch provides the possibility for users to tell kernel even
the system has a little snapshot partition, but it is still enough.
For example, if the system need to save 120MB memory, origin swsusp will require
a 130MB partition to save snapshot. but if users know 30MB is enough for them(
compressed image will be less than 30MB), they just make a 30MB partition by
echo 0 > /sys/power/check_swap_size
Signed-off-by: Barry Song <Baohua.Song@csr.com>
Cc: Xiangzhen Ye <Xiangzhen.Ye@csr.com>
---
-v2:drop swap_enough bootargs and use /sys/power/check_swap_size node
Documentation/power/interface.txt | 5 +++++
kernel/power/hibernate.c | 22 ++++++++++++++++++++++
kernel/power/power.h | 2 ++
kernel/power/swap.c | 9 +++++++++
4 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/Documentation/power/interface.txt b/Documentation/power/interface.txt
index c537834..5e205f0 100644
--- a/Documentation/power/interface.txt
+++ b/Documentation/power/interface.txt
@@ -47,6 +47,11 @@ Writing to this file will accept one of
'testproc'
'test'
+/sys/power/check_swap_size controls whether we can skip checking the swap
+partition size by worst compression ratio. If users know the swap partition
+is enough for compressed snapshot, write 0 to /sys/power/check_swap_size.
+It is useful for an embedded system with known running softwares.
+
/sys/power/image_size controls the size of the image created by
the suspend-to-disk mechanism. It can be written a string
representing a non-negative integer that will be used as an upper
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 1c53f7f..5552473 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -1024,11 +1024,33 @@ static ssize_t reserved_size_store(struct kobject *kobj,
power_attr(reserved_size);
+static ssize_t check_swap_size_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", check_swap_size);
+}
+
+static ssize_t check_swap_size_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t n)
+{
+ int check_size;
+
+ if (sscanf(buf, "%d", &check_size) == 1) {
+ check_swap_size = check_size;
+ return n;
+ }
+
+ return -EINVAL;
+}
+
+power_attr(check_swap_size);
+
static struct attribute * g[] = {
&disk_attr.attr,
&resume_attr.attr,
&image_size_attr.attr,
&reserved_size_attr.attr,
+ &check_swap_size_attr.attr,
NULL,
};
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 23a2db1..4f0fa78 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -74,6 +74,8 @@ static struct kobj_attribute _name##_attr = { \
/* Preferred image size in bytes (default 500 MB) */
extern unsigned long image_size;
+/* If 0, skip checking whether the swap size is enough for compressed snapshot */
+extern int check_swap_size;
/* Size of memory reserved for drivers (default SPARE_PAGES x PAGE_SIZE) */
extern unsigned long reserved_size;
extern int in_suspend;
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 11a594c..db90195 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -37,6 +37,12 @@
#define HIBERNATE_SIG "S1SUSPEND"
/*
+ * if users know swap partitions are enough for compressed snapshots,
+ * echo 0 > /sys/power/check_swap_size
+ */
+int check_swap_size = 1;
+
+/*
* The swap map is a data structure used for keeping track of each page
* written to a swap partition. It consists of many swap_map_page
* structures that contain each an array of MAP_PAGE_ENTRIES swap entries.
@@ -772,6 +778,9 @@ static int enough_swap(unsigned int nr_pages, unsigned int flags)
unsigned int free_swap = count_swap_pages(root_swap, 1);
unsigned int required;
+ if (!check_swap_size)
+ return 1;
+
pr_debug("PM: Free swap pages: %u\n", free_swap);
required = PAGES_FOR_IO + ((flags & SF_NOCOMPRESS_MODE) ?
--
1.7.1
Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog
^ permalink raw reply related
* Re: [PATCH UPDATED 03/10] threadgroup: extend threadgroup_lock() to cover exit and exec
From: Linus Torvalds @ 2011-11-25 4:02 UTC (permalink / raw)
To: Tejun Heo
Cc: akpm, fweisbec, containers, lizf, linux-kernel, oleg, linux-pm,
paul, kamezawa.hiroyu
In-Reply-To: <20111124225054.GA14828@google.com>
On Thu, Nov 24, 2011 at 2:50 PM, Tejun Heo <tj@kernel.org> wrote:
>
> Linus, this is something being scheduled for the next merge window.
> It extends threadgroup locking which cgroup used to do to exclude only
> fork path so that it includes exit and exec paths. threadgroup
> locking is used by cgroup to implement process-scope cgroup migration.
>
> Migration happens in multiple steps, at each of which the matching
> method of each cgroup plugin is called. When a whole process is being
> migrated, methods being called at those different steps expect to see
> consistent image of the thread group.
Ugh.
I see why you want to do this, but I really don't much like some of it.
I've got a few questions:
- do you even *need* two separate locks at all? If you extend the
threadgroup_fork_lock to cover exec/exit too (and rename it), then why
does that separate cred_guard_mutex make sense at all?
Taking two locks for the common exit case seems disgusting. What do
the separate locks buy us?
- could we possible avoid the lock(s) entirely for the
single-threaded case? The fact that ptrace wants to serialize makes me
say "maybe we can't avoid it", but I thought I'd ask. Even if we do
need/want two separate locks, do we really want to take them both for
the case that shouldn't really need even a single one?
Hmm? Simplifying the locking rules is always a good idea, and I think
this seems to make some of it more straightforward, but at the same
time I shudder when I look at some of the patches in the series that
nest locking three locks deep. Ugh.
Linus
^ permalink raw reply
* Re: [PATCHSET] cgroup: stable threadgroup during attach & subsys methods consolidation
From: Tejun Heo @ 2011-11-24 22:51 UTC (permalink / raw)
To: paul, rjw, lizf
Cc: fweisbec, containers, linux-kernel, oleg, linux-pm, akpm,
kamezawa.hiroyu
In-Reply-To: <1320191193-8110-1-git-send-email-tj@kernel.org>
Series committed to cgroup/for-3.3 with updated patch and pushed out
to linux-next.
Thank you.
--
tejun
^ permalink raw reply
* [PATCH UPDATED 03/10] threadgroup: extend threadgroup_lock() to cover exit and exec
From: Tejun Heo @ 2011-11-24 22:50 UTC (permalink / raw)
To: paul, rjw, lizf, Linus Torvalds
Cc: fweisbec, containers, linux-kernel, oleg, linux-pm, akpm,
kamezawa.hiroyu
In-Reply-To: <1320191193-8110-4-git-send-email-tj@kernel.org>
threadgroup_lock() protected only protected against new addition to
the threadgroup, which was inherently somewhat incomplete and
problematic for its only user cgroup. On-going migration could race
against exec and exit leading to interesting problems - the symmetry
between various attach methods, task exiting during method execution,
->exit() racing against attach methods, migrating task switching basic
properties during exec and so on.
This patch extends threadgroup_lock() such that it protects against
all three threadgroup altering operations - fork, exit and exec. For
exit, threadgroup_change_begin/end() calls are added to exit path.
For exec, threadgroup_[un]lock() are updated to also grab and release
cred_guard_mutex.
With this change, threadgroup_lock() guarantees that the target
threadgroup will remain stable - no new task will be added, no new
PF_EXITING will be set and exec won't happen.
The next patch will update cgroup so that it can take full advantage
of this change.
-v2: beefed up comment as suggested by Frederic.
Signed-off-by: Tejun Heo <tj@kernel.org>
Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Acked-by: Li Zefan <lizf@cn.fujitsu.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Paul Menage <paul@paulmenage.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
Linus, this is something being scheduled for the next merge window.
It extends threadgroup locking which cgroup used to do to exclude only
fork path so that it includes exit and exec paths. threadgroup
locking is used by cgroup to implement process-scope cgroup migration.
Migration happens in multiple steps, at each of which the matching
method of each cgroup plugin is called. When a whole process is being
migrated, methods being called at those different steps expect to see
consistent image of the thread group.
cgroup currently only locks out addition of new tasks into the thread
group and holds extra ref to member tasks. This mandates all cgroup
plugins to deal with tasks being torn down and exec morphing the
threadgroup. This patch extends the scope of threadgroup locking such
that the thread group is guaranteed to be stable (no new task, tasks
are either live or dead and no exec morphing) while locked.
This is part of changes to clean up cgroup methods and iron out corner
case fuzziness and should make difficult-to-reproduce race conditions
less likely and cgroup plugins easier to implement and verify.
The synchronization is strictly per-threadgroup and goes away if
cgroup is not configured.
The whole series is avilable at
http://thread.gmane.org/gmane.linux.kernel.containers/21716
git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.3
Thank you.
include/linux/sched.h | 47 +++++++++++++++++++++++++++++++++++++++++------
kernel/exit.c | 19 +++++++++++++++----
2 files changed, 56 insertions(+), 10 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index c5acbce..481c5ed 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -635,11 +635,12 @@ struct signal_struct {
#endif
#ifdef CONFIG_CGROUPS
/*
- * The group_rwsem prevents threads from forking with
- * CLONE_THREAD while held for writing. Use this for fork-sensitive
- * threadgroup-wide operations. It's taken for reading in fork.c in
- * copy_process().
- * Currently only needed write-side by cgroups.
+ * group_rwsem prevents new tasks from entering the threadgroup and
+ * member tasks from exiting. fork and exit paths are protected
+ * with this rwsem using threadgroup_change_begin/end(). Users
+ * which require threadgroup to remain stable should use
+ * threadgroup_[un]lock() which also takes care of exec path.
+ * Currently, cgroup is the only user.
*/
struct rw_semaphore group_rwsem;
#endif
@@ -2374,7 +2375,6 @@ static inline void unlock_task_sighand(struct task_struct *tsk,
spin_unlock_irqrestore(&tsk->sighand->siglock, *flags);
}
-/* See the declaration of group_rwsem in signal_struct. */
#ifdef CONFIG_CGROUPS
static inline void threadgroup_change_begin(struct task_struct *tsk)
{
@@ -2384,13 +2384,48 @@ static inline void threadgroup_change_end(struct task_struct *tsk)
{
up_read(&tsk->signal->group_rwsem);
}
+
+/**
+ * threadgroup_lock - lock threadgroup
+ * @tsk: member task of the threadgroup to lock
+ *
+ * Lock the threadgroup @tsk belongs to. No new task is allowed to enter
+ * and member tasks aren't allowed to exit (as indicated by PF_EXITING) or
+ * perform exec. This is useful for cases where the threadgroup needs to
+ * stay stable across blockable operations.
+ *
+ * fork and exit explicitly call threadgroup_change_{begin|end}() for
+ * synchronization. This excludes most of do_exit() to ensure that, while
+ * locked, tasks belonging to a locked group are not in the process of
+ * deconstruction - they're either alive or dead.
+ *
+ * During exec, a task goes and puts its thread group through unusual
+ * changes. After de-threading, exclusive access is assumed to resources
+ * which are usually shared by tasks in the same group - e.g. sighand may
+ * be replaced with a new one. Also, the exec'ing task takes over group
+ * leader role including its pid. Exclude these changes while locked by
+ * grabbing cred_guard_mutex which is used to synchronize exec path.
+ */
static inline void threadgroup_lock(struct task_struct *tsk)
{
+ /*
+ * exec uses exit for de-threading nesting group_rwsem inside
+ * cred_guard_mutex. Grab cred_guard_mutex first.
+ */
+ mutex_lock(&tsk->signal->cred_guard_mutex);
down_write(&tsk->signal->group_rwsem);
}
+
+/**
+ * threadgroup_unlock - unlock threadgroup
+ * @tsk: member task of the threadgroup to unlock
+ *
+ * Reverse threadgroup_lock().
+ */
static inline void threadgroup_unlock(struct task_struct *tsk)
{
up_write(&tsk->signal->group_rwsem);
+ mutex_unlock(&tsk->signal->cred_guard_mutex);
}
#else
static inline void threadgroup_change_begin(struct task_struct *tsk) {}
diff --git a/kernel/exit.c b/kernel/exit.c
index d0b7d98..b2cb562 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -936,6 +936,14 @@ NORET_TYPE void do_exit(long code)
schedule();
}
+ /*
+ * @tsk's threadgroup is going through changes - lock out users
+ * which expect stable threadgroup. Do this before actually
+ * starting tearing down @tsk so that locked threadgroup has either
+ * alive or dead tasks, not something inbetween.
+ */
+ threadgroup_change_begin(tsk);
+
exit_irq_thread();
exit_signals(tsk); /* sets PF_EXITING */
@@ -1018,10 +1026,6 @@ NORET_TYPE void do_exit(long code)
kfree(current->pi_state_cache);
#endif
/*
- * Make sure we are holding no locks:
- */
- debug_check_no_locks_held(tsk);
- /*
* We can do this unlocked here. The futex code uses this flag
* just to verify whether the pi state cleanup has been done
* or not. In the worst case it loops once more.
@@ -1038,6 +1042,13 @@ NORET_TYPE void do_exit(long code)
preempt_disable();
exit_rcu();
+
+ /*
+ * Release threadgroup and make sure we are holding no locks.
+ */
+ threadgroup_change_end(tsk);
+ debug_check_no_locks_held(tsk);
+
/* causes final put_task_struct in finish_task_switch(). */
tsk->state = TASK_DEAD;
schedule();
--
1.7.3.1
^ permalink raw reply related
* Re: [PATCH 03/10] threadgroup: extend threadgroup_lock() to cover exit and exec
From: Tejun Heo @ 2011-11-24 21:22 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: akpm, containers, lizf, linux-kernel, oleg, linux-pm, paul,
kamezawa.hiroyu
In-Reply-To: <20111123140139.GB10669@somewhere.redhat.com>
Hello, Frederic.
On Wed, Nov 23, 2011 at 03:02:05PM +0100, Frederic Weisbecker wrote:
> This:
>
> static inline void threadgroup_lock(struct task_struct *tsk)
> {
> + /* exec uses exit for de-threading, grab cred_guard_mutex first */
> + mutex_lock(&tsk->signal->cred_guard_mutex);
> down_write(&tsk->signal->group_rwsem);
>
> is really not obvious.
>
> Just point out we want to synchronize against the leader, pid and the sighand
> that may change concurrently.
Sure thing. Will beef up the comments.
> > > Also note this is currently protected by the tasklist readlock. Cred
> > > guard mutex is certainly better, I just don't remember if you remove
> > > the tasklist lock in a further patch.
> >
> > I don't remove tasklist_lock.
>
> I believe you can do it after using the cred guard mutex. That needs to
> be double checked though. I think it was mostly there to keep while_each_thread()
> stable non-racy against leader change. cred_guard_mutex should handle that
> now.
Yes, probably, but I still don't think removing that is a good idea.
We're walking tasklist in a rather cold path, IMHO it's better to keep
the locking obvious.
> > So, to me, what seems more important is how to make it easier for each
> > cgroup client instead of what's the minimal that's necessary right
> > now.
> >
> > Heh, did I make any sense? :)
>
> Yep, fine for me :)
> Just wanted to ensure I (and others) understood and identified well the issues
> and the fixes.
Yeap, fair enough. Will update the patches, repost and push them
through cgroup/for-3.3, and move onto other pending patchsets.
Thank you.
--
tejun
^ permalink raw reply
* Re: [PATCH 4/6] OMAP3: cpuidle: next C-state decision depends on the PM QoS MPU and CORE constraints
From: Kevin Hilman @ 2011-11-23 19:43 UTC (permalink / raw)
To: Jean Pihet; +Cc: Linux PM mailing list, linux-omap, Jean Pihet
In-Reply-To: <CAORVsuV5bV-p0kxhh8g4T076Dg3Mj89X_=4E+BpCSTdLiUmNZA@mail.gmail.com>
Jean Pihet <jean.pihet@newoldbits.com> writes:
> On Thu, Nov 17, 2011 at 10:29 PM, Kevin Hilman <khilman@ti.com> wrote:
>> jean.pihet@newoldbits.com writes:
>>
>>> From: Jean Pihet <j-pihet@ti.com>
>>>
>>> The MPU latency figures for cpuidle include the MPU itself and also
>>> the peripherals needed for the MPU to execute instructions (e.g.
>>> main memory, caches, IRQ controller, MMU etc). On OMAP3 those
>>> peripherals belong to the MPU and CORE power domains and so the
>>> cpuidle C-states are a combination of MPU and CORE states.
>>>
>>> This patch implements the relation between the cpuidle and per-
>>> device PM QoS frameworks in the OMAP3 specific idle callbacks.
>>>
>>> The chosen C-state shall satisfy the following conditions:
>>> . the 'valid' field is enabled,
>>> . it satisfies the enable_off_mode flag,
>>
>> Not directly related to this patch, but is there any reason to keep the
>> 'enable_off_mode' flag after this series?
> enable_off_mode could be removed completely after this series unless
> there is a need to prevent OFF mode for debug reasons.
Great.
For debug reasons, we can just as easily set constraints to prevent off
mode, so I would like to see it disappear.
Kevin
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH 3/6] OMAP: PM: register to the per-device PM QoS framework
From: Kevin Hilman @ 2011-11-23 19:42 UTC (permalink / raw)
To: Jean Pihet; +Cc: linux-omap, Linux PM mailing list, Jean Pihet
In-Reply-To: <CAORVsuURENRLO-FAhau-eS2VmfZrG=BOgih_SPHUSD3Nv3j96w@mail.gmail.com>
Jean Pihet <jean.pihet@newoldbits.com> writes:
> On Thu, Nov 17, 2011 at 10:24 PM, Kevin Hilman <khilman@ti.com> wrote:
>> jean.pihet@newoldbits.com writes:
>>
>>> From: Jean Pihet <j-pihet@ti.com>
>>>
>>> Implement the devices wake-up latency constraints using the global
>>> device PM QoS notification handler which applies the constraints to the
>>> underlying layer by calling the corresponding function at hwmod level.
>>>
>>> Tested on OMAP3 Beagleboard and OMAP4 Pandaboard in RET/OFF using wake-up
>>> latency constraints on MPU, CORE and PER.
>>>
>>> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ...
>
>>> +/* Interface to the per-device PM QoS framework */
>>> +static int omap2_dev_pm_qos_handler(struct notifier_block *nb,
>>> + unsigned long new_value,
>>> + void *req)
>>> +{
>>> + struct omap_device *od;
>>> + struct omap_hwmod *oh;
>>> + struct platform_device *pdev;
>>> + struct dev_pm_qos_request *dev_pm_qos_req = req;
>>> +
>>> + pr_debug("OMAP PM CONSTRAINTS: req@0x%p, new_value=%lu\n",
>>
>> s/CONSTRAINTS/constraints/
>> another one below.
> Ok
>
>>
>>> + req, new_value);
>>> +
>>> + /* Look for the platform device for the constraint target device */
>>> + pdev = to_platform_device(dev_pm_qos_req->dev);
>>> +
>>> + /* Try to catch non platform devices */
>>
>> why?
> The constraints targets are the power domains, which you find by
> walking through the chain of structs dev, pdev, omap_device, hwmod and
> finally pwrdm.
OK
>>
>>> + if (pdev->name == NULL) {
>>> + pr_err("%s: Error: platform device for device %s not valid\n",
>>> + __func__, dev_name(dev_pm_qos_req->dev));
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Find the associated omap_device for dev */
>>> + od = container_of(pdev, struct omap_device, pdev);
>>
>> What about devices that are valid platform_devices, but not omap_devices?
> Do you mean that od should be tested for NULL value?
First, it should be using the to_omap_device() helper function from
omap_device.h.
Until v3.2, it was based on container_of() so checking for NULL will not
help. However, as of v3.2, because I decoupled platform_device and
omap_device, checking for a NULL return from to_omap_device() will be a
good enough check.
Kevin
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH 03/10] threadgroup: extend threadgroup_lock() to cover exit and exec
From: Frederic Weisbecker @ 2011-11-23 14:34 UTC (permalink / raw)
To: Tejun Heo
Cc: akpm, containers, lizf, linux-kernel, oleg, linux-pm, paul,
kamezawa.hiroyu
In-Reply-To: <20111121220326.GM25776@google.com>
On Mon, Nov 21, 2011 at 02:03:26PM -0800, Tejun Heo wrote:
> On Mon, Nov 14, 2011 at 02:54:08PM +0100, Frederic Weisbecker wrote:
> > > Also note this is currently protected by the tasklist readlock. Cred guard mutex is
> > > certainly better, I just don't remember if you remove the tasklist lock in a
> > > further patch.
> >
> > Ah recalling what Ben Blum said, we also need the leader to stay stable because it
> > is excpected to be passed in ->can_attach(), ->attach(), ->cancel_attach(), ...
> > Although that's going to change after your patches that pass a flex array.
>
> Not really without locking out exec. The thing is whoever is exec'ing
> will be the leader and we can't guarantee that the first in the flex
> array is always the leader. One method may see it as the leader, the
> next one might not.
Exactly!
^ permalink raw reply
* Re: [PATCH 03/10] threadgroup: extend threadgroup_lock() to cover exit and exec
From: Frederic Weisbecker @ 2011-11-23 14:02 UTC (permalink / raw)
To: Tejun Heo
Cc: akpm, containers, lizf, linux-kernel, oleg, linux-pm, paul,
kamezawa.hiroyu
In-Reply-To: <20111121215839.GL25776@google.com>
On Mon, Nov 21, 2011 at 01:58:39PM -0800, Tejun Heo wrote:
> > but IMHO the races involved in exit and exec
> > are too different, specific and complicated on their own to be solved in a
> > single one patch. This should be split in two things.
> >
> > The specific problems and their fix need to be described more in detail
> > in the changelog because the issues are very tricky.
> >
> > The exec case:
> >
> > IIUC, the race in exec is about the group leader that can be changed
> > to become the exec'ing thread, making while_each_thread() unsafe.
>
> Not only that, pid changes, sighand struct may get swapped, and other
> weird things which aren't usually expected for a live task happen.
> It's basically semi-killed and then resurrected.
Right. I've no problem with the fact you want to make the threadgroup
more stable against subsystem attachment. Just please put more
detailed comments about what you are protecting. It took me quite
some time to precisely identify the race involved in that game in
order to consciously review your patches. And I'm thinking about
the next people who will work on this piece of code.
This:
static inline void threadgroup_lock(struct task_struct *tsk)
{
+ /* exec uses exit for de-threading, grab cred_guard_mutex first */
+ mutex_lock(&tsk->signal->cred_guard_mutex);
down_write(&tsk->signal->group_rwsem);
is really not obvious.
Just point out we want to synchronize against the leader, pid and the sighand
that may change concurrently.
> > We also have other things happening there like all the other threads
> > in the group that get killed, but that should be handled by the threadgroup_change_begin()
> > you put in the exit path.
> > The old leader is also killed but release_task() -> __unhash_process() is called
> > for it manually from the exec path. However this thread too should be covered by your
> > synchronisation in exit().
> >
> > So after your protection in the exit path, the only thing to protect against in exec
> > is that group_leader that can change concurrently. But I may be missing something in the picture.
>
> Hmm... an exec'ing task goes through transitions which usually aren't
> allowed to happen. It assumes exclusive access to group-shared data
> structures and may ditch the current one and create a new one.
>
> Sure, it's possible to make every cgroup method correct w.r.t. all
> those via explicit locking or by being more careful but I don't see
> much point in such excercise. If the code is sitting in some hot path
> and the added exclusion is gonna add significant amount of contention,
> sure, we should trade off easiness for better performance /
> scalability but this is for cgroup modifications, a fundamentally cold
> path, and the added locking is per-task or per-threadgroup. I don't
> see any reason not to be easy here.
Sure. I have no problem with that. I just wish we can precisely identify
what we are protecting against, and not have a foggy locking. Especially
when it's about such a very core issue.
> Please read on.
>
> > Also note this is currently protected by the tasklist readlock. Cred
> > guard mutex is certainly better, I just don't remember if you remove
> > the tasklist lock in a further patch.
>
> I don't remove tasklist_lock.
I believe you can do it after using the cred guard mutex. That needs to
be double checked though. I think it was mostly there to keep while_each_thread()
stable non-racy against leader change. cred_guard_mutex should handle that
now.
>
> > The exit case:
> >
> > There the issue is about racing against cgroup_exit() where the task can be
> > assigned to the root cgroup. Is this really a problem in fact? I don't know
> > if subsystems care about that. Perhaps some of them are buggy in that
> > regard. At least the task counter handles that and it needs a
> > ->cancel_attach_task() for this purpose in the case the migration fails due to exit.
> > Not sure about others. A real synchronization against exit is less error prone for sure.
> > In the end that's probably a win.
>
> This is the same story. Yes, we can narrow the locking and try to
> make sure everyone handles partially destroyed tasks properly in all
> methods, but for what? If we can give stable threadgroups to all
> cgroup methods without introducing performance or scalability
> bottleneck, that's the right thing to do. Please also note that bugs
> stemming from failure to handle those corner cases properly will be
> subtle, difficult to reproduce and track down.
Agreed.
> In general, for any subsystem with pluggable interface, I think it's
> best to put as much complexity as possible into the core layer to make
> things eaiser for its customers. It becomes excruciatingly painful if
> the API invites subtle corner case bugs and the subsystem grows
> several dozen clients down the road.
Sure.
>
> So, to me, what seems more important is how to make it easier for each
> cgroup client instead of what's the minimal that's necessary right
> now.
>
> Heh, did I make any sense? :)
Yep, fine for me :)
Just wanted to ensure I (and others) understood and identified well the issues
and the fixes.
^ permalink raw reply
* Re: 3.2-rc2+: Reported regressions from 3.0 and 3.1
From: Rafał Miłecki @ 2011-11-23 7:37 UTC (permalink / raw)
To: Linus Torvalds, stephen hemminger
Cc: Linux SCSI List, Linux ACPI, Network Development,
Linux Wireless List, Linux Kernel Mailing List, DRI,
Florian Mickler, Andrew Morton, Kernel Testers List,
Linux PM List, Maciej Rutecki
In-Reply-To: <CA+55aFyy19VYSdZW0+jNxAb8ix0xpX2j9YFw9oQi3jm3+mDEvw@mail.gmail.com>
W dniu 21 listopada 2011 23:22 użytkownik Linus Torvalds
<torvalds@linux-foundation.org> napisał:
> On Mon, Nov 21, 2011 at 1:49 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>
>> Subject : [3.1-rc8 REGRESSION] sky2 hangs machine on turning off or suspending
>> Submitter : Rafał Miłecki <zajec5@gmail.com>
>> Date : 2011-11-09 11:46
>> Message-ID : CACna6ryTdLcWVYgHu=_mRFga1sFivpE_DyZOY-HMmKggkWAJAw@mail.gmail.com
>> References : http://marc.info/?l=linux-netdev&m=132083922228088&w=4
>
> This should be fixed by commit 1401a8008a09 ("sky2: fix hang on
> shutdown (and other irq issues)") in current -git.
This patch doesn't fix my hang.
However git contains also:
sky2: fix hang in napi_disable
This is the one fixing my case.
So the bug is resolved, however I'm a little disappointed noone
ping-ed me about that patches. I've spent some time on bisecting this
issue, expected to get some response :/
--
Rafał
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [PATCH 4/6] OMAP3: cpuidle: next C-state decision depends on the PM QoS MPU and CORE constraints
From: Jean Pihet @ 2011-11-22 19:54 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Linux PM mailing list, Jean Pihet
In-Reply-To: <877h2y30a5.fsf@ti.com>
On Thu, Nov 17, 2011 at 10:29 PM, Kevin Hilman <khilman@ti.com> wrote:
> jean.pihet@newoldbits.com writes:
>
>> From: Jean Pihet <j-pihet@ti.com>
>>
>> The MPU latency figures for cpuidle include the MPU itself and also
>> the peripherals needed for the MPU to execute instructions (e.g.
>> main memory, caches, IRQ controller, MMU etc). On OMAP3 those
>> peripherals belong to the MPU and CORE power domains and so the
>> cpuidle C-states are a combination of MPU and CORE states.
>>
>> This patch implements the relation between the cpuidle and per-
>> device PM QoS frameworks in the OMAP3 specific idle callbacks.
>>
>> The chosen C-state shall satisfy the following conditions:
>> . the 'valid' field is enabled,
>> . it satisfies the enable_off_mode flag,
>
> Not directly related to this patch, but is there any reason to keep the
> 'enable_off_mode' flag after this series?
enable_off_mode could be removed completely after this series unless
there is a need to prevent OFF mode for debug reasons.
>
> Kevin
>
Thanks,
Jean
^ permalink raw reply
* Re: [PATCH 3/6] OMAP: PM: register to the per-device PM QoS framework
From: Jean Pihet @ 2011-11-22 19:52 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Linux PM mailing list, Jean Pihet
In-Reply-To: <87d3cq30ja.fsf@ti.com>
On Thu, Nov 17, 2011 at 10:24 PM, Kevin Hilman <khilman@ti.com> wrote:
> jean.pihet@newoldbits.com writes:
>
>> From: Jean Pihet <j-pihet@ti.com>
>>
>> Implement the devices wake-up latency constraints using the global
>> device PM QoS notification handler which applies the constraints to the
>> underlying layer by calling the corresponding function at hwmod level.
>>
>> Tested on OMAP3 Beagleboard and OMAP4 Pandaboard in RET/OFF using wake-up
>> latency constraints on MPU, CORE and PER.
>>
>> Signed-off-by: Jean Pihet <j-pihet@ti.com>
...
>> +/* Interface to the per-device PM QoS framework */
>> +static int omap2_dev_pm_qos_handler(struct notifier_block *nb,
>> + unsigned long new_value,
>> + void *req)
>> +{
>> + struct omap_device *od;
>> + struct omap_hwmod *oh;
>> + struct platform_device *pdev;
>> + struct dev_pm_qos_request *dev_pm_qos_req = req;
>> +
>> + pr_debug("OMAP PM CONSTRAINTS: req@0x%p, new_value=%lu\n",
>
> s/CONSTRAINTS/constraints/
> another one below.
Ok
>
>> + req, new_value);
>> +
>> + /* Look for the platform device for the constraint target device */
>> + pdev = to_platform_device(dev_pm_qos_req->dev);
>> +
>> + /* Try to catch non platform devices */
>
> why?
The constraints targets are the power domains, which you find by
walking through the chain of structs dev, pdev, omap_device, hwmod and
finally pwrdm.
>
>> + if (pdev->name == NULL) {
>> + pr_err("%s: Error: platform device for device %s not valid\n",
>> + __func__, dev_name(dev_pm_qos_req->dev));
>> + return -EINVAL;
>> + }
>> +
>> + /* Find the associated omap_device for dev */
>> + od = container_of(pdev, struct omap_device, pdev);
>
> What about devices that are valid platform_devices, but not omap_devices?
Do you mean that od should be tested for NULL value?
>
>> + if (od->hwmods_cnt != 1) {
>> + pr_err("%s: Error: No unique hwmod for device %s\n",
>> + __func__, dev_name(dev_pm_qos_req->dev));
>> + return -EINVAL;
>> + }
>> +
>> + /* Find the primary omap_hwmod for dev */
>> + oh = od->hwmods[0];
>> +
>> + pr_debug("OMAP PM CONSTRAINTS: req@0x%p, dev=0x%p, new_value=%lu\n",
>> + req, dev_pm_qos_req->dev, new_value);
>> +
>> + /* Apply the constraint */
>> + return omap_hwmod_set_wkup_lat_constraint(oh, dev_pm_qos_req,
>> + new_value);
>> +}
>> +
>> +static struct notifier_block omap2_dev_pm_qos_notifier = {
>> + .notifier_call = omap2_dev_pm_qos_handler,
>> +};
>> +
>> +static int __init omap2_dev_pm_qos_init(void)
>> +{
>> + int ret;
>> +
>> + ret = dev_pm_qos_add_global_notifier(&omap2_dev_pm_qos_notifier);
>> + if (ret)
>> + WARN(1, KERN_ERR "Cannot add global notifier for dev PM QoS\n");
>
> minor: could use WARN_ON()
Ok
>
>> + return ret;
>> +}
>> +
>> static int __init omap2_common_pm_init(void)
>> {
>> omap2_init_processor_devices();
>> omap_pm_if_init();
>>
>> + /* Register to the per-device PM QoS framework */
>> + omap2_dev_pm_qos_init();
>> +
>> return 0;
>> }
>> postcore_initcall(omap2_common_pm_init);
>
> Kevin
>
Thanks,
Jean
^ permalink raw reply
* Re: [PATCH 1/6] OMAP2+: powerdomain: control power domains next state
From: Jean Pihet @ 2011-11-22 19:44 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Linux PM mailing list, Jean Pihet
In-Reply-To: <87obwa30vx.fsf@ti.com>
Kevin,
On Thu, Nov 17, 2011 at 10:16 PM, Kevin Hilman <khilman@ti.com> wrote:
> jean.pihet@newoldbits.com writes:
>
>> From: Jean Pihet <j-pihet@ti.com>
>>
>> When a PM QoS device latency constraint is requested or removed the
>> PM QoS layer notifies the underlying layer with the updated aggregated
>> constraint value. The constraint is stored in the powerdomain constraints
>> list and then applied to the corresponding power domain.
>> The power domains get the next power state programmed directly in the
>> registers via pwrdm_wakeuplat_update_pwrst.
>>
>> Tested on OMAP3 Beagleboard and OMAP4 Pandaboard in RET/OFF using
>> wake-up latency constraints on MPU, CORE and PER.
>>
>> Signed-off-by: Jean Pihet <j-pihet@ti.com>
>> ---
...
>> @@ -104,6 +106,12 @@ static int _pwrdm_register(struct powerdomain *pwrdm)
>> for (i = 0; i < pwrdm->banks; i++)
>> pwrdm->ret_mem_off_counter[i] = 0;
>>
>> + /* Initialize the per-od wake-up constraints data */
>
> This comment needs an update (they are not per-od, but per pwrdm), or
> could probably be removed, since it doesn't add any value over the code.
Ok to remove it
>
>> + spin_lock_init(&pwrdm->wkup_lat_plist_lock);
>> + plist_head_init(&pwrdm->wkup_lat_plist_head);
>> + pwrdm->wkup_lat_next_state = PWRDM_POWER_OFF;
>> +
>> + /* Initialize the pwrdm state */
>> pwrdm_wait_transition(pwrdm);
>> pwrdm->state = pwrdm_read_pwrst(pwrdm);
>> pwrdm->state_counter[pwrdm->state] = 1;
...
>> +int pwrdm_set_wkup_lat_constraint(struct powerdomain *pwrdm, void *cookie,
>> + long min_latency)
>> +{
>> + struct pwrdm_wkup_constraints_entry *user = NULL, *new_user = NULL;
>> + int ret = 0, free_new_user = 0, free_node = 0;
>> + long value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
>> + unsigned long flags;
>> +
>> + pr_debug("powerdomain: %s: pwrdm %s, cookie=0x%p, min_latency=%ld\n",
>> + __func__, pwrdm->name, cookie, min_latency);
>> +
>> + if (min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE) {
>> + new_user = kzalloc(sizeof(struct pwrdm_wkup_constraints_entry),
>> + GFP_KERNEL);
>> + if (!new_user) {
>> + pr_err("%s: FATAL ERROR: kzalloc failed\n", __func__);
>> + return -ENOMEM;
>> + }
>> + free_new_user = 1;
>> + }
>> +
>> + spin_lock_irqsave(&pwrdm->wkup_lat_plist_lock, flags);
>> +
>> + /* Manage the constraints list */
>> + ret = _pwrdm_update_wakeuplat_list(pwrdm, cookie, min_latency,
>> + user, new_user,
>> + &free_new_user, &free_node);
>> +
>> + /* Find the aggregated constraint value from the list */
>> + if (!ret)
>> + if (!plist_head_empty(&pwrdm->wkup_lat_plist_head))
>> + value = plist_first(&pwrdm->wkup_lat_plist_head)->prio;
>> +
>> + spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags);
>> +
>> + if (free_node)
>> + kfree(user);
>> +
>> + if (free_new_user)
>> + kfree(new_user);
>
> The alloc/free of these buffers is not terribly obvious when reading. I
Agreed.
> think the code/changelog needs some comments describing the logic
> behind how/when these nodes are allocated and freed.
Ok I will add it.
...
>
> Kevin
>
Thanks,
Jean
^ permalink raw reply
* Re: [PATCHSET] cgroup: stable threadgroup during attach & subsys methods consolidation
From: Tejun Heo @ 2011-11-22 16:20 UTC (permalink / raw)
To: Li Zefan
Cc: akpm, fweisbec, containers, linux-kernel, oleg, linux-pm, paul,
kamezawa.hiroyu
In-Reply-To: <4ECB089C.3080208@cn.fujitsu.com>
On Tue, Nov 22, 2011 at 10:27:40AM +0800, Li Zefan wrote:
> Tejun Heo wrote:
> > On Tue, Nov 01, 2011 at 04:46:23PM -0700, Tejun Heo wrote:
> >> This patchset is combination of the following two patchsets.
> >>
> >> [1] cgroup: extend threadgroup locking
> >> [2] cgroup: introduce cgroup_taskset and consolidate subsys methods, take#2
> >
> > Li, do you agree with the proposed patches? If so, I'll wait for
> > Frederic's response, repost updated series and merge it into the
> > cgroup tree.
> >
>
> I remember I've acked both patchsets in your earlier post. :)
Heh, right, it has been too long and I forgot. I'll add the acks.
Thank you.
--
tejun
^ permalink raw reply
* Re: 3.2-rc2+: Reported regressions from 3.0 and 3.1
From: Konrad Rzeszutek Wilk @ 2011-11-22 13:54 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux SCSI List, Florian Mickler, Network Development,
Linux Wireless List, Linux Kernel Mailing List, DRI, Linux ACPI,
Andrew Morton, Kernel Testers List, Linus Torvalds, Linux PM List,
Maciej Rutecki
In-Reply-To: <201111212249.31196.rjw@sisk.pl>
> Subject : Regression in 3.1 causes Xen to use wrong idle routine
> Submitter : Stefan Bader <stefan.bader@canonical.com>
> Date : 2011-10-26 10:24
> Message-ID : 4EA7DFD1.9060608@canonical.com
> References : http://marc.info/?l=linux-acpi&m=131962467924564&w=2
The patch mentioned in http://mid.gmane.org/20111115144004.GE22675@phenom.dumpdata.com
should do it. But the patch needs an Ack from ACPI/x86 folks.
^ permalink raw reply
* Re: 3.2-rc2+: Reported regressions from 3.0 and 3.1
From: Andrea Arcangeli @ 2011-11-22 12:22 UTC (permalink / raw)
To: Andrew Morton
Cc: Linux SCSI List, Linux ACPI, Network Development,
Linux Wireless List, Linux Kernel Mailing List, DRI,
Florian Mickler, Srivatsa S. Bhat, Linux PM List,
Kernel Testers List, Linus Torvalds, Maciej Rutecki
In-Reply-To: <20111121215918.febd9fd4.akpm@linux-foundation.org>
On Mon, Nov 21, 2011 at 09:59:18PM -0800, Andrew Morton wrote:
> grr, nothing in that patch's changelog indicates that it fixed a
> regression nor that it fixed an infinite blockage of suspend.
Well it's not a recent thing so I didn't flag it as a regression. It
doesn't infinite block it, suspend works fine almost all the time (or
it would have been noticed before), and if you've bad luck and it
doesn't suspend the first time around like someone experienced, if you
try again a bit later it'll work.
> I moved it to my 3.2 queue, thanks.
Thanks!
^ permalink raw reply
* Re: [REGRESSION]: hibernate/sleep regression w/ bisection
From: Andrew Watts @ 2011-11-22 11:40 UTC (permalink / raw)
To: Jerome Glisse
Cc: David Airlie, Dmitry Torokhov, linux-kernel, dri-devel, Tejun Heo,
linux-pm
In-Reply-To: <CAH3drwbtWJcPOixW0gze0e0jbTn_uY0x5E=KY8N-WDcb1t8h2w@mail.gmail.com>
On Mon, Nov 21, 2011 at 04:29:23PM -0500, Jerome Glisse wrote:
> On Mon, Nov 21, 2011 at 4:09 PM, Tejun Heo <tj@kernel.org> wrote:
> >
> > Out of curiosity, did it get somewhere?
> >
> > Thanks.
> >
> > --
> > tejun
> >
>
> Yeah i am pretty sure i know what's going wrong but there is no easy
> fix, there is a deadlock in the kernel when gem wait ioctl is call on
> scanout buffer and that a gpu reset happen
>
> Cheers,
> Jerome
Tejun, I was about to send a similar email asking about this; many thanks
for sharing my interest.
Jerome, sounds encouraging that you think you know what the issue is. What
are the next steps? Please let me know how I can assist on my end (code
validation, etc.).
~ Andy
^ permalink raw reply
* Re: 3.2-rc2+: Reported regressions from 3.0 and 3.1
From: Andy Lutomirski @ 2011-11-22 7:16 UTC (permalink / raw)
To: Linus Torvalds
Cc: Linux SCSI List, Linux ACPI, Network Development,
Linux Wireless List, Linux Kernel Mailing List, DRI,
Florian Mickler, Andrew Morton, Kernel Testers List,
Linux PM List, Maciej Rutecki
In-Reply-To: <CALCETrUpXrsjBLZsZU3u+y7KCDO0LW55QoBQkBqN_BPFZHsrkw@mail.gmail.com>
On Mon, Nov 21, 2011 at 2:34 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Nov 21, 2011 at 2:11 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Mon, Nov 21, 2011 at 1:49 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>>
>>> Subject : [3.1 REGRESSION] Commit 5cec93c216db77c45f7ce970d46283bcb1933884 breaks the Chromium seccomp sandbox
>>> Submitter : Nix <nix@esperi.org.uk>
>>> Date : 2011-11-14 0:40
>>> Message-ID : 8762inleno.fsf@spindle.srvr.nix
>>> References : http://marc.info/?l=linux-kernel&m=132123396226377&w=2
>>
This is apparently fixed in seccompsandbox. See:
https://code.google.com/p/seccompsandbox/issues/detail?id=17
https://code.google.com/p/seccompsandbox/source/detail?r=178
Unless someone objects, I'll consider this to not be a kernel
regression worth fixing.
--Andy
^ permalink raw reply
* Re: 3.2-rc2+: Reported regressions from 3.0 and 3.1
From: Andrew Morton @ 2011-11-22 5:59 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Andrea Arcangeli, Linux SCSI List, Linux ACPI,
Network Development, Linux Wireless List,
Linux Kernel Mailing List, DRI, Florian Mickler, Linux PM List,
Kernel Testers List, Linus Torvalds, Maciej Rutecki
In-Reply-To: <4ECB37E4.6020204@linux.vnet.ibm.com>
On Tue, 22 Nov 2011 11:19:24 +0530 "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> > Subject : khugepaged blocks suspend2ram (3.2.0-rc1-00252-g8f042aa)
> > Submitter : Sergei Trofimovich <slyich@gmail.com>
> > Date : 2011-11-12 10:48
> > Message-ID : 20111112104859.7744b282@sf.home
> > References : https://lkml.org/lkml/2011/11/12/11
> >
>
> Andrea's patch already fixes this issue, which was reported first by
> Jiri Slaby. https://lkml.org/lkml/2011/11/11/93
> I remember Andrew Morton taking this patch in his -mm tree. But it is
> not in mainline yet. So can we consider this closed or not?
grr, nothing in that patch's changelog indicates that it fixed a
regression nor that it fixed an infinite blockage of suspend.
I moved it to my 3.2 queue, thanks.
^ permalink raw reply
* Re: 3.2-rc2+: Reported regressions from 3.0 and 3.1
From: Srivatsa S. Bhat @ 2011-11-22 5:49 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux SCSI List, Linux ACPI, Network Development,
Linux Wireless List, Linux Kernel Mailing List, DRI,
Florian Mickler, Andrew Morton, Kernel Testers List,
Linus Torvalds, Linux PM List, Maciej Rutecki
In-Reply-To: <201111212249.31196.rjw@sisk.pl>
On 11/22/2011 03:19 AM, Rafael J. Wysocki wrote:
> This message contains a list of some regressions from 3.0 and 3.1
> for which there are no fixes in the mainline known to the tracking team.
> If any of them have been fixed already, please let us know.
>
> If you know of any other unresolved regressions from 3.0 and 3.1, please let us
> know either and we'll add them to the list. Also, please let us know if any of
> the entries below are invalid.
>
> The entries below are simplified and the statistics are not present due to the
> continuing Bugzilla outage.
>
> Subject : iwlagn is getting very shaky
> Submitter : Norbert Preining <preining@logic.at>
> Date : 2011-10-19 6:01
> Message-ID : 20111019060108.GA11588@gamma.logic.tuwien.ac.at
> References : http://marc.info/?l=linux-kernel&m=131914553920614&w=2
>
> Subject : Regression: "irqpoll" hasn't been working for me since March 16 IRQ
> Submitter : Edward Donovan <edward.donovan@numble.net>
> Date : 2011-10-19 22:09
> Message-ID : CADdbW+HXdCPfJu2RTF6zz+ujCmiu_dmZwL2iScuF53p=AaZ1Uw@mail.gmail.com
> References : http://marc.info/?l=linux-kernel&m=131914554220679&w=2
>
> Subject : Regression in 3.1 causes Xen to use wrong idle routine
> Submitter : Stefan Bader <stefan.bader@canonical.com>
> Date : 2011-10-26 10:24
> Message-ID : 4EA7DFD1.9060608@canonical.com
> References : http://marc.info/?l=linux-acpi&m=131962467924564&w=2
>
> Subject : 3.1+ iwlwifi lockup
> Submitter : Dave Jones <davej@redhat.com>
> Date : 2011-10-31 14:34
> Message-ID : 20111031143408.GA17152@redhat.com
> References : http://marc.info/?l=linux-kernel&m=132007169420160&w=2
>
> Subject : hugetlb oops on 3.1.0-rc8-devel
> Submitter : Andy Lutomirski <luto@amacapital.net>
> Date : 2011-11-01 22:20
> Message-ID : CALCETrW1mpVCz2tO5roaz1r6vnno+srHR-dHA6_pkRi2qiCfdw@mail.gmail.com
> References : http://marc.info/?l=linux-kernel&m=132018604426692&w=2
>
> Subject : Simultaneous cat and external keyboard input causing kernel panic
> Submitter : Timo Jyrinki <timo.jyrinki@gmail.com>
> Date : 2011-11-03 12:14
> Message-ID : CAJtFfxmovJHspHHKbvBVc4pw+u5mjGmUejCXEzdV+GqE=jVSOQ@mail.gmail.com
> References : http://marc.info/?l=linux-kernel&m=132032253903074&w=2
>
> Subject : Linus GIT - INFO: possible circular locking dependency detected
> Submitter : Miles Lane <miles.lane@gmail.com>
> Date : 2011-11-03 15:57
> Message-ID : CAHFgRy8S0xLfhZxTUOEH5A0PL_Fb79-0-gmbQ=9h2D-xMqt1hA@mail.gmail.com
> References : http://marc.info/?l=linux-kernel&m=132033587908426&w=2
>
> Subject : lockdep warning after aa6afca5bcab: "proc: fix races against execve() of /proc/PID/fd**"
> Submitter : Ari Savolainen <ari.m.savolainen@gmail.com>
> Date : 2011-11-08 3:47
> Message-ID : CAEbykaXYZEFhTgWMm2AfaWQ2SaXYuO_ypTnw+6AVWScOYSCuuw@mail.gmail.com
> References : http://marc.info/?l=linux-kernel&m=132072413125099&w=2
>
> Subject : DMA-API check_sync errors with 3.2
> Submitter : Josh Boyer <jwboyer@redhat.com>
> Date : 2011-11-08 17:31
> Message-ID : 20111108173153.GE14216@zod.bos.redhat.com
> References : http://marc.info/?l=linux-kernel&m=132077357608797&w=2
>
> Subject : [3.1-rc8 REGRESSION] sky2 hangs machine on turning off or suspending
> Submitter : Rafał Miłecki <zajec5@gmail.com>
> Date : 2011-11-09 11:46
> Message-ID : CACna6ryTdLcWVYgHu=_mRFga1sFivpE_DyZOY-HMmKggkWAJAw@mail.gmail.com
> References : http://marc.info/?l=linux-netdev&m=132083922228088&w=4
>
> Subject : 3.2-rc1 doesn't boot on dual socket opteron without swap
> Submitter : Niklas Schnelle <niklas@komani.de>
> Date : 2011-11-10 20:09
> Message-ID : 1320955769.1718.8.camel@jupiter
> References : http://marc.info/?l=linux-kernel&m=132095583501767&w=2
>
> Subject : Sparc-32 doesn't work in 3.1.
> Submitter : Rob Landley <rob@landley.net>
> Date : 2011-11-12 11:22
> Message-ID : 4EBEAB5A.5020809@landley.net
> References : http://www.spinics.net/lists/kernel/msg1260383.html
>
> Subject : khugepaged blocks suspend2ram (3.2.0-rc1-00252-g8f042aa)
> Submitter : Sergei Trofimovich <slyich@gmail.com>
> Date : 2011-11-12 10:48
> Message-ID : 20111112104859.7744b282@sf.home
> References : https://lkml.org/lkml/2011/11/12/11
>
Andrea's patch already fixes this issue, which was reported first by
Jiri Slaby. https://lkml.org/lkml/2011/11/11/93
I remember Andrew Morton taking this patch in his -mm tree. But it is
not in mainline yet. So can we consider this closed or not?
Thanks,
Srivatsa S. Bhat
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: 3.2-rc2+: Reported regressions from 3.0 and 3.1
From: Ari Savolainen @ 2011-11-22 5:27 UTC (permalink / raw)
To: Linus Torvalds, Rafael J. Wysocki
Cc: Linux SCSI List, Linux ACPI, Network Development,
Linux Wireless List, Linux Kernel Mailing List, DRI,
Florian Mickler, Andrew Morton, Kernel Testers List,
Linux PM List, Maciej Rutecki
In-Reply-To: <CA+55aFyceMKgS-YRv=r=FrHQ1P9=z7=2PC5gvZcCHoYKnzn_Aw@mail.gmail.com>
2011/11/22 Linus Torvalds <torvalds@linux-foundation.org>:
> On Mon, Nov 21, 2011 at 1:49 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>
>> Subject : lockdep warning after aa6afca5bcab: "proc: fix races against execve() of /proc/PID/fd**"
>> Submitter : Ari Savolainen <ari.m.savolainen@gmail.com>
>> Date : 2011-11-08 3:47
>> Message-ID : CAEbykaXYZEFhTgWMm2AfaWQ2SaXYuO_ypTnw+6AVWScOYSCuuw@mail.gmail.com
>> References : http://marc.info/?l=linux-kernel&m=132072413125099&w=2
>
> Commit aa6afca5bcab was reverted by commit 5e442a493fc5, so this one
> is presumably stale.
>
> Linus
Yes, this went away after the reversion.
Ari
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox