powerpc/eeh: remove eeh_event_handler()->daemonize()

PATCH Superseded Oleg Nesterov 5 participants 9 messages Aug 16, 2011 – Aug 18, 2011

Subsystems (2)

PowerPC PCI enhanced error handling (eeh) for PowerPC

lore

Share & tools

lore

Oleg Nesterov

author

2011-08-16 19:12 UTC #0 lore raw

PATCH 1/1 powerpc/eeh: remove eeh_event_handler()->daemonize()

daemonize() is only needed when a user-space task does kernel_thread().

eeh_event_handler() thread is created by the worker kthread, and thus it doesn't need the soon-to-be-deprecated daemonize().

Note: looks like eeh_event_wq can be static and it can do all work itself without kernel_thread().

Signed-off-by Oleg Nesterov <[email protected]>

---

 arch/powerpc/platforms/pseries/eeh_event.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/powerpc/platforms/pseries/eeh_event.c renamed from arch/powerpc/platforms/pseries/eeh_event.c~3_daemonize_eeh +1 -1

@@ -60,7 +60,7 @@ static int eeh_event_handler(void * dumm
 	struct eeh_event	*event;
 	struct pci_dn *pdn;
 
-	daemonize ("eehd");
+	set_task_comm(current, "eehd");
 	set_current_state(TASK_INTERRUPTIBLE);
 
 	spin_lock_irqsave(&eeh_eventlist_lock, flags);

Linas Vepstas

Acked-by on v1 1/1

2011-08-16 19:24 UTC #1 lore raw

Quoted 9 lines · Oleg Nesterov

daemonize() is only needed when a user-space task does kernel_thread().
eeh_event_handler() thread is created by the worker kthread, and thus it doesn't need the soon-to-be-deprecated daemonize().
Note: looks like eeh_event_wq can be static and it can do all work itself without kernel_thread().
Signed-off-by Oleg Nesterov <[email protected]>

I've not been following the deprecation of daemonize(), so I presume that the suggested change is indeed appropriate.

As to the kernel thread, realize that the eeh recovery proceedure can take many seconds, or in certain pathological situations, it can hang (e.g. if a sata controller dies and fails to recover when there's a mounted file system on it ..)

so, given that, I'll just say:

Acked-by Linas Vepstas <[email protected]>

BTW, if someone wants to tackle a big, difficult project, one would be to untangle the mess that happens when the hardware underneath an active block device dies. Currently, the kernel will deadlock in umpteen zillion places waiting for blocks and files that will never arrive.

--linas

Quoted 18 lines

---
arch/powerpc/platforms/pseries/eeh_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/powerpc/platforms/pseries/eeh_event.c 2011-08-16 21:03:56.000000000 +0200 renamed from arch/powerpc/platforms/pseries/eeh_event.c~3_daemonize_eeh 2011-04-06 21:33:42.000000000 +0200 +1 -1
@@ -60,7 +60,7 @@ static int eeh_event_handler(void * dumm
        struct eeh_event        *event;
        struct pci_dn *pdn;
 
-       daemonize ("eehd");
+       set_task_comm(current, "eehd");
        set_current_state(TASK_INTERRUPTIBLE);
 
        spin_lock_irqsave(&eeh_eventlist_lock, flags);

Tejun Heo

2011-08-16 19:29 UTC #2 lore raw

Hello,

Quoted 5 lines · Linas Vepstas

BTW, if someone wants to tackle a big, difficult project, one would be to untangle the mess that happens when the hardware underneath an active block device dies. Currently, the kernel will deadlock in umpteen zillion places waiting for blocks and files that will never arrive.

Umm... that has been working fine for SCSI and ATA for years now. Just aborting pending commands and freeing the device should do it. Drivers need to take care of driver specific resources (per-req and per-dev) but that's not different from aborting timed out commands. What's missing for your use case?

Thanks.

Signature · 1 line

tejun

Linas Vepstas

2011-08-16 21:39 UTC #3 lore raw

Quoted 9 lines · Tejun Heo

Hello,
Quoted 5 lines · Linas Vepstas
BTW, if someone wants to tackle a big, difficult project, one would be to untangle the mess that happens when the hardware underneath an active block device dies. Currently, the kernel will deadlock in umpteen zillion places waiting for blocks and files that will never arrive.

Quoted 5 lines

Umm... that has been working fine for SCSI and ATA for years now. Just aborting pending commands and freeing the device should do it. Drivers need to take care of driver specific resources (per-req and per-dev) but that's not different from aborting timed out commands. What's missing for your use case?

Sorry, I guess I'm out-of-date; its been far more than a few years since I last tried this :-) The use case was eeh recovery, where the hardware was so hosed that it wouldn't recover. The test cases were whatever it was that the test team could throw at the system.

--linas

Tejun Heo

Acked-by on v1 1/1

2011-08-16 19:26 UTC #4 lore raw

In reply to Oleg Nesterov

Quoted 9 lines · Oleg Nesterov

daemonize() is only needed when a user-space task does kernel_thread().
eeh_event_handler() thread is created by the worker kthread, and thus it doesn't need the soon-to-be-deprecated daemonize().
Note: looks like eeh_event_wq can be static and it can do all work itself without kernel_thread().
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Tejun Heo <[email protected]>

Yeay! Nice series. How do you wanna route these? Through -mm?

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

author

2011-08-17 17:30 UTC #5 lore raw

Quoted 7 lines · Tejun Heo

Quoted 5 lines · Oleg Nesterov
daemonize() is only needed when a user-space task does kernel_thread().
eeh_event_handler() thread is created by the worker kthread, and thus it doesn't need the soon-to-be-deprecated daemonize().

Quoted 6 lines

Quoted 4 lines
Note: looks like eeh_event_wq can be static and it can do all work itself without kernel_thread().
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Tejun Heo <[email protected]>

Thanks.

Quoted 1 line

How do you wanna route these? Through -mm?

Oh, I don't know. -mm, or probably maintainers can pick them. They don't depend on each other.

Btw. Perhaps we should kill CLONE_KERNEL? And note that the only user is eeh_thread_launcher().

The problem is, CLONE_KERNEL looks as "use me if you want to create a kernel thread". But CLONE_SIGHAND means the kernel thread should not play with signals, and this is not obvious.

Oleg.

Tejun Heo

2011-08-18 07:35 UTC #6 lore raw

Hello,

Quoted 6 lines · Oleg Nesterov

Btw. Perhaps we should kill CLONE_KERNEL? And note that the only user is eeh_thread_launcher().
The problem is, CLONE_KERNEL looks as "use me if you want to create a kernel thread". But CLONE_SIGHAND means the kernel thread should not play with signals, and this is not obvious.

I guess it predates kthread(). There's no reason to use kernel_thread() directly outside of a few core places, so removing it sounds like a pretty good idea.

Thanks.

Signature · 1 line

tejun

Benjamin Herrenschmidt

2011-08-16 19:35 UTC #7 lore raw

In reply to Oleg Nesterov

Quoted 10 lines · Oleg Nesterov

daemonize() is only needed when a user-space task does kernel_thread().
eeh_event_handler() thread is created by the worker kthread, and thus it doesn't need the soon-to-be-deprecated daemonize().
Note: looks like eeh_event_wq can be static and it can do all work itself without kernel_thread().
Signed-off-by Oleg Nesterov <[email protected]>
---

Right, but don't bother cleaning that code too much tho :-) I'm quite close to reworking/refactoring most of it anyways.

Cheers,
Ben.

Quoted 20 lines

 arch/powerpc/platforms/pseries/eeh_event.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/powerpc/platforms/pseries/eeh_event.c renamed from arch/powerpc/platforms/pseries/eeh_event.c~3_daemonize_eeh +1 -2
@@ -60,7 +60,7 @@ static int eeh_event_handler(void * dumm
 	struct eeh_event	*event;
 	struct pci_dn *pdn;
 
-	daemonize ("eehd");
+	set_task_comm(current, "eehd");
 	set_current_state(TASK_INTERRUPTIBLE);
 
 	spin_lock_irqsave(&eeh_eventlist_lock, flags);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Matt Fleming

Acked-by on v1 1/1

2011-08-16 21:27 UTC #8 lore raw

In reply to Oleg Nesterov

Quoted 9 lines · Oleg Nesterov

daemonize() is only needed when a user-space task does kernel_thread().
eeh_event_handler() thread is created by the worker kthread, and thus it doesn't need the soon-to-be-deprecated daemonize().
Note: looks like eeh_event_wq can be static and it can do all work itself without kernel_thread().
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Matt Fleming <[email protected]>