[PATCH] pthreadpool: create a tevent_threaded_context per registered event context

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] pthreadpool: create a tevent_threaded_context per registered event context

Samba - samba-technical mailing list
Hi!

Attached is a patch that enhances pthreadpool_tevent to create one
tevent_threaded_context per registered event context instead of per
pthreadpool_tevent_job_send request.

Please review carefully -- the rundown is a bit tricky -- and push if
happy. Thanks!

Fwiw: it passed two private autobuilds and it survives smbtorture
local.messaging tests with a modified messaging subsystem that sends out all
messages via helper threads.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

pthreadpool_tevent.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pthreadpool: create a tevent_threaded_context per registered event context

Samba - samba-technical mailing list
On Tue, Nov 14, 2017 at 04:35:40PM +0100, Ralph Böhme wrote:

> Hi!
>
> Attached is a patch that enhances pthreadpool_tevent to create one
> tevent_threaded_context per registered event context instead of per
> pthreadpool_tevent_job_send request.
>
> Please review carefully -- the rundown is a bit tricky -- and push if
> happy. Thanks!
>
> Fwiw: it passed two private autobuilds and it survives smbtorture
> local.messaging tests with a modified messaging subsystem that sends out all
> messages via helper threads.

FYI, I'm reviewing this now, although it will take me a while
to make sure I understand it before giving a +1 :-).

Cheers,

        Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pthreadpool: create a tevent_threaded_context per registered event context

Samba - samba-technical mailing list
On Wed, Nov 15, 2017 at 04:30:28PM -0800, Jeremy Allison via samba-technical wrote:

> On Tue, Nov 14, 2017 at 04:35:40PM +0100, Ralph Böhme wrote:
> > Hi!
> >
> > Attached is a patch that enhances pthreadpool_tevent to create one
> > tevent_threaded_context per registered event context instead of per
> > pthreadpool_tevent_job_send request.
> >
> > Please review carefully -- the rundown is a bit tricky -- and push if
> > happy. Thanks!
> >
> > Fwiw: it passed two private autobuilds and it survives smbtorture
> > local.messaging tests with a modified messaging subsystem that sends out all
> > messages via helper threads.
>
> FYI, I'm reviewing this now, although it will take me a while
> to make sure I understand it before giving a +1 :-).

OK, I understand what this is doing. But I would really like
a version that has *copious* comments to aid anyone looking
at this code rather than having to figure it out from the
talloc heirarchies like I just did :-) :-).

Can you give me a day to add those comments and re-submit
it back to you for checking that they make sense and I really
did understand it ? :-).

Cheers,

        Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pthreadpool: create a tevent_threaded_context per registered event context

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Nov 14, 2017 at 04:35:40PM +0100, Ralph Böhme wrote:

> Hi!
>
> Attached is a patch that enhances pthreadpool_tevent to create one
> tevent_threaded_context per registered event context instead of per
> pthreadpool_tevent_job_send request.
>
> Please review carefully -- the rundown is a bit tricky -- and push if
> happy. Thanks!
>
> Fwiw: it passed two private autobuilds and it survives smbtorture
> local.messaging tests with a modified messaging subsystem that sends out all
> messages via helper threads.
OK, how about this version. That actual code is exactly as you
posted, all I've done is thoroughly commented it such that if
I had to maintain this I'd know immediately what it was doing :-).

Let me know if this is OK !

Jeremy.

0001-pthreadpool-create-a-tevent_threaded_context-per-reg.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pthreadpool: create a tevent_threaded_context per registered event context

Samba - samba-technical mailing list
On Thu, Nov 16, 2017 at 10:14:27AM -0800, Jeremy Allison wrote:

> On Tue, Nov 14, 2017 at 04:35:40PM +0100, Ralph Böhme wrote:
> > Hi!
> >
> > Attached is a patch that enhances pthreadpool_tevent to create one
> > tevent_threaded_context per registered event context instead of per
> > pthreadpool_tevent_job_send request.
> >
> > Please review carefully -- the rundown is a bit tricky -- and push if
> > happy. Thanks!
> >
> > Fwiw: it passed two private autobuilds and it survives smbtorture
> > local.messaging tests with a modified messaging subsystem that sends out all
> > messages via helper threads.
>
> OK, how about this version. That actual code is exactly as you
> posted, all I've done is thoroughly commented it such that if
> I had to maintain this I'd know immediately what it was doing :-).
>
> Let me know if this is OK !

awesome! Thanks a lot!

I actually think this deserves a pair-programmed-with: you and a signed-off-by:
you. Let me know you're ok with this change.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pthreadpool: create a tevent_threaded_context per registered event context

Samba - samba-technical mailing list
On Thu, Nov 16, 2017 at 07:31:31PM +0100, Ralph Böhme wrote:

> On Thu, Nov 16, 2017 at 10:14:27AM -0800, Jeremy Allison wrote:
> > On Tue, Nov 14, 2017 at 04:35:40PM +0100, Ralph Böhme wrote:
> > > Hi!
> > >
> > > Attached is a patch that enhances pthreadpool_tevent to create one
> > > tevent_threaded_context per registered event context instead of per
> > > pthreadpool_tevent_job_send request.
> > >
> > > Please review carefully -- the rundown is a bit tricky -- and push if
> > > happy. Thanks!
> > >
> > > Fwiw: it passed two private autobuilds and it survives smbtorture
> > > local.messaging tests with a modified messaging subsystem that sends out all
> > > messages via helper threads.
> >
> > OK, how about this version. That actual code is exactly as you
> > posted, all I've done is thoroughly commented it such that if
> > I had to maintain this I'd know immediately what it was doing :-).
> >
> > Let me know if this is OK !
>
> awesome! Thanks a lot!
>
> I actually think this deserves a pair-programmed-with: you and a signed-off-by:
> you. Let me know you're ok with this change.

Sure, if you want to that's OK - but it's not required :-).

Cheers,

        Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] pthreadpool: create a tevent_threaded_context per registered event context

Samba - samba-technical mailing list
On Thu, Nov 16, 2017 at 10:39:17AM -0800, Jeremy Allison wrote:
> Sure, if you want to that's OK - but it's not required :-).

pushed. Thanks again!

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/