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

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

[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 <oleg@redhat.com>
---

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

--- 3.1/arch/powerpc/platforms/pseries/eeh_event.c~3_daemonize_eeh	2011-04-06 21:33:42.000000000 +0200
+++ 3.1/arch/powerpc/platforms/pseries/eeh_event.c	2011-08-16 21:03:56.000000000 +0200
@@ -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);

Re: [PATCH] powerpc/eeh: remove eeh_event_handler()->daemonize()

[Benjamin Herrenschmidt]

On Tue, 2011-08-16 at 21:08 +0200, Oleg Nesterov wrote:
> 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 <oleg@redhat.com>
> ---

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

Cheers,
Ben.

>  arch/powerpc/platforms/pseries/eeh_event.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- 3.1/arch/powerpc/platforms/pseries/eeh_event.c~3_daemonize_eeh	2011-04-06 21:33:42.000000000 +0200
> +++ 3.1/arch/powerpc/platforms/pseries/eeh_event.c	2011-08-16 21:03:56.000000000 +0200
> @@ -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 majordomo@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: [PATCH] powerpc/eeh: remove eeh_event_handler()->daemonize()

[Linas Vepstas]

On 16 August 2011 14:08, Oleg Nesterov <oleg@redhat.com> wrote:
> 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 <oleg@redhat.com>

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 <linasvepstas@gmail.com>

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


> ---
>
>  arch/powerpc/platforms/pseries/eeh_event.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- 3.1/arch/powerpc/platforms/pseries/eeh_event.c~3_daemonize_eeh      2011-04-06 21:33:42.000000000 +0200
> +++ 3.1/arch/powerpc/platforms/pseries/eeh_event.c      2011-08-16 21:03:56.000000000 +0200
> @@ -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);
>
>

Re: [PATCH] powerpc/eeh: remove eeh_event_handler()->daemonize()

[Tejun Heo]

Hello,

On Tue, Aug 16, 2011 at 02:24:12PM -0500, Linas Vepstas wrote:
> 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.

-- 
tejun

Re: [PATCH] powerpc/eeh: remove eeh_event_handler()->daemonize()

[Linas Vepstas]

On 16 August 2011 14:29, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Tue, Aug 16, 2011 at 02:24:12PM -0500, Linas Vepstas wrote:
>> 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?

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

--linas

Re: [PATCH] powerpc/eeh: remove eeh_event_handler()->daemonize()

[Tejun Heo]

On Tue, Aug 16, 2011 at 09:08:58PM +0200, Oleg Nesterov wrote:
> 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 <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

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

Thanks.

-- 
tejun

Re: [PATCH] powerpc/eeh: remove eeh_event_handler()->daemonize()

[Oleg Nesterov]

On 08/16, Tejun Heo wrote:
>
> On Tue, Aug 16, 2011 at 09:08:58PM +0200, Oleg Nesterov wrote:
> > 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 <oleg@redhat.com>
>
> Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

> 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.

Re: [PATCH] powerpc/eeh: remove eeh_event_handler()->daemonize()

[Tejun Heo]

Hello,

On Wed, Aug 17, 2011 at 07:27:13PM +0200, Oleg Nesterov wrote:
> 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.

-- 
tejun

Re: [PATCH] powerpc/eeh: remove eeh_event_handler()->daemonize()

[Matt Fleming]

On Tue, 2011-08-16 at 21:08 +0200, Oleg Nesterov wrote:
> 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 <oleg@redhat.com>

Acked-by: Matt Fleming <matt.fleming@intel.com>