Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Thu, Apr 19, 2007 at 01:58:45AM -0600, Eric W. Biederman wrote: > From: Eric W. Biederman <[EMAIL PROTECTED]> > > This patch modifies the startup of eehd to use kthread_run > not a combination of kernel_thread and daemonize. Making > the code slightly simpler and more maintainable. For the patch that touched arch/powerpc/platforms/pseries/eeh_event.c, I ran a variety of tests, and couldn't see/find/evoke any adverse effects, so .. Acked-by: Linas Vepstas <[EMAIL PROTECTED]> > The second question is whether this is the right implementation. > kthread_create already works by using a workqueue to create the thread > and then waits for it. If we really want to support creating threads > asynchronously on demand we should have a proper API in kthread.c for > this instead of spreading workqueues. Yes, exactly; all I really want is to start a thread from an interrupt context, and pass a structure to it. This is pretty much all that arch/powerpc/platforms/pseries/eeh_event.c is trying to do, and little else. --linas - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Tue, Apr 24, 2007 at 11:38:53AM +1000, Benjamin Herrenschmidt wrote: > > The only reason for using threads here is to get the error recovery > > out of an interrupt context (where errors may be detected), and then, > > an hour later, decrement a counter (which is how we limit these to > > 6 per hour). Thread reaping is "trivial", the thread just exits > > after an hour. > > In addition, it should be a thread and not done from within keventd > because : > > - It can take a long time (well, relatively but still too long for a > work queue) Uhh, 15 or 20 seconds even. That's a long time by any kernel standard. > - The driver callbacks might need to use keventd or do flush_workqueue > to synchronize with their own workqueues when doing an internal > recovery. > > > Since these are events rare, I've no particular concern about > > performance or resource consumption. The current code seems > > to work just fine. :-) > > I think moving to kthread's is cleaner (just a wrapper around kernel > threads that simplify dealing with reaping them out mostly) and I agree > with Christoph that it would be nice to be able to "fire off" kthreads > from interrupt context.. in many cases, we abuse work queues for things > that should really done from kthreads instead (basically anything that > takes more than a couple hundred microsecs or so). It would be nice to have threads that can be "fired off" from an interrupt context. That would simplify the EEH code slightly (removing a few dozen lines of code that do this bounce). I presume that various device drivers might find this useful as well. --linas - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Tue, Apr 24, 2007 at 03:55:06PM +1000, Paul Mackerras wrote: > Christoph Hellwig writes: > > > The first question is obviously, is this really something we want? > > spawning kernel thread on demand without reaping them properly seems > > quite dangerous. > > What specifically has to be done to reap a kernel thread? Are you > concerned about the number of threads, or about having zombies hanging > around? I'm mostly concerned about number of threads and possible leakage of threads. Linas already explained it's not a problem in this case, so it's covered. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Tue, 24 Apr 2007 15:00:42 +1000, Benjamin Herrenschmidt <[EMAIL PROTECTED]> wrote: > Like anything else, modules should have separated the entrypoints for > > - Initiating a removal request > - Releasing the module > > The former is use did "rmmod", can unregister things from subsystems, > etc... (and can file if the driver decides to refuse removal requests > when it's busy doing things or whatever policy that module wants to > implement). > > The later is called when all references to the modules have been > dropped, it's a bit like the kref "release" (and could be implemented as > one). That sounds quite similar to the problems we have with kobject refcounting vs. module unloading. The patchset I posted at http://marc.info/?l=linux-kernel=117679014404994=2 exposes the refcount of the kobject embedded in the module. Maybe the kthread code could use that reference as well? - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Christoph Hellwig writes: > The first question is obviously, is this really something we want? > spawning kernel thread on demand without reaping them properly seems > quite dangerous. What specifically has to be done to reap a kernel thread? Are you concerned about the number of threads, or about having zombies hanging around? Paul. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
> Since we need to have some way to track them having an explicit data > structure that the callers manage seems to make sense. Oh sure, I wasn't arguing against that at all... It might be handy to have a release() callback (optional) that gets called after the kthread stops/exits, once we know the data structure isn't going to be used anymore (if practical to implement, depends on your approach). Ben. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Christoph Hellwig writes: The first question is obviously, is this really something we want? spawning kernel thread on demand without reaping them properly seems quite dangerous. What specifically has to be done to reap a kernel thread? Are you concerned about the number of threads, or about having zombies hanging around? Paul. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Tue, 24 Apr 2007 15:00:42 +1000, Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: Like anything else, modules should have separated the entrypoints for - Initiating a removal request - Releasing the module The former is use did rmmod, can unregister things from subsystems, etc... (and can file if the driver decides to refuse removal requests when it's busy doing things or whatever policy that module wants to implement). The later is called when all references to the modules have been dropped, it's a bit like the kref release (and could be implemented as one). That sounds quite similar to the problems we have with kobject refcounting vs. module unloading. The patchset I posted at http://marc.info/?l=linux-kernelm=117679014404994w=2 exposes the refcount of the kobject embedded in the module. Maybe the kthread code could use that reference as well? - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Tue, Apr 24, 2007 at 03:55:06PM +1000, Paul Mackerras wrote: Christoph Hellwig writes: The first question is obviously, is this really something we want? spawning kernel thread on demand without reaping them properly seems quite dangerous. What specifically has to be done to reap a kernel thread? Are you concerned about the number of threads, or about having zombies hanging around? I'm mostly concerned about number of threads and possible leakage of threads. Linas already explained it's not a problem in this case, so it's covered. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Tue, Apr 24, 2007 at 11:38:53AM +1000, Benjamin Herrenschmidt wrote: The only reason for using threads here is to get the error recovery out of an interrupt context (where errors may be detected), and then, an hour later, decrement a counter (which is how we limit these to 6 per hour). Thread reaping is trivial, the thread just exits after an hour. In addition, it should be a thread and not done from within keventd because : - It can take a long time (well, relatively but still too long for a work queue) Uhh, 15 or 20 seconds even. That's a long time by any kernel standard. - The driver callbacks might need to use keventd or do flush_workqueue to synchronize with their own workqueues when doing an internal recovery. Since these are events rare, I've no particular concern about performance or resource consumption. The current code seems to work just fine. :-) I think moving to kthread's is cleaner (just a wrapper around kernel threads that simplify dealing with reaping them out mostly) and I agree with Christoph that it would be nice to be able to fire off kthreads from interrupt context.. in many cases, we abuse work queues for things that should really done from kthreads instead (basically anything that takes more than a couple hundred microsecs or so). It would be nice to have threads that can be fired off from an interrupt context. That would simplify the EEH code slightly (removing a few dozen lines of code that do this bounce). I presume that various device drivers might find this useful as well. --linas - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Thu, Apr 19, 2007 at 01:58:45AM -0600, Eric W. Biederman wrote: From: Eric W. Biederman [EMAIL PROTECTED] This patch modifies the startup of eehd to use kthread_run not a combination of kernel_thread and daemonize. Making the code slightly simpler and more maintainable. For the patch that touched arch/powerpc/platforms/pseries/eeh_event.c, I ran a variety of tests, and couldn't see/find/evoke any adverse effects, so .. Acked-by: Linas Vepstas [EMAIL PROTECTED] The second question is whether this is the right implementation. kthread_create already works by using a workqueue to create the thread and then waits for it. If we really want to support creating threads asynchronously on demand we should have a proper API in kthread.c for this instead of spreading workqueues. Yes, exactly; all I really want is to start a thread from an interrupt context, and pass a structure to it. This is pretty much all that arch/powerpc/platforms/pseries/eeh_event.c is trying to do, and little else. --linas - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Benjamin Herrenschmidt <[EMAIL PROTECTED]> writes: >> Further in general it doesn't make sense to grab a module reference >> and call that sufficient because we would like to request that the >> module exits. > > Which is, btw, I think a total misdesign of our module stuff, but heh, I > remember that lead to some flamewars back then... > > Like anything else, modules should have separated the entrypoints for > > - Initiating a removal request > - Releasing the module > > The former is use did "rmmod", can unregister things from subsystems, > etc... (and can file if the driver decides to refuse removal requests > when it's busy doing things or whatever policy that module wants to > implement). > > The later is called when all references to the modules have been > dropped, it's a bit like the kref "release" (and could be implemented as > one). > > If we had done that (simple) thing back then, module refcounting would > have been much less of a problem... I remember some reasons why that was > veto'ed but I didn't and still don't agree. The basic point is because a thread can terminate sooner if we have an explicit request to stop, we need that in the design. Because we need to find the threads to request that they stop we need to have some way to track them. Since we need to have some way to track them having an explicit data structure that the callers manage seems to make sense. Eric - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
> Further in general it doesn't make sense to grab a module reference > and call that sufficient because we would like to request that the > module exits. Which is, btw, I think a total misdesign of our module stuff, but heh, I remember that lead to some flamewars back then... Like anything else, modules should have separated the entrypoints for - Initiating a removal request - Releasing the module The former is use did "rmmod", can unregister things from subsystems, etc... (and can file if the driver decides to refuse removal requests when it's busy doing things or whatever policy that module wants to implement). The later is called when all references to the modules have been dropped, it's a bit like the kref "release" (and could be implemented as one). If we had done that (simple) thing back then, module refcounting would have been much less of a problem... I remember some reasons why that was veto'ed but I didn't and still don't agree. Ben. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Paul Mackerras <[EMAIL PROTECTED]> writes: > Eric W. Biederman writes: > >> Well the basic problem is that for any piece of code that can be modular >> we need a way to ensure all threads it has running are shutdown when we >> remove the module. > > The EEH code can't be modular, and wouldn't make any sense to be > modular, since it's part of the infrastructure for accessing PCI > devices. Agreed. However most kthread users are modular and make sense to be so we need to design to handle modular users. I don't think the idiom of go fire off a thread to handle something is specific to non-modular users. Eric - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Eric W. Biederman writes: > Well the basic problem is that for any piece of code that can be modular > we need a way to ensure all threads it has running are shutdown when we > remove the module. The EEH code can't be modular, and wouldn't make any sense to be modular, since it's part of the infrastructure for accessing PCI devices. Paul. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Benjamin Herrenschmidt <[EMAIL PROTECTED]> writes: > Not sure... I can see places where I might want to spawn an arbitrary > number of these without having to preallocate structures... and if I > allocate on the fly, then I need a way to free that structure when the > kthread is reaped which I don't think we have currently, do we ? (In > fact, I could use that for other things too now that I'm thinking of > it ... I might have a go at providing optional kthread destructors). Well the basic problem is that for any piece of code that can be modular we need a way to ensure all threads it has running are shutdown when we remove the module. Which means a fire and forget model however simple is unfortunately the wrong thing. Now we might be able to wrap this in some kind of manager construct, so you don't have to manage each thread individually, but we still have the problem of ensuring all of the threads exit when we terminate the module. Further in general it doesn't make sense to grab a module reference and call that sufficient because we would like to request that the module exits. Eric - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Mon, 2007-04-23 at 20:08 -0600, Eric W. Biederman wrote: > Benjamin Herrenschmidt <[EMAIL PROTECTED]> writes: > > >> The only reason for using threads here is to get the error recovery > >> out of an interrupt context (where errors may be detected), and then, > >> an hour later, decrement a counter (which is how we limit these to > >> 6 per hour). Thread reaping is "trivial", the thread just exits > >> after an hour. > > > > In addition, it should be a thread and not done from within keventd > > because : > > > > - It can take a long time (well, relatively but still too long for a > > work queue) > > > > - The driver callbacks might need to use keventd or do flush_workqueue > > to synchronize with their own workqueues when doing an internal > > recovery. > > > >> Since these are events rare, I've no particular concern about > >> performance or resource consumption. The current code seems > >> to work just fine. :-) > > > > I think moving to kthread's is cleaner (just a wrapper around kernel > > threads that simplify dealing with reaping them out mostly) and I agree > > with Christoph that it would be nice to be able to "fire off" kthreads > > from interrupt context.. in many cases, we abuse work queues for things > > that should really done from kthreads instead (basically anything that > > takes more than a couple hundred microsecs or so). > > On that note does anyone have a problem is we manage the irq spawning > safe kthreads the same way that we manage the work queue entries. > > i.e. by a structure allocated by the caller? Not sure... I can see places where I might want to spawn an arbitrary number of these without having to preallocate structures... and if I allocate on the fly, then I need a way to free that structure when the kthread is reaped which I don't think we have currently, do we ? (In fact, I could use that for other things too now that I'm thinking of it ... I might have a go at providing optional kthread destructors). Ben. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Benjamin Herrenschmidt <[EMAIL PROTECTED]> writes: >> The only reason for using threads here is to get the error recovery >> out of an interrupt context (where errors may be detected), and then, >> an hour later, decrement a counter (which is how we limit these to >> 6 per hour). Thread reaping is "trivial", the thread just exits >> after an hour. > > In addition, it should be a thread and not done from within keventd > because : > > - It can take a long time (well, relatively but still too long for a > work queue) > > - The driver callbacks might need to use keventd or do flush_workqueue > to synchronize with their own workqueues when doing an internal > recovery. > >> Since these are events rare, I've no particular concern about >> performance or resource consumption. The current code seems >> to work just fine. :-) > > I think moving to kthread's is cleaner (just a wrapper around kernel > threads that simplify dealing with reaping them out mostly) and I agree > with Christoph that it would be nice to be able to "fire off" kthreads > from interrupt context.. in many cases, we abuse work queues for things > that should really done from kthreads instead (basically anything that > takes more than a couple hundred microsecs or so). On that note does anyone have a problem is we manage the irq spawning safe kthreads the same way that we manage the work queue entries. i.e. by a structure allocated by the caller? Eric - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
> The only reason for using threads here is to get the error recovery > out of an interrupt context (where errors may be detected), and then, > an hour later, decrement a counter (which is how we limit these to > 6 per hour). Thread reaping is "trivial", the thread just exits > after an hour. In addition, it should be a thread and not done from within keventd because : - It can take a long time (well, relatively but still too long for a work queue) - The driver callbacks might need to use keventd or do flush_workqueue to synchronize with their own workqueues when doing an internal recovery. > Since these are events rare, I've no particular concern about > performance or resource consumption. The current code seems > to work just fine. :-) I think moving to kthread's is cleaner (just a wrapper around kernel threads that simplify dealing with reaping them out mostly) and I agree with Christoph that it would be nice to be able to "fire off" kthreads from interrupt context.. in many cases, we abuse work queues for things that should really done from kthreads instead (basically anything that takes more than a couple hundred microsecs or so). Ben. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Sun, Apr 22, 2007 at 01:31:55PM +0100, Christoph Hellwig wrote: > On Thu, Apr 19, 2007 at 01:58:45AM -0600, Eric W. Biederman wrote: > > From: Eric W. Biederman <[EMAIL PROTECTED]> > > > > This patch modifies the startup of eehd to use kthread_run > > not a combination of kernel_thread and daemonize. Making > > the code slightly simpler and more maintainable. > > This one has the same scheme as the various s390 drivers where a thread > is spawned using a workqueue on demand. I think we should not blindly > convert it but think a litte more about it. > > The first question is obviously, is this really something we want? > spawning kernel thread on demand without reaping them properly seems > quite dangerous. I'm not quite sure what the intent of this patch really is, being at most a somewhat passing and casual user of kernel threads. Some background may be useful: (this in reply to some comments from Andrew Morton) EEH events are supposed to be very rare, as they correspond to hardware failures, typically PCI bus parity errors, but also things like wild DMA's. The code that generates these will limit them to no more than 6 per hour per pci device. Any more than that, and the PCI device is permanently disabled (the sysadmin would need to do something to recover). The only reason for using threads here is to get the error recovery out of an interrupt context (where errors may be detected), and then, an hour later, decrement a counter (which is how we limit these to 6 per hour). Thread reaping is "trivial", the thread just exits after an hour. Since these are events rare, I've no particular concern about performance or resource consumption. The current code seems to work just fine. :-) --linas - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Sun, Apr 22, 2007 at 01:31:55PM +0100, Christoph Hellwig wrote: On Thu, Apr 19, 2007 at 01:58:45AM -0600, Eric W. Biederman wrote: From: Eric W. Biederman [EMAIL PROTECTED] This patch modifies the startup of eehd to use kthread_run not a combination of kernel_thread and daemonize. Making the code slightly simpler and more maintainable. This one has the same scheme as the various s390 drivers where a thread is spawned using a workqueue on demand. I think we should not blindly convert it but think a litte more about it. The first question is obviously, is this really something we want? spawning kernel thread on demand without reaping them properly seems quite dangerous. I'm not quite sure what the intent of this patch really is, being at most a somewhat passing and casual user of kernel threads. Some background may be useful: (this in reply to some comments from Andrew Morton) EEH events are supposed to be very rare, as they correspond to hardware failures, typically PCI bus parity errors, but also things like wild DMA's. The code that generates these will limit them to no more than 6 per hour per pci device. Any more than that, and the PCI device is permanently disabled (the sysadmin would need to do something to recover). The only reason for using threads here is to get the error recovery out of an interrupt context (where errors may be detected), and then, an hour later, decrement a counter (which is how we limit these to 6 per hour). Thread reaping is trivial, the thread just exits after an hour. Since these are events rare, I've no particular concern about performance or resource consumption. The current code seems to work just fine. :-) --linas - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
The only reason for using threads here is to get the error recovery out of an interrupt context (where errors may be detected), and then, an hour later, decrement a counter (which is how we limit these to 6 per hour). Thread reaping is trivial, the thread just exits after an hour. In addition, it should be a thread and not done from within keventd because : - It can take a long time (well, relatively but still too long for a work queue) - The driver callbacks might need to use keventd or do flush_workqueue to synchronize with their own workqueues when doing an internal recovery. Since these are events rare, I've no particular concern about performance or resource consumption. The current code seems to work just fine. :-) I think moving to kthread's is cleaner (just a wrapper around kernel threads that simplify dealing with reaping them out mostly) and I agree with Christoph that it would be nice to be able to fire off kthreads from interrupt context.. in many cases, we abuse work queues for things that should really done from kthreads instead (basically anything that takes more than a couple hundred microsecs or so). Ben. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Benjamin Herrenschmidt [EMAIL PROTECTED] writes: The only reason for using threads here is to get the error recovery out of an interrupt context (where errors may be detected), and then, an hour later, decrement a counter (which is how we limit these to 6 per hour). Thread reaping is trivial, the thread just exits after an hour. In addition, it should be a thread and not done from within keventd because : - It can take a long time (well, relatively but still too long for a work queue) - The driver callbacks might need to use keventd or do flush_workqueue to synchronize with their own workqueues when doing an internal recovery. Since these are events rare, I've no particular concern about performance or resource consumption. The current code seems to work just fine. :-) I think moving to kthread's is cleaner (just a wrapper around kernel threads that simplify dealing with reaping them out mostly) and I agree with Christoph that it would be nice to be able to fire off kthreads from interrupt context.. in many cases, we abuse work queues for things that should really done from kthreads instead (basically anything that takes more than a couple hundred microsecs or so). On that note does anyone have a problem is we manage the irq spawning safe kthreads the same way that we manage the work queue entries. i.e. by a structure allocated by the caller? Eric - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Mon, 2007-04-23 at 20:08 -0600, Eric W. Biederman wrote: Benjamin Herrenschmidt [EMAIL PROTECTED] writes: The only reason for using threads here is to get the error recovery out of an interrupt context (where errors may be detected), and then, an hour later, decrement a counter (which is how we limit these to 6 per hour). Thread reaping is trivial, the thread just exits after an hour. In addition, it should be a thread and not done from within keventd because : - It can take a long time (well, relatively but still too long for a work queue) - The driver callbacks might need to use keventd or do flush_workqueue to synchronize with their own workqueues when doing an internal recovery. Since these are events rare, I've no particular concern about performance or resource consumption. The current code seems to work just fine. :-) I think moving to kthread's is cleaner (just a wrapper around kernel threads that simplify dealing with reaping them out mostly) and I agree with Christoph that it would be nice to be able to fire off kthreads from interrupt context.. in many cases, we abuse work queues for things that should really done from kthreads instead (basically anything that takes more than a couple hundred microsecs or so). On that note does anyone have a problem is we manage the irq spawning safe kthreads the same way that we manage the work queue entries. i.e. by a structure allocated by the caller? Not sure... I can see places where I might want to spawn an arbitrary number of these without having to preallocate structures... and if I allocate on the fly, then I need a way to free that structure when the kthread is reaped which I don't think we have currently, do we ? (In fact, I could use that for other things too now that I'm thinking of it ... I might have a go at providing optional kthread destructors). Ben. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Benjamin Herrenschmidt [EMAIL PROTECTED] writes: Not sure... I can see places where I might want to spawn an arbitrary number of these without having to preallocate structures... and if I allocate on the fly, then I need a way to free that structure when the kthread is reaped which I don't think we have currently, do we ? (In fact, I could use that for other things too now that I'm thinking of it ... I might have a go at providing optional kthread destructors). Well the basic problem is that for any piece of code that can be modular we need a way to ensure all threads it has running are shutdown when we remove the module. Which means a fire and forget model however simple is unfortunately the wrong thing. Now we might be able to wrap this in some kind of manager construct, so you don't have to manage each thread individually, but we still have the problem of ensuring all of the threads exit when we terminate the module. Further in general it doesn't make sense to grab a module reference and call that sufficient because we would like to request that the module exits. Eric - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Eric W. Biederman writes: Well the basic problem is that for any piece of code that can be modular we need a way to ensure all threads it has running are shutdown when we remove the module. The EEH code can't be modular, and wouldn't make any sense to be modular, since it's part of the infrastructure for accessing PCI devices. Paul. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Paul Mackerras [EMAIL PROTECTED] writes: Eric W. Biederman writes: Well the basic problem is that for any piece of code that can be modular we need a way to ensure all threads it has running are shutdown when we remove the module. The EEH code can't be modular, and wouldn't make any sense to be modular, since it's part of the infrastructure for accessing PCI devices. Agreed. However most kthread users are modular and make sense to be so we need to design to handle modular users. I don't think the idiom of go fire off a thread to handle something is specific to non-modular users. Eric - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Further in general it doesn't make sense to grab a module reference and call that sufficient because we would like to request that the module exits. Which is, btw, I think a total misdesign of our module stuff, but heh, I remember that lead to some flamewars back then... Like anything else, modules should have separated the entrypoints for - Initiating a removal request - Releasing the module The former is use did rmmod, can unregister things from subsystems, etc... (and can file if the driver decides to refuse removal requests when it's busy doing things or whatever policy that module wants to implement). The later is called when all references to the modules have been dropped, it's a bit like the kref release (and could be implemented as one). If we had done that (simple) thing back then, module refcounting would have been much less of a problem... I remember some reasons why that was veto'ed but I didn't and still don't agree. Ben. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
Benjamin Herrenschmidt [EMAIL PROTECTED] writes: Further in general it doesn't make sense to grab a module reference and call that sufficient because we would like to request that the module exits. Which is, btw, I think a total misdesign of our module stuff, but heh, I remember that lead to some flamewars back then... Like anything else, modules should have separated the entrypoints for - Initiating a removal request - Releasing the module The former is use did rmmod, can unregister things from subsystems, etc... (and can file if the driver decides to refuse removal requests when it's busy doing things or whatever policy that module wants to implement). The later is called when all references to the modules have been dropped, it's a bit like the kref release (and could be implemented as one). If we had done that (simple) thing back then, module refcounting would have been much less of a problem... I remember some reasons why that was veto'ed but I didn't and still don't agree. The basic point is because a thread can terminate sooner if we have an explicit request to stop, we need that in the design. Because we need to find the threads to request that they stop we need to have some way to track them. Since we need to have some way to track them having an explicit data structure that the callers manage seems to make sense. Eric - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Thu, Apr 19, 2007 at 01:58:45AM -0600, Eric W. Biederman wrote: > From: Eric W. Biederman <[EMAIL PROTECTED]> > > This patch modifies the startup of eehd to use kthread_run > not a combination of kernel_thread and daemonize. Making > the code slightly simpler and more maintainable. This one has the same scheme as the various s390 drivers where a thread is spawned using a workqueue on demand. I think we should not blindly convert it but think a litte more about it. The first question is obviously, is this really something we want? spawning kernel thread on demand without reaping them properly seems quite dangerous. The second question is whether this is the right implementation. kthread_create already works by using a workqueue to create the thread and then waits for it. If we really want to support creating threads asynchronously on demand we should have a proper API in kthread.c for this instead of spreading workqueues. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Thu, Apr 19, 2007 at 01:58:45AM -0600, Eric W. Biederman wrote: From: Eric W. Biederman [EMAIL PROTECTED] This patch modifies the startup of eehd to use kthread_run not a combination of kernel_thread and daemonize. Making the code slightly simpler and more maintainable. This one has the same scheme as the various s390 drivers where a thread is spawned using a workqueue on demand. I think we should not blindly convert it but think a litte more about it. The first question is obviously, is this really something we want? spawning kernel thread on demand without reaping them properly seems quite dangerous. The second question is whether this is the right implementation. kthread_create already works by using a workqueue to create the thread and then waits for it. If we really want to support creating threads asynchronously on demand we should have a proper API in kthread.c for this instead of spreading workqueues. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Thu, 19 Apr 2007 01:58:45 -0600 "Eric W. Biederman" <[EMAIL PROTECTED]> wrote: > This patch modifies the startup of eehd to use kthread_run > not a combination of kernel_thread and daemonize. Making > the code slightly simpler and more maintainable. > You're making me look at a lot of things which I'd prefer not to have looked at. > arch/powerpc/platforms/pseries/eeh_event.c |4 ++-- This one kicks off a kernel thread in response to each "PCI error event", and that kernel thread hangs about for one hour then exits. One wonders what happens if we get 1,000,000 of these events per second. - 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/
Re: [PATCH] powerpc pseries eeh: Convert to kthread API
On Thu, 19 Apr 2007 01:58:45 -0600 Eric W. Biederman [EMAIL PROTECTED] wrote: This patch modifies the startup of eehd to use kthread_run not a combination of kernel_thread and daemonize. Making the code slightly simpler and more maintainable. You're making me look at a lot of things which I'd prefer not to have looked at. arch/powerpc/platforms/pseries/eeh_event.c |4 ++-- This one kicks off a kernel thread in response to each PCI error event, and that kernel thread hangs about for one hour then exits. One wonders what happens if we get 1,000,000 of these events per second. - 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/
