iv_work_submit_work() might spawn a new child thread from incorrect parent

[bazsi]

In syslog-ng, our main thread polls various sources and whenever there's a new I/O event, it submits this as work to a set of worker threads, an iv_work_pool instance.

This works great. I was about to reuse the same worker pool to submit job from another thread, i.e. not from the main thread.

In case there's no worker running at the time this 2nd thread submits a job to be executed, it will be spawned, as expected.

The problem happens when this worker thread wants to exit: since iv_thread_create() is called on-demand, from the 2nd thread, it is registered as a child of the 2nd thread, which means that iv_thread->dead is registered in the 2nd thread.

When the 2nd thread exits (earlier than the work_pool is destroyed), its state is freed (iv_deinit()), thus when the work-pool's thread exits, it's going to report back to a thread that does not exist at that point, causing a fatal error like this:

iv_fd_epoll_event_send: epoll_ctl returned error 9[Bad file descriptor]

Since the iv_work_pool_submit() seems to handle calls from multiple threads (as there's proper locking), I was under the assumption that it would work if I submit work from multiple threads.

I had multiple ideas how this could be handled, but I thought it's best to ask first:

• start iv_work_pool workers in a "detached" state, so we don't report completion status
• start iv_work_pool workers so that they get associated with the same thread that created iv_work_pool
• de-associate iv_work_pool workers when the parent exits (e.g in. iv_thread_tls_deinit_thread() where pthr_detach is also called)

This behaviour also surfaces in our main thread, where we don't call iv_deinit(), so that the ivykis state remains for as long as threads might be exiting, but I didn't want ad-hoc threads to not call iv_deinit()

what do you think?

[buytenh]

Hello! The iv_work(3) man page mentions:

       iv_work_pool_submit_work can  only  be  called  from  the  thread  that
       iv_work_pool_create for this pool object was called in.

This restriction exists because of some of the reasons you mentioned. (This restriction wasn't enforced originally, and enforcing it now would involve breaking the ABI.)

The locking in iv_work exists mainly to synchronize the worker threads with the thread that owns the work pool (and which submits items to the work pool). It wasn't meant to allow submitting work items from multiple different threads.

Allowing submission of work from arbitrary threads raises the question, as you alluded to, of where to run the completion handlers.

Note that most of the complexity in iv_thread (which carries over to iv_work) is to make sure that the parent thread doesn't quit before the child thread does, both to prevent the child thread from accessing already freed memory when it exists, and to make sure that the application doesn't terminate until all work we wanted to perform has completed.

Your first suggestion seems like it would prevent all current iv_work users from having their specified completion handlers invoked? Breaking existing users doesn't seem like a viable way to enable a new feature.

As to the second suggestion, where would you run the completion handler in this case? In the thread that created the work pool? How would you handle the case where thread A creates the work pool, and then thread B tries to enqueue work to the work pool while thread A tries to destroy the work pool?

As to the third suggestion, where would you run the completion handler for a work item if this happens? (Would you not run it at all?)

The removal of completion notification would obviate most of the current complexity in iv_work (and in iv_thread). I'm thinking that if you don't have a need for completion handlers to be invoked, you might be better off hacking up your own version of iv_work, where all work items are just queued up somewhere and then handled in a fire-and-forget kind of mode.

[bazsi]

Hi Lennert and thank you for the detailed reply. very valid questions. I am giving this some more thought and come back to you.

[bazsi]

Hello! The iv_work(3) man page mentions:

       iv_work_pool_submit_work can  only  be  called  from  the  thread  that
       iv_work_pool_create for this pool object was called in.

This restriction exists because of some of the reasons you mentioned. (This restriction wasn't enforced originally, and enforcing it now would involve breaking the ABI.)

I usually start with reading the code instead of looking for documentation, but I may have to reconsider this about ivykis. Great library with great documentation. Thanks for that.

The locking in iv_work exists mainly to synchronize the worker threads with the thread that owns the work pool (and which submits items to the work pool). It wasn't meant to allow submitting work items from multiple different threads.

Got it.

Allowing submission of work from arbitrary threads raises the question, as you alluded to, of where to run the completion handlers.

Just a minor point, in my case it wasn't the completion handler that causes an issue, rather the "worker-thread-exit" machinery. In this specific case I wasn't using completion callbacks. Still in other cases syslog-ng uses them and they are very valid and useful part of the interface.

Note that most of the complexity in iv_thread (which carries over to iv_work) is to make sure that the parent thread doesn't quit before the child thread does, both to prevent the child thread from accessing already freed memory when it exists, and to make sure that the application doesn't terminate until all work we wanted to perform has completed.

BTW: iv_work_pool_put() does not seem to wait for threads to exit. it signals that they should exit, but it does not seem to wait for them. I think this is the reason that we don't have an iv_deinit() at the end of our main(). But this is a side discussion and I might even miss something.

Your first suggestion seems like it would prevent all current iv_work users from having their specified completion handlers invoked? Breaking existing users doesn't seem like a viable way to enable a new feature.

As to the second suggestion, where would you run the completion handler in this case? In the thread that created the work pool? How would you handle the case where thread A creates the work pool, and then thread B tries to enqueue work to the work pool while thread A tries to destroy the work pool?

As to the third suggestion, where would you run the completion handler for a work item if this happens? (Would you not run it at all?)

The removal of completion notification would obviate most of the current complexity in iv_work (and in iv_thread). I'm thinking that if you don't have a need for completion handlers to be invoked, you might be better off hacking up your own version of iv_work, where all work items are just queued up somewhere and then handled in a fire-and-forget kind of mode.

Let me explain the two separate use-cases of iv_work_pool in my context:

1. Async workers use-case:

1. the main thread runs an ivykis main loop to poll all of syslog-ng's inputs
2. whenever there's an I/O event, we push a task to our iv_work_pool to handle it
3. the task submitted performs all read or write calls as necessary to process input
4. the completion callback is used to re-register the input for polling

Also, syslog-ng tracks the number of outstanding I/O tasks so we know when it is safe to exit, so we currently don't rely on iv_work_pool to do this for us. (probably because we have other threads as well, but I honestly don't remember).

2. Re-schedule work from a task use-case:

1. in this use-case we are in an iv_work_pool worker thread, busy processing an I/O task that was originally submitted by the main thread (e.g. previous use-case)
2. sometimes the configuration of syslog-ng contains highly CPU intensive operations to be performed on an incoming log message (e.g. a long list of regular expressions), and in these cases, handing over this heavyweight stuff to a separate thread improves performance a lot (the overhead of the handover between threads is cheaper than the work yet to be done, and by splitting the work we can do stuff in parallel, on multiple cores).
3. in this use-case whenever a threshold is crossed (I didn't yet decide what would trigger this behaviour), the worker thread would submit a new task to the same iv_work_pool to handle the remainder of the processing, this way the original task can go back handling I/O and thus fetch new messages.

What I have found via experiments was that this handover between threads only improves performance as long as we don't overcommit the number of cores with threads. This means that I would like to limit parallel processing of messages to the number of cores in the system. And this is how we need to use the same iv_work_pool instance, its max_threads is set to the number of CPU cores (by default).

Using a separate iv_work_pool-like object would not give me an easy way to limit the parallelism between I/O and CPU intensive processing.

With that in mind, let me summarize what I would need, hopefully clarifying things a little:

• to be able to submit work from a "worker" thread. On the syslog-ng side, I called these work items to be "slaves" of a work item originally submitted by the thread owning iv_work_pool. The primary reason for this submission is to split up long running work so they can execute in parallel.
• there's no need to submit tasks from a random, non-worker thread.
• the fact that the work is submitted by a worker ensures that the iv_work_pool still exists (since one of its workers is still running tasks)
• if a new thread is spawned in this case, that should not be associated with the current thread, rather to the main thread. It needs to die the same way the rest of the worker threads are. Essentially all threads should behave as if all workers were started right at the beginning of the work pool.
• the responsibility of the work at hand is transferred to the slave completely, so the original worker shall not rely on completions to continue work that was transferred.
• I think the completion callback of the slave work items should go to the main thread, just as all completions do. Right now I don't have a need for completions in this case, so it might be ok to say that slave work items don't have completions at all but I guess, doing completions in the main thread should be easy.

With all that, I think the only needed change in iv_work_pool is being able to associate newly created threads with the main thread (e.g. the one owning iv_work_pool). The rest would just work.

What do you think?

[bazsi]

Is this above something that would be worth supporting in your opinion? How else would you implement this?
Thanks

[bazsi]

I have created a proof-of-concept branch here: https://github.com/bazsi/ivykis/tree/iv-work-pool-support-for-slave-work-items

It currently uses an iv_event to wake up the main thread to launch a new worker thread, which should resolve the issue at the cost of some latency in starting up the worker (e.g. when the main thread is busy). What do you think? Should I clean that up or search for a completely different solution?

thanks

[bazsi]

@buytenh any chance you could look into this in the near future? Thanks

[buytenh]

@bazsi Given that your change has merged, I think we can close this one now, right?

[bazsi]

Yup, absolutely. Closing.