| Message ID | 5f557caee004f22cee33e8753063f0315459d7e1.1631738177.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Builtin FSMonitor Part 1 | expand |
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Jeff Hostetler <jeffhost@microsoft.com> > > Do not leak the thread name (contained within the thread context) when > a thread terminates. > > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> > --- > trace2/tr2_tls.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c > index 067c23755fb..7da94aba522 100644 > --- a/trace2/tr2_tls.c > +++ b/trace2/tr2_tls.c > @@ -95,6 +95,7 @@ void tr2tls_unset_self(void) > > pthread_setspecific(tr2tls_key, NULL); > > + strbuf_release(&ctx->thread_name); > free(ctx->array_us_start); > free(ctx); > } So the idea is create allocates a new instance, and unset is to release the resource held by it? This is not a problem in _this_ patch but more about the base code that is being fixed here, but using strbuf as thread_name member sends a strong signal that it is designed to be inexpensive to change thread_name after a context is created by create_self. If it is not the case, the member should be "const char *", which may be computed using a temporary strbuf in create_self() and taken from the strbuf with strbuf_detach(), I would think. Thanks.
On Wed, Sep 15, 2021 at 02:01:39PM -0700, Junio C Hamano wrote: > "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Jeff Hostetler <jeffhost@microsoft.com> > > > > Do not leak the thread name (contained within the thread context) when > > a thread terminates. > > > > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> > > --- > > trace2/tr2_tls.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c > > index 067c23755fb..7da94aba522 100644 > > --- a/trace2/tr2_tls.c > > +++ b/trace2/tr2_tls.c > > @@ -95,6 +95,7 @@ void tr2tls_unset_self(void) > > > > pthread_setspecific(tr2tls_key, NULL); > > > > + strbuf_release(&ctx->thread_name); > > free(ctx->array_us_start); > > free(ctx); > > } > > So the idea is create allocates a new instance, and unset is to > release the resource held by it? > > This is not a problem in _this_ patch but more about the base code > that is being fixed here, but using strbuf as thread_name member > sends a strong signal that it is designed to be inexpensive to > change thread_name after a context is created by create_self. If > it is not the case, the member should be "const char *", which may > be computed using a temporary strbuf in create_self() and taken from > the strbuf with strbuf_detach(), I would think. It looks like we do not change the contents of the thread_name buffer anywhere. I assume that we store the strbuf in struct tr2tls_thread_ctx because we do some string formatting in tr2tls_create_self(). But there we could easily say ctx->thread_name = strbuf_release(&buf). More concerning to me is that we use signed integers to keep track of nr and alloc of array_us_start. But both of these are separate issues and don't need to be dealt with here. Thanks, Taylor
On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote: > From: Jeff Hostetler <jeffhost@microsoft.com> > > Do not leak the thread name (contained within the thread context) when > a thread terminates. > > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> > --- > trace2/tr2_tls.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c > index 067c23755fb..7da94aba522 100644 > --- a/trace2/tr2_tls.c > +++ b/trace2/tr2_tls.c > @@ -95,6 +95,7 @@ void tr2tls_unset_self(void) > > pthread_setspecific(tr2tls_key, NULL); > > + strbuf_release(&ctx->thread_name); > free(ctx->array_us_start); > free(ctx); > } As noted in your cover letter: * A fix for a memory leak in the Trace2 code. (This was independently reported in last week in "ab/tr2-leaks-and-fixes".) So I think this patch can be dropped from this series, since it's exact duplicate of my 48f68715b14 (tr2: stop leaking "thread_name" memory, 2021-08-27) in ab/tr2-leaks-and-fixes, currently in "next" and marked for a merge with master. When submitting a series that depends on another one it's best to rebase it on top of it & indicate it as such in the cover letter, Junio can queue such a series on top of another one. In this case I'm still not sure why this fix is here, i.e. surely nothing later in the series absolutely needs this stray memory leak fix...
On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote: > So I think this patch can be dropped from this series, since it's exact > duplicate of my 48f68715b14 (tr2: stop leaking "thread_name" memory, > 2021-08-27) in ab/tr2-leaks-and-fixes, currently in "next" and marked > for a merge with master. I agree it can be dropped. > When submitting a series that depends on another one it's best to rebase > it on top of it & indicate it as such in the cover letter, Junio can > queue such a series on top of another one. > > In this case I'm still not sure why this fix is here, i.e. surely > nothing later in the series absolutely needs this stray memory leak > fix... But there's no need for Jeff to depend on your branch, since (as you mentioned) this cleanup isn't relevant for anything else in this series, which is a sort of grab-bag of miscellaneous clean-ups. Thanks, Taylor
On Thu, Sep 16 2021, Taylor Blau wrote: > On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote: >> So I think this patch can be dropped from this series, since it's exact >> duplicate of my 48f68715b14 (tr2: stop leaking "thread_name" memory, >> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in "next" and marked >> for a merge with master. > > I agree it can be dropped. > >> When submitting a series that depends on another one it's best to rebase >> it on top of it & indicate it as such in the cover letter, Junio can >> queue such a series on top of another one. >> >> In this case I'm still not sure why this fix is here, i.e. surely >> nothing later in the series absolutely needs this stray memory leak >> fix... > > But there's no need for Jeff to depend on your branch, since (as you > mentioned) this cleanup isn't relevant for anything else in this series, > which is a sort of grab-bag of miscellaneous clean-ups. Indeed, to be clear it was just general advice about queue-on-top. But to clarify what I was getting at here: If we just came up with the same diff I'd have assumed Jeff just hadn't need the change in "next", but since he clearly has I was confused by it being here. I.e. it doesn't *seem* like anything in the rest of the series depends on it, so why have it here at all since the bug is being fixed anyway? Or if it does depend on it in some subtle way I've missed, perhaps it does need to be queued on top of ab/tr2-leaks-and-fixes, and the relevant commit/subtle dependency needs to be called out in a commit message. Or maybe Jeff had just come up with this independently, noticed it just before submission and just updated the CL, not the patch or series itself :)
On 9/16/21 4:01 AM, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Sep 16 2021, Taylor Blau wrote: > >> On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote: >>> So I think this patch can be dropped from this series, since it's exact >>> duplicate of my 48f68715b14 (tr2: stop leaking "thread_name" memory, >>> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in "next" and marked >>> for a merge with master. >> >> I agree it can be dropped. >> >>> When submitting a series that depends on another one it's best to rebase >>> it on top of it & indicate it as such in the cover letter, Junio can >>> queue such a series on top of another one. >>> >>> In this case I'm still not sure why this fix is here, i.e. surely >>> nothing later in the series absolutely needs this stray memory leak >>> fix... >> >> But there's no need for Jeff to depend on your branch, since (as you >> mentioned) this cleanup isn't relevant for anything else in this series, >> which is a sort of grab-bag of miscellaneous clean-ups. > > Indeed, to be clear it was just general advice about queue-on-top. > > But to clarify what I was getting at here: If we just came up with the > same diff I'd have assumed Jeff just hadn't need the change in "next", > but since he clearly has I was confused by it being here. > > I.e. it doesn't *seem* like anything in the rest of the series depends > on it, so why have it here at all since the bug is being fixed anyway? > Or if it does depend on it in some subtle way I've missed, perhaps it > does need to be queued on top of ab/tr2-leaks-and-fixes, and the > relevant commit/subtle dependency needs to be called out in a commit > message. > > Or maybe Jeff had just come up with this independently, noticed it just > before submission and just updated the CL, not the patch or series > itself :) > I'll drop this commit since your version is already queued up and headed to master. I've been carrying it in my dev branch for a while and was using it to make leak reporting a little quieter. And yes, I just noticed that yours had advanced when I wrote the cover letter and ACKd it rather than dropping it. And no, nothing in the rest of the whole FSMonitor series depends on this, so I can leave my series based upon master rather than your branch. Thanks Jeff
On Thu, Sep 16 2021, Jeff Hostetler wrote: > On 9/16/21 4:01 AM, Ævar Arnfjörð Bjarmason wrote: >> On Thu, Sep 16 2021, Taylor Blau wrote: >> >>> On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote: >>>> So I think this patch can be dropped from this series, since it's exact >>>> duplicate of my 48f68715b14 (tr2: stop leaking "thread_name" memory, >>>> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in "next" and marked >>>> for a merge with master. >>> >>> I agree it can be dropped. >>> >>>> When submitting a series that depends on another one it's best to rebase >>>> it on top of it & indicate it as such in the cover letter, Junio can >>>> queue such a series on top of another one. >>>> >>>> In this case I'm still not sure why this fix is here, i.e. surely >>>> nothing later in the series absolutely needs this stray memory leak >>>> fix... >>> >>> But there's no need for Jeff to depend on your branch, since (as you >>> mentioned) this cleanup isn't relevant for anything else in this series, >>> which is a sort of grab-bag of miscellaneous clean-ups. >> Indeed, to be clear it was just general advice about queue-on-top. >> But to clarify what I was getting at here: If we just came up with >> the >> same diff I'd have assumed Jeff just hadn't need the change in "next", >> but since he clearly has I was confused by it being here. >> I.e. it doesn't *seem* like anything in the rest of the series >> depends >> on it, so why have it here at all since the bug is being fixed anyway? >> Or if it does depend on it in some subtle way I've missed, perhaps it >> does need to be queued on top of ab/tr2-leaks-and-fixes, and the >> relevant commit/subtle dependency needs to be called out in a commit >> message. >> Or maybe Jeff had just come up with this independently, noticed it >> just >> before submission and just updated the CL, not the patch or series >> itself :) >> > > I'll drop this commit since your version is already queued up > and headed to master. I've been carrying it in my dev branch > for a while and was using it to make leak reporting a little > quieter. > > And yes, I just noticed that yours had advanced when I wrote the > cover letter and ACKd it rather than dropping it. > > And no, nothing in the rest of the whole FSMonitor series depends > on this, so I can leave my series based upon master rather than > your branch. > > Thanks > Jeff Sounds good, thanks for clarifying. In any case by the time you'll re-roll this (or soon thereafter) Junio will probably have merged it down anyway.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Indeed, to be clear it was just general advice about queue-on-top. > > But to clarify what I was getting at here: If we just came up with the > same diff I'd have assumed Jeff just hadn't need the change in "next", > but since he clearly has I was confused by it being here. > > I.e. it doesn't *seem* like anything in the rest of the series depends > on it, so why have it here at all since the bug is being fixed anyway? I'd imagine that it was there just for the same reason series from some people (yours included) tend to bloat, either over iterations or from day one, by including "this is not necessary for the end goal of this topic at all, but since I noticed it, I am including this fix, which should be obvious enough" unrelated fix. Here is a lesson to be learned.
diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c index 067c23755fb..7da94aba522 100644 --- a/trace2/tr2_tls.c +++ b/trace2/tr2_tls.c @@ -95,6 +95,7 @@ void tr2tls_unset_self(void) pthread_setspecific(tr2tls_key, NULL); + strbuf_release(&ctx->thread_name); free(ctx->array_us_start); free(ctx); }