| Message ID | 20200318204408.102694393@linutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Lock ordering documentation and annotation for lockdep | expand |
On 2020-03-18 21:43:09 [+0100], Thomas Gleixner wrote: > --- a/arch/powerpc/platforms/ps3/device-init.c > +++ b/arch/powerpc/platforms/ps3/device-init.c > @@ -725,12 +728,12 @@ static int ps3_notification_read_write(s > unsigned long flags; > int res; > > - init_completion(&dev->done); > spin_lock_irqsave(&dev->lock, flags); > res = write ? lv1_storage_write(dev->sbd.dev_id, 0, 0, 1, 0, lpar, > &dev->tag) > : lv1_storage_read(dev->sbd.dev_id, 0, 0, 1, 0, lpar, > &dev->tag); > + dev->done = false; > spin_unlock_irqrestore(&dev->lock, flags); > if (res) { > pr_err("%s:%u: %s failed %d\n", __func__, __LINE__, op, res); > @@ -738,14 +741,10 @@ static int ps3_notification_read_write(s > } > pr_debug("%s:%u: notification %s issued\n", __func__, __LINE__, op); > > - res = wait_event_interruptible(dev->done.wait, > - dev->done.done || kthread_should_stop()); > + rcuwait_wait_event(&dev->wait, dev->done || kthread_should_stop(), TASK_IDLE); > + … Not sure it matters but this struct `dev' is allocated on stack. Should the interrupt fire *before* rcuwait_wait_event() set wait.task to NULL then it is of random value on the first invocation of rcuwait_wake_up(). -> diff --git a/arch/powerpc/platforms/ps3/device-init.c b/arch/powerpc/platforms/ps3/device-init.c index 197347c3c0b24..e87360a0fb40d 100644 --- a/arch/powerpc/platforms/ps3/device-init.c +++ b/arch/powerpc/platforms/ps3/device-init.c @@ -809,6 +809,7 @@ static int ps3_probe_thread(void *data) } spin_lock_init(&dev.lock); + rcuwait_init(&dev.wait); res = request_irq(irq, ps3_notification_interrupt, 0, "ps3_notification", &dev); Sebastian
On Thu, Mar 19, 2020 at 10:00:24AM +0100, Sebastian Andrzej Siewior wrote: > On 2020-03-18 21:43:09 [+0100], Thomas Gleixner wrote: > > --- a/arch/powerpc/platforms/ps3/device-init.c > > +++ b/arch/powerpc/platforms/ps3/device-init.c > > @@ -725,12 +728,12 @@ static int ps3_notification_read_write(s > > unsigned long flags; > > int res; > > > > - init_completion(&dev->done); > > spin_lock_irqsave(&dev->lock, flags); > > res = write ? lv1_storage_write(dev->sbd.dev_id, 0, 0, 1, 0, lpar, > > &dev->tag) > > : lv1_storage_read(dev->sbd.dev_id, 0, 0, 1, 0, lpar, > > &dev->tag); > > + dev->done = false; > > spin_unlock_irqrestore(&dev->lock, flags); > > if (res) { > > pr_err("%s:%u: %s failed %d\n", __func__, __LINE__, op, res); > > @@ -738,14 +741,10 @@ static int ps3_notification_read_write(s > > } > > pr_debug("%s:%u: notification %s issued\n", __func__, __LINE__, op); > > > > - res = wait_event_interruptible(dev->done.wait, > > - dev->done.done || kthread_should_stop()); > > + rcuwait_wait_event(&dev->wait, dev->done || kthread_should_stop(), TASK_IDLE); > > + > … > > Not sure it matters but this struct `dev' is allocated on stack. Should > the interrupt fire *before* rcuwait_wait_event() set wait.task to NULL > then it is of random value on the first invocation of rcuwait_wake_up(). > -> > > diff --git a/arch/powerpc/platforms/ps3/device-init.c b/arch/powerpc/platforms/ps3/device-init.c > index 197347c3c0b24..e87360a0fb40d 100644 > --- a/arch/powerpc/platforms/ps3/device-init.c > +++ b/arch/powerpc/platforms/ps3/device-init.c > @@ -809,6 +809,7 @@ static int ps3_probe_thread(void *data) > } > > spin_lock_init(&dev.lock); > + rcuwait_init(&dev.wait); > > res = request_irq(irq, ps3_notification_interrupt, 0, > "ps3_notification", &dev); > Very good, sorry for that.
On Wed, 18 Mar 2020, Thomas Gleixner wrote: >AFAICT the kthread uses TASK_INTERRUPTIBLE to not increase loadavg, kthreads >cannot receive signals by default and this one doesn't look different. Use >TASK_IDLE instead. Hmm it seems in general this needs to be done kernel-wide. This kthread abuse of TASK_INTERRUPTIBLE seems to be a common thing. There's also the users doing schedule_timeout_interruptible()... Thanks, Davidlohr
On Wed, Mar 18, 2020 at 09:43:09PM +0100, Thomas Gleixner wrote: > The PS3 notification interrupt and kthread use a hacked up completion to > communicate. Since we're wanting to change the completion implementation and > this is abuse anyway, replace it with a simple rcuwait since there is only ever > the one waiter. > > AFAICT the kthread uses TASK_INTERRUPTIBLE to not increase loadavg, kthreads > cannot receive signals by default and this one doesn't look different. Use > TASK_IDLE instead. I think the right fix here is to jut convert the thing to a threaded interrupt handler and kill off the stupid kthread. But I wonder how alive the whole PS3 support is to start with..
On 2020-03-19 03:04:59 [-0700], Christoph Hellwig wrote:
> But I wonder how alive the whole PS3 support is to start with..
OtherOS can only be used on "old" PS3 which do not have have their
firmware upgraded past version 3.21, released April 1, 2010 [0].
It was not possible to install OtherOS on PS3-slim and I don't remember
if it was a successor or a budget version (but it had lower power
consumption as per my memory).
*I* remember from back then that a few universities bought quite a few
of them and used them as a computation cluster. However, whatever broke
over the last 10 years is broken.
[0] https://en.wikipedia.org/wiki/OtherOS
Sebastian
Hi, On 3/19/20 3:26 AM, Sebastian Andrzej Siewior wrote: > On 2020-03-19 03:04:59 [-0700], Christoph Hellwig wrote: >> But I wonder how alive the whole PS3 support is to start with.. > > OtherOS can only be used on "old" PS3 which do not have have their > firmware upgraded past version 3.21, released April 1, 2010 [0]. > It was not possible to install OtherOS on PS3-slim and I don't remember > if it was a successor or a budget version (but it had lower power > consumption as per my memory). > *I* remember from back then that a few universities bought quite a few > of them and used them as a computation cluster. However, whatever broke > over the last 10 years is broken. > > [0] https://en.wikipedia.org/wiki/OtherOS There are still PS3-Linux users out there. They generally use firmware and other tools available through the 'hacker' communities that allow Linux to be run on more than just the 'officially supported' platforms. Anyway, the change to use rcuwait seems fine if that's needed for the completion re-work. I'll try to do some testing with the patch set next week. -Geoff
Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > On 2020-03-19 03:04:59 [-0700], Christoph Hellwig wrote: >> But I wonder how alive the whole PS3 support is to start with.. > > OtherOS can only be used on "old" PS3 which do not have have their > firmware upgraded past version 3.21, released April 1, 2010 [0]. > It was not possible to install OtherOS on PS3-slim and I don't remember > if it was a successor or a budget version (but it had lower power > consumption as per my memory). > *I* remember from back then that a few universities bought quite a few > of them and used them as a computation cluster. However, whatever broke > over the last 10 years is broken. > > [0] https://en.wikipedia.org/wiki/OtherOS Last time I asked on the list there were still a handful of users. And I had a patch submitted from a user as recently as last October, so it still has some life. cheers
Christoph Hellwig <hch@infradead.org> writes: > On Wed, Mar 18, 2020 at 09:43:09PM +0100, Thomas Gleixner wrote: >> The PS3 notification interrupt and kthread use a hacked up completion to >> communicate. Since we're wanting to change the completion implementation and >> this is abuse anyway, replace it with a simple rcuwait since there is only ever >> the one waiter. >> >> AFAICT the kthread uses TASK_INTERRUPTIBLE to not increase loadavg, kthreads >> cannot receive signals by default and this one doesn't look different. Use >> TASK_IDLE instead. > > I think the right fix here is to jut convert the thing to a threaded > interrupt handler and kill off the stupid kthread. That'd be a major surgery. > But I wonder how alive the whole PS3 support is to start with.. There seem to be a few enthusiast left which have a Other-OS capable PS3 Thanks, tglx
--- a/arch/powerpc/platforms/ps3/device-init.c +++ b/arch/powerpc/platforms/ps3/device-init.c @@ -13,6 +13,7 @@ #include <linux/init.h> #include <linux/slab.h> #include <linux/reboot.h> +#include <linux/rcuwait.h> #include <asm/firmware.h> #include <asm/lv1call.h> @@ -670,7 +671,8 @@ struct ps3_notification_device { spinlock_t lock; u64 tag; u64 lv1_status; - struct completion done; + struct rcuwait wait; + bool done; }; enum ps3_notify_type { @@ -712,7 +714,8 @@ static irqreturn_t ps3_notification_inte pr_debug("%s:%u: completed, status 0x%llx\n", __func__, __LINE__, status); dev->lv1_status = status; - complete(&dev->done); + dev->done = true; + rcuwait_wake_up(&dev->wait); } spin_unlock(&dev->lock); return IRQ_HANDLED; @@ -725,12 +728,12 @@ static int ps3_notification_read_write(s unsigned long flags; int res; - init_completion(&dev->done); spin_lock_irqsave(&dev->lock, flags); res = write ? lv1_storage_write(dev->sbd.dev_id, 0, 0, 1, 0, lpar, &dev->tag) : lv1_storage_read(dev->sbd.dev_id, 0, 0, 1, 0, lpar, &dev->tag); + dev->done = false; spin_unlock_irqrestore(&dev->lock, flags); if (res) { pr_err("%s:%u: %s failed %d\n", __func__, __LINE__, op, res); @@ -738,14 +741,10 @@ static int ps3_notification_read_write(s } pr_debug("%s:%u: notification %s issued\n", __func__, __LINE__, op); - res = wait_event_interruptible(dev->done.wait, - dev->done.done || kthread_should_stop()); + rcuwait_wait_event(&dev->wait, dev->done || kthread_should_stop(), TASK_IDLE); + if (kthread_should_stop()) res = -EINTR; - if (res) { - pr_debug("%s:%u: interrupted %s\n", __func__, __LINE__, op); - return res; - } if (dev->lv1_status) { pr_err("%s:%u: %s not completed, status 0x%llx\n", __func__,
The PS3 notification interrupt and kthread use a hacked up completion to communicate. Since we're wanting to change the completion implementation and this is abuse anyway, replace it with a simple rcuwait since there is only ever the one waiter. AFAICT the kthread uses TASK_INTERRUPTIBLE to not increase loadavg, kthreads cannot receive signals by default and this one doesn't look different. Use TASK_IDLE instead. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Arnd Bergmann <arnd@arndb.de> Cc: linuxppc-dev@lists.ozlabs.org --- V2: New patch to avoid the magic completion wait variant --- arch/powerpc/platforms/ps3/device-init.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)