Message ID | 1479420564-15799-1-git-send-email-xiyou.wangcong@gmail.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, Nov 17, 2016 at 2:09 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > Commit 2b15af6f95 ("af_unix: use freezable blocking calls in read") > converts schedule_timeout() to its freezable version, it was probably > correct at that time, but later, commit 2b514574f7e8 > ("net: af_unix: implement splice for stream af_unix sockets") breaks > the strong requirement for a freezable sleep, according to > commit 0f9548ca1091: > > We shouldn't try_to_freeze if locks are held. Holding a lock can cause a > deadlock if the lock is later acquired in the suspend or hibernate path > (e.g. by dpm). Holding a lock can also cause a deadlock in the case of > cgroup_freezer if a lock is held inside a frozen cgroup that is later > acquired by a process outside that group. > > The pipe_lock is still held at that point. So just revert commit 2b15af6f95. On my phone 77 threads are blocked in unix_stream_recvmsg. A simple revert of this patch will cause every one of those threads to wake up twice per suspend cycle, which can be multiple times a second. How about adding a freezable flag to unix_stream_read_state so unix_stream_recvmsg can stay freezable, and unix_stream_splice_read can be unfreezable?
On Thu, Nov 17, 2016 at 2:30 PM, Colin Cross <ccross@android.com> wrote: > On Thu, Nov 17, 2016 at 2:09 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> Commit 2b15af6f95 ("af_unix: use freezable blocking calls in read") >> converts schedule_timeout() to its freezable version, it was probably >> correct at that time, but later, commit 2b514574f7e8 >> ("net: af_unix: implement splice for stream af_unix sockets") breaks >> the strong requirement for a freezable sleep, according to >> commit 0f9548ca1091: >> >> We shouldn't try_to_freeze if locks are held. Holding a lock can cause a >> deadlock if the lock is later acquired in the suspend or hibernate path >> (e.g. by dpm). Holding a lock can also cause a deadlock in the case of >> cgroup_freezer if a lock is held inside a frozen cgroup that is later >> acquired by a process outside that group. >> >> The pipe_lock is still held at that point. So just revert commit 2b15af6f95. > > On my phone 77 threads are blocked in unix_stream_recvmsg. A simple > revert of this patch will cause every one of those threads to wake up > twice per suspend cycle, which can be multiple times a second. How > about adding a freezable flag to unix_stream_read_state so > unix_stream_recvmsg can stay freezable, and unix_stream_splice_read > can be unfreezable? Fair enough, I didn't know it could have such an impact. I will send v2. Thanks.
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 5d1c14a..6f37f9e 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -116,7 +116,6 @@ #include <linux/mount.h> #include <net/checksum.h> #include <linux/security.h> -#include <linux/freezer.h> struct hlist_head unix_socket_table[2 * UNIX_HASH_SIZE]; EXPORT_SYMBOL_GPL(unix_socket_table); @@ -2220,7 +2219,7 @@ static long unix_stream_data_wait(struct sock *sk, long timeo, sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk); unix_state_unlock(sk); - timeo = freezable_schedule_timeout(timeo); + timeo = schedule_timeout(timeo); unix_state_lock(sk); if (sock_flag(sk, SOCK_DEAD))
Commit 2b15af6f95 ("af_unix: use freezable blocking calls in read") converts schedule_timeout() to its freezable version, it was probably correct at that time, but later, commit 2b514574f7e8 ("net: af_unix: implement splice for stream af_unix sockets") breaks the strong requirement for a freezable sleep, according to commit 0f9548ca1091: We shouldn't try_to_freeze if locks are held. Holding a lock can cause a deadlock if the lock is later acquired in the suspend or hibernate path (e.g. by dpm). Holding a lock can also cause a deadlock in the case of cgroup_freezer if a lock is held inside a frozen cgroup that is later acquired by a process outside that group. The pipe_lock is still held at that point. So just revert commit 2b15af6f95. Reported-by: Dmitry Vyukov <dvyukov@google.com> Cc: Tejun Heo <tj@kernel.org> Cc: Colin Cross <ccross@android.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/unix/af_unix.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)