[PATCH] tevent and threads - infrastructure improvements - version #2

[Ralph Böhme]

On Mon, Sep 14, 2015 at 12:09:30PM -0700, Jeremy Allison wrote:
> On Mon, Sep 14, 2015 at 07:26:56PM +0200, Ralph Böhme wrote:
> > On Mon, Sep 14, 2015 at 09:23:08AM -0700, Jeremy Allison wrote:
> > > On Sun, Sep 13, 2015 at 02:34:11PM +0200, Ralph Böhme wrote:
> > > > Hi Jeremy,
> > > > 
> > > > On Fri, Jul 24, 2015 at 10:16:15AM -0700, Jeremy Allison wrote:
> > > > > On Thu, Jul 23, 2015 at 04:50:37PM -0700, Jeremy Allison wrote:
> > > > > > 
> > > > > > FYI. I now have a working implementation of this
> > > > > > API - passes valgrind memcheck and drd !
> > > > > > 
> > > > > > Hurrah for me :-).
> > > > > > 
> > > > > > Will post an updated patch once I've finished
> > > > > > updating the tutorial.
> > > > > 
> > > > > Here it is. Passes valgrind --tool=drd and
> > > > > valgrind --tool=memcheck.
> > > > > 
> > > > > Metze please let me know if this is what
> > > > > you had in mind.
> > > > > 
> > > > > Everyone else just review :-).
> > > > 
> > > > a few minor issue:
> > > > 
> > > > * always use talloc_get_type_abort() where possible
> > > 
> > > Looks good to me. Do you want me to squash these
> > > into the main patch and add your signed-off-by ?
> > > 
> > > > * reverse the order of signalling and unlocking in
> > > >   tevent_thread_proxy_schedule()
> > > 
> > > LGTM. Again, squash and signed-off ?
> > > 
> > > Also, are you getting to a 'Reviewed-by' ? :-).
> > 
> > yes on both, feel free to squash and add my signed-off and
> > reviewed-by.
> 
> Unfortunately - remember:
> 
> http://bholley.net/blog/2015/must-be-this-tall-to-write-multi-threaded-code.html
> 
> :-).

:) Yeah, mixing threads and talloc destructors is where the fun
starts.

> The optimization change you added:

That just looked like the correct way to do it, I wasn't really in
optimisation mode. :)

> Reverse order of unlocking and notifying in
> tevent_thread_proxy_schedule(): unlocking before notifying avoids a
> potential wakeup of the signalled thread with the mutex still locked.
> 
> introduces valgrind --tool=drd and --tool =helgrind errors
> that are not there with the original code path.
> 
> It's OK to wake up the signalled thread with the mutex still locked,
> it'll just wait on the mutex until it gets unlocked. No big deal :-).
> 
> The reason the mutex must be held until after the signaling
> is done is that in the second test code there may be a destructor
> waiting to remove the proxy object once the mutex gets released,
> and if that happens you get conflicting write errors.

Hm. I don't see a pending desctructor in the second test that would
fire before all threads have finished. Which talloc_free is triggering
it? I'll give it a closer look tomorrow.

So while I think you're probably right that we may need to lock the
proxy object when we're still working on it, I'm wondering if there's
something else fishy.

-Ralph

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de,mailto:kontakt@sernet.de