Re: Wireless regression in workqueue: use mod_delayed_work() instead of __cancel + queue

[Anders Kaseorg]

On Wed, 28 Nov 2012, Anders Kaseorg wrote:
> My Intel 6250 wireless card (iwldvm) can no longer associate with a 
> WPA-Enterprise network (PEAP-MSCHAPv2).  To my surprise, I bisected this 
> regression to commit e7c2f967445dd2041f0f8e3179cca22bb8bb7f79, 
> workqueue: use mod_delayed_work() instead of __cancel + queue.
> 
> A bunch of logs collected by Ubuntu apport are in this bug report: 
>   https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1083980
> 
> How can I help to debug this?
> 
> I see that someone else reported another regression with the same commit 
> last week, although this looks unrelated at first glance: 
>   http://thread.gmane.org/gmane.linux.kernel/1395938
> 
> Anders

(Oops, re-sending to fix a typo in the LKML address.)

Anders
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Wireless regression in workqueue: use mod_delayed_work() instead of __cancel + queue

[Anders Kaseorg]

On Fri, 30 Nov 2012, Tejun Heo wrote:
> Hey, again.
> 
> Can you please test whether the following patch makes any difference?
> 
> Thanks!
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 042d221..26368ef 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1477,7 +1477,10 @@ bool mod_delayed_work_on(int cpu, struct 
> workqueue_struct *wq,
>   } while (unlikely(ret == -EAGAIN));
>  
>   if (likely(ret >= 0)) {
> - __queue_delayed_work(cpu, wq, dwork, delay);
> + if (!delay)
> + __queue_work(cpu, wq, &dwork->work);
> + else
> + __queue_delayed_work(cpu, wq, dwork, delay);
>   local_irq_restore(flags);
>   }
>  

Yes.  I tested that both directly on top of the bad commit, and on 
v3.7-rc7, and it fixes the bug in both cases.

Thanks,
Anders
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Wireless regression in workqueue: use mod_delayed_work() instead of __cancel + queue

[Anders Kaseorg]

On Sat, 1 Dec 2012, Tejun Heo wrote:
> Can you please test this one too?  Thanks!
> 
> […]
> + if (!delay) {
> + __queue_work(cpu, wq, &dwork->work);
> + return;
> + }
> +
> […]
> - if (!delay)
> - return queue_work_on(cpu, wq, &dwork->work);
> -

Yes, this one fixes the bug too (on v3.7.0-rc7).

Anders
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] fifo: Do not restart open() if it already found a partner

[Anders Kaseorg]

If a parent and child process open the two ends of a fifo, and the
child immediately exits, the parent may receive a SIGCHLD before its
open() returns.  In that case, we need to make sure that open() will
return successfully after the SIGCHLD handler returns, instead of
throwing EINTR or being restarted.  Otherwise, the restarted open()
would incorrectly wait for a second partner on the other end.

The following test demonstrates the EINTR that was wrongly thrown from
the parent’s open().  Change .sa_flags = 0 to .sa_flags = SA_RESTART
to see a deadlock instead, in which the restarted open() waits for a
second reader that will never come.  (On my systems, this happens
pretty reliably within about 5 to 500 iterations.  Others report that
it manages to loop ~forever sometimes; YMMV.)

  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 

  #define CHECK(x) do if ((x) == -1) {perror(#x); abort();} while(0)

  void handler(int signum) {}

  int main()
  {
  struct sigaction act = {.sa_handler = handler, .sa_flags = 0};
  CHECK(sigaction(SIGCHLD, &act, NULL));
  CHECK(mknod("fifo", S_IFIFO | S_IRWXU, 0));
  for (;;) {
  int fd;
  pid_t pid;
  putc('.', stderr);
  CHECK(pid = fork());
  if (pid == 0) {
  CHECK(fd = open("fifo", O_RDONLY));
  _exit(0);
  }
  CHECK(fd = open("fifo", O_WRONLY));
  CHECK(close(fd));
  CHECK(waitpid(pid, NULL, 0));
  }
  }

This is what I suspect was causing the Git test suite to fail in
t9010-svn-fe.sh:
http://bugs.debian.org/678852

Signed-off-by: Anders Kaseorg 
Reviewed-by: Jonathan Nieder 
---
(Re-sending v2 to Linus with no changes.)

 fs/fifo.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/fifo.c b/fs/fifo.c
index b1a524d..cf6f434 100644
--- a/fs/fifo.c
+++ b/fs/fifo.c
@@ -14,7 +14,7 @@
 #include 
 #include 
 
-static void wait_for_partner(struct inode* inode, unsigned int *cnt)
+static int wait_for_partner(struct inode* inode, unsigned int *cnt)
 {
int cur = *cnt; 
 
@@ -23,6 +23,7 @@ static void wait_for_partner(struct inode* inode, unsigned 
int *cnt)
if (signal_pending(current))
break;
}
+   return cur == *cnt ? -ERESTARTSYS : 0;
 }
 
 static void wake_up_partner(struct inode* inode)
@@ -67,8 +68,7 @@ static int fifo_open(struct inode *inode, struct file *filp)
 * seen a writer */
filp->f_version = pipe->w_counter;
} else {
-   wait_for_partner(inode, &pipe->w_counter);
-   if(signal_pending(current))
+   if (wait_for_partner(inode, &pipe->w_counter))
goto err_rd;
}
}
@@ -90,8 +90,7 @@ static int fifo_open(struct inode *inode, struct file *filp)
wake_up_partner(inode);
 
if (!pipe->readers) {
-   wait_for_partner(inode, &pipe->r_counter);
-   if (signal_pending(current))
+   if (wait_for_partner(inode, &pipe->r_counter))
goto err_wr;
}
break;
-- 
1.7.11.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/