Is there any recommended procedure to be followed before calling thread_exit so that the thread consumes all outstanding messages and is safe to destroy? I would assume something like while (spdk_thread_poll(thread) > 0); spdk_thread_exit(thread); spdk_thread_destroy(thread);

Is there any recommended procedure to be followed before calling thread_exit so that the thread consumes all outstanding messages and is safe to destroy? I would assume something like while (spdk_thread_poll(thread) > 0); spdk_thread_exit(thread); spdk_thread_destroy(thread);

On Wed, 2019-12-04 at 22:35 +0300, Andrey Kuzmin wrote: > Hi guys, > > I'm using spdk_thread_create to provide single-threaded context for the > operations that otherwise would require locking. Everything runs nice up to > exiting/destroying such a thread. In particular, after calling > spdk_thread_exit I'm witnessing thread being unable to process pending > messages (such as _spdk_put_io_channel which are generated by > spdk_put_io_channel issued on the exiting thread before calling > spdk_thread_exit), and destroying thread at this doesn't seem safe. > Thanks for starting this thread. As the creation/deletion of lightweight threads becomes more dynamic in SPDK, I think we need to come back and revisit some of these interfaces. In my opinion, the main ambiguity right now lies in two places. The first is in the proper use of the spdk_set_thread() function and the second is in use of the spdk_thread_destroy() function. If we look at the current public interface for the threading abstraction (include/spdk/thread.h), I think it makes sense to begin to view it as two interfaces instead of one. One interface is used by libraries that depend on the threading library. The other is the interface used by whatever code is implementing the underlying framework. Some of the functions do double-duty here because they're useful in both contexts, but some are very much intended to only be used in one of the scenarios. I think both spdk_set_thread() and spdk_thread_destroy() are two of the functions that are intended to be called only by the underlying framework (in addition to other functions like spdk_thread_poll()). Calling them from a bdev module, for example, isn't going to work.

To get everything sorted out, I propose a few changes: 1) spdk_thread_poll() should return an error (negative return code) if thread- >exit is marked true. In reactor.c, that will trigger a removal from the list and a call to spdk_thread_destroy. 2) We need to decide on the correct behavior when a user calls spdk_thread_exit() and there are still messages pending or pollers registered to the thread. I propose we make it so that spdk_thread_exit() can fail, as Shuhei suggested in the other thread.

within spdk_thread_poll (by checking that the thread-local variable is null). 4) spdk_set_thread() should also enforce that it isn't being called within spdk_thread_poll() in the same manner as #3.

What do you think? Are you interested in taking a shot at these changes?

Thanks, Ben > Is there any recommended procedure to be followed before calling > thread_exit so that the thread consumes all outstanding messages and is safe > to destroy? I would assume something like > > while (spdk_thread_poll(thread) > 0); > spdk_thread_exit(thread); > spdk_thread_destroy(thread); > > should do the trick, but I'd prefer to rely on a recommended procedure. > > Thanks, > Andrey > _______________________________________________ > SPDK mailing list -- spdk(a)lists.01.org > To unsubscribe send an email to spdk-leave(a)lists.01.org _______________________________________________ SPDK mailing list -- spdk(a)lists.01.org To unsubscribe send an email to spdk-leave(a)lists.01.org

Hi Andrey, Ben, I'd be glad to contribute at the discussion/review level, but don't see myself either fit or available for the actual framework-level coding (my schedule for the upcoming months is pretty tight). I may be able to take them with your help but it will be after iSCSI refactoring, maybe a week or more later. So that's great if you start. 🙂 -Shuehi ________________________________ 差出人: Andrey Kuzmin <andrey.v.kuzmin(a)gmail.com&gt; 送信日時: 2019年12月10日 5:54 宛先: Storage Performance Development Kit <spdk(a)lists.01.org&gt; 件名: [SPDK] Re: spdk_thread_{exit,destroy} safety Hi Ben, glad to hear from you on this. Few comments inline below. On Mon, Dec 9, 2019, 23:07 Walker, Benjamin <benjamin.walker(a)intel.com&gt; wrote: > On Wed, 2019-12-04 at 22:35 +0300, Andrey Kuzmin wrote: > > Hi guys, > > > > I'm using spdk_thread_create to provide single-threaded context for the > > operations that otherwise would require locking. Everything runs nice up > to > > exiting/destroying such a thread. In particular, after calling > > spdk_thread_exit I'm witnessing thread being unable to process pending > > messages (such as _spdk_put_io_channel which are generated by > > spdk_put_io_channel issued on the exiting thread before calling > > spdk_thread_exit), and destroying thread at this doesn't seem safe. > > > > Thanks for starting this thread. As the creation/deletion of lightweight > threads > becomes more dynamic in SPDK, I think we need to come back and revisit > some of > these interfaces. > > In my opinion, the main ambiguity right now lies in two places. The first > is in > the proper use of the spdk_set_thread() function and the second is in use > of the > spdk_thread_destroy() function. > > If we look at the current public interface for the threading abstraction > (include/spdk/thread.h), I think it makes sense to begin to view it as two > interfaces instead of one. One interface is used by libraries that depend > on the > threading library. The other is the interface used by whatever code is > implementing the underlying framework. Some of the functions do > double-duty here > because they're useful in both contexts, but some are very much intended > to only > be used in one of the scenarios. I think both spdk_set_thread() and > spdk_thread_destroy() are two of the functions that are intended to be > called > only by the underlying framework (in addition to other functions like > spdk_thread_poll()). Calling them from a bdev module, for example, isn't > going > to work. > Then it likely makes sense to move everything intended for framework only under include/spdk_internal, and to limit public interface to the part intended for spdk_thread* user. > To get everything sorted out, I propose a few changes: > > 1) spdk_thread_poll() should return an error (negative return code) if > thread- > >exit is marked true. In reactor.c, that will trigger a removal from the > list > and a call to spdk_thread_destroy. > 2) We need to decide on the correct behavior when a user calls > spdk_thread_exit() and there are still messages pending or pollers > registered to > the thread. I propose we make it so that spdk_thread_exit() can fail, as > Shuhei > suggested in the other thread. > The problem with (1-2) is how to distinguish between application-level messages addressed to the exiting thread and those issued by the framework in response to that thread's actions. The former must be dry by the time thread_exit is called, and ensuring this must be the responsibility of the spdk_thread_create caller (and also is something he/she is in control of). The latter - and a ready-made example off the top of my head is spdk_put_io_channel - is neither something the user can control nor be responsible for ensuring. 3) Calling spdk_thread_destroy() should enforce that it isn't being called > within spdk_thread_poll (by checking that the thread-local variable is > null). > 4) spdk_set_thread() should also enforce that it isn't being called within > spdk_thread_poll() in the same manner as #3. > An intriguing idea - or at least that's how it seemed to me when I started thinking on lw_thread issue - is to turn spdk_thread_exit into spdk_thread_destroy message to the exiting thread (and to mark thread as exited only when respective reactor receives/handles the message). This approach seems to provide nice control points re resources spdk_thread user is responsible for (pollers/timers/channels, as one can ensure those have been properly disabled or put inside thread_exit) and lwp thread handling, while still leaving the window for framework-initiated messages to get properly polled before the reactor executes thread_destroy/free. > > What do you think? Are you interested in taking a shot at these changes? > I'd be glad to contribute at the discussion/review level, but don't see myself either fit or available for the actual framework-level coding (my schedule for the upcoming months is pretty tight). Thanks, Andrey > > Thanks, > Ben > > > Is there any recommended procedure to be followed before calling > > thread_exit so that the thread consumes all outstanding messages and is > safe > > to destroy? I would assume something like > > > > while (spdk_thread_poll(thread) > 0); > > spdk_thread_exit(thread); > > spdk_thread_destroy(thread); > > > > should do the trick, but I'd prefer to rely on a recommended procedure. > > > > Thanks, > > Andrey > > _______________________________________________ > > SPDK mailing list -- spdk(a)lists.01.org > > To unsubscribe send an email to spdk-leave(a)lists.01.org > > _______________________________________________ > SPDK mailing list -- spdk(a)lists.01.org > To unsubscribe send an email to spdk-leave(a)lists.01.org > _______________________________________________ SPDK mailing list -- spdk(a)lists.01.org To unsubscribe send an email to spdk-leave(a)lists.01.org _______________________________________________ SPDK mailing list -- spdk(a)lists.01.org To unsubscribe send an email to spdk-leave(a)lists.01.org