<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
</head>
<body>
If this state tracking has been off for as long as it has (I’ve never come across it before), I feel like it can probably go without a fight. I just don’t think the info provided is particularly important/useful.<br>
<br>
Trace points or even really just blunter tracing tools can probably get the desired info in a pinch...<br>
<br>
- Patrick
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> lustre-devel <lustre-devel-bounces@lists.lustre.org> on behalf of James Simmons <jsimmons@infradead.org><br>
<b>Sent:</b> Monday, February 11, 2019 11:19:45 PM<br>
<b>To:</b> NeilBrown<br>
<b>Cc:</b> Lustre Development List<br>
<b>Subject:</b> Re: [lustre-devel] [PATCH 11/21] lustre: cl_object: remove vestigial debugging.</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText"><br>
> >> cl_env_inc() and cl_env_dec() don't do anything,<br>
> >> so discard them.<br>
> ><br>
> > I don't know about this one. I saw Andreas response as well. <br>
> > So this was apart of "LU-744 obdclass: revise stats for cl_object cache"<br>
> > In the end it was turned off by default in the OpenSFS branch since it<br>
> > made performance take a hit. To enable in OpenSFS branch you have to run<br>
> > ./configure --enable-pgstate-track. We have a few like this for Lustre so<br>
> > I was going to bring this up. Do we want to remove this work for both the<br>
> > upstream client as well as OpenSFS tree or properly implement this by<br>
> > making it a Kconfig option when CONFIG_LUSTRE_DEBUG_EXPENSIVE_CHECK is<br>
> > enabled? Its just a matter of porting OpenSFS commit <br>
> > 5cae09a2409dcd396a1ee7be1eca7d3bbf77365e<br>
> ><br>
> > What do you think?<br>
> <br>
> How useful are these stats?<br>
> Stats that are never compiled in aren't likely to tell you much :-)<br>
> <br>
> Has any thought been given to per-cpu stats counting? That seems to be<br>
> the preferred approach for high-frequency accounting.<br>
> <br>
> I think having a CONFIG option to enable expensive consistency checks is<br>
> a good idea - developers will enable it when they don't care about<br>
> performance.<br>
> Having a CONFIG option for expensive performance stats seems... weird.<br>
> Who would use it, and how meaningful would the number be?<br>
<br>
Which per-cpu stats are you talking about? I know perf can do this kind of<br>
profiling with performance counters but I don't think you can use those<br>
with cl_pages specifically. I know the lustre developers really dislike<br>
trace point but this could be one of those cases where it makes sense.<br>
<br>
> NeilBrown<br>
> <br>
> <br>
> ><br>
> >> Signed-off-by: NeilBrown <neilb@suse.com><br>
> >> ---<br>
> >> drivers/staging/lustre/lustre/obdclass/cl_object.c | 15 ---------------<br>
> >> 1 file changed, 15 deletions(-)<br>
> >> <br>
> >> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c<br>
> >> index d71a680660da..1e704078664e 100644<br>
> >> --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c<br>
> >> +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c<br>
> >> @@ -565,14 +565,6 @@ struct cl_env {<br>
> >> void *ce_debug;<br>
> >> };<br>
> >> <br>
> >> -static void cl_env_inc(enum cache_stats_item item)<br>
> >> -{<br>
> >> -}<br>
> >> -<br>
> >> -static void cl_env_dec(enum cache_stats_item item)<br>
> >> -{<br>
> >> -}<br>
> >> -<br>
> >> static void cl_env_init0(struct cl_env *cle, void *debug)<br>
> >> {<br>
> >> LASSERT(cle->ce_ref == 0);<br>
> >> @@ -581,7 +573,6 @@ static void cl_env_init0(struct cl_env *cle, void *debug)<br>
> >> <br>
> >> cle->ce_ref = 1;<br>
> >> cle->ce_debug = debug;<br>
> >> - cl_env_inc(CS_busy);<br>
> >> }<br>
> >> <br>
> >> static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug)<br>
> >> @@ -611,9 +602,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug)<br>
> >> if (rc != 0) {<br>
> >> kmem_cache_free(cl_env_kmem, cle);<br>
> >> env = ERR_PTR(rc);<br>
> >> - } else {<br>
> >> - cl_env_inc(CS_create);<br>
> >> - cl_env_inc(CS_total);<br>
> >> }<br>
> >> } else {<br>
> >> env = ERR_PTR(-ENOMEM);<br>
> >> @@ -623,7 +611,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug)<br>
> >> <br>
> >> static void cl_env_fini(struct cl_env *cle)<br>
> >> {<br>
> >> - cl_env_dec(CS_total);<br>
> >> lu_context_fini(&cle->ce_lu.le_ctx);<br>
> >> lu_context_fini(&cle->ce_ses);<br>
> >> kmem_cache_free(cl_env_kmem, cle);<br>
> >> @@ -782,7 +769,6 @@ void cl_env_put(struct lu_env *env, u16 *refcheck)<br>
> >> if (--cle->ce_ref == 0) {<br>
> >> int cpu = get_cpu();<br>
> >> <br>
> >> - cl_env_dec(CS_busy);<br>
> >> cle->ce_debug = NULL;<br>
> >> cl_env_exit(cle);<br>
> >> /*<br>
> >> @@ -903,7 +889,6 @@ void cl_env_percpu_put(struct lu_env *env)<br>
> >> cle->ce_ref--;<br>
> >> LASSERT(cle->ce_ref == 0);<br>
> >> <br>
> >> - cl_env_dec(CS_busy);<br>
> >> cle->ce_debug = NULL;<br>
> >> <br>
> >> put_cpu();<br>
> >> <br>
> >> <br>
> >> <br>
> <br>
_______________________________________________<br>
lustre-devel mailing list<br>
lustre-devel@lists.lustre.org<br>
<a href="http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org">http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org</a><br>
</div>
</span></font></div>
</body>
</html>