| Message ID | 154949781313.10620.6363156783936746853.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lustre: Assorted cleanups for obdclass | expand |
On Feb 6, 2019, at 17:03, NeilBrown <neilb@suse.com> wrote: > > cl_env_inc() and cl_env_dec() don't do anything, > so discard them. > > Signed-off-by: NeilBrown <neilb@suse.com> It looks like these were being used for debugging page allocations, wrapped under CONFIG_DEBUG_PAGESTATE_TRACKING, but that was dropped from the upstream kernel. It looks like you could also delete cs_page_inc() and cs_page_dec() along with enum cache_stat_item, since they are also wrapped under the same CONFIG value. Reviewed-by: Andreas Dilger <adilger@whamcloud.com> > --- > drivers/staging/lustre/lustre/obdclass/cl_object.c | 15 --------------- > 1 file changed, 15 deletions(-) > > diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c > index d71a680660da..1e704078664e 100644 > --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c > +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c > @@ -565,14 +565,6 @@ struct cl_env { > void *ce_debug; > }; > > -static void cl_env_inc(enum cache_stats_item item) > -{ > -} > - > -static void cl_env_dec(enum cache_stats_item item) > -{ > -} > - > static void cl_env_init0(struct cl_env *cle, void *debug) > { > LASSERT(cle->ce_ref == 0); > @@ -581,7 +573,6 @@ static void cl_env_init0(struct cl_env *cle, void *debug) > > cle->ce_ref = 1; > cle->ce_debug = debug; > - cl_env_inc(CS_busy); > } > > static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > @@ -611,9 +602,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > if (rc != 0) { > kmem_cache_free(cl_env_kmem, cle); > env = ERR_PTR(rc); > - } else { > - cl_env_inc(CS_create); > - cl_env_inc(CS_total); > } > } else { > env = ERR_PTR(-ENOMEM); > @@ -623,7 +611,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > > static void cl_env_fini(struct cl_env *cle) > { > - cl_env_dec(CS_total); > lu_context_fini(&cle->ce_lu.le_ctx); > lu_context_fini(&cle->ce_ses); > kmem_cache_free(cl_env_kmem, cle); > @@ -782,7 +769,6 @@ void cl_env_put(struct lu_env *env, u16 *refcheck) > if (--cle->ce_ref == 0) { > int cpu = get_cpu(); > > - cl_env_dec(CS_busy); > cle->ce_debug = NULL; > cl_env_exit(cle); > /* > @@ -903,7 +889,6 @@ void cl_env_percpu_put(struct lu_env *env) > cle->ce_ref--; > LASSERT(cle->ce_ref == 0); > > - cl_env_dec(CS_busy); > cle->ce_debug = NULL; > > put_cpu(); > > Cheers, Andreas --- Andreas Dilger CTO Whamcloud
On Fri, Feb 08 2019, Andreas Dilger wrote: > On Feb 6, 2019, at 17:03, NeilBrown <neilb@suse.com> wrote: >> >> cl_env_inc() and cl_env_dec() don't do anything, >> so discard them. >> >> Signed-off-by: NeilBrown <neilb@suse.com> > > It looks like these were being used for debugging page allocations, > wrapped under CONFIG_DEBUG_PAGESTATE_TRACKING, but that was dropped > from the upstream kernel. It looks like you could also delete > cs_page_inc() and cs_page_dec() along with enum cache_stat_item, > since they are also wrapped under the same CONFIG value. cs_page_inc/dec were never in driver/staging. CS_PAGE_INC/DEC were for a while, but were removed nearly 4 years ago. Commit 76c807210aea ("staging: lustre: cl_page: delete empty macros") > > Reviewed-by: Andreas Dilger <adilger@whamcloud.com> Thanks, NeilBrown > >> --- >> drivers/staging/lustre/lustre/obdclass/cl_object.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> >> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c >> index d71a680660da..1e704078664e 100644 >> --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c >> +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c >> @@ -565,14 +565,6 @@ struct cl_env { >> void *ce_debug; >> }; >> >> -static void cl_env_inc(enum cache_stats_item item) >> -{ >> -} >> - >> -static void cl_env_dec(enum cache_stats_item item) >> -{ >> -} >> - >> static void cl_env_init0(struct cl_env *cle, void *debug) >> { >> LASSERT(cle->ce_ref == 0); >> @@ -581,7 +573,6 @@ static void cl_env_init0(struct cl_env *cle, void *debug) >> >> cle->ce_ref = 1; >> cle->ce_debug = debug; >> - cl_env_inc(CS_busy); >> } >> >> static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> @@ -611,9 +602,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> if (rc != 0) { >> kmem_cache_free(cl_env_kmem, cle); >> env = ERR_PTR(rc); >> - } else { >> - cl_env_inc(CS_create); >> - cl_env_inc(CS_total); >> } >> } else { >> env = ERR_PTR(-ENOMEM); >> @@ -623,7 +611,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> >> static void cl_env_fini(struct cl_env *cle) >> { >> - cl_env_dec(CS_total); >> lu_context_fini(&cle->ce_lu.le_ctx); >> lu_context_fini(&cle->ce_ses); >> kmem_cache_free(cl_env_kmem, cle); >> @@ -782,7 +769,6 @@ void cl_env_put(struct lu_env *env, u16 *refcheck) >> if (--cle->ce_ref == 0) { >> int cpu = get_cpu(); >> >> - cl_env_dec(CS_busy); >> cle->ce_debug = NULL; >> cl_env_exit(cle); >> /* >> @@ -903,7 +889,6 @@ void cl_env_percpu_put(struct lu_env *env) >> cle->ce_ref--; >> LASSERT(cle->ce_ref == 0); >> >> - cl_env_dec(CS_busy); >> cle->ce_debug = NULL; >> >> put_cpu(); >> >> > > Cheers, Andreas > --- > Andreas Dilger > CTO Whamcloud
> cl_env_inc() and cl_env_dec() don't do anything, > so discard them. I don't know about this one. I saw Andreas response as well. So this was apart of "LU-744 obdclass: revise stats for cl_object cache" In the end it was turned off by default in the OpenSFS branch since it made performance take a hit. To enable in OpenSFS branch you have to run ./configure --enable-pgstate-track. We have a few like this for Lustre so I was going to bring this up. Do we want to remove this work for both the upstream client as well as OpenSFS tree or properly implement this by making it a Kconfig option when CONFIG_LUSTRE_DEBUG_EXPENSIVE_CHECK is enabled? Its just a matter of porting OpenSFS commit 5cae09a2409dcd396a1ee7be1eca7d3bbf77365e What do you think? > Signed-off-by: NeilBrown <neilb@suse.com> > --- > drivers/staging/lustre/lustre/obdclass/cl_object.c | 15 --------------- > 1 file changed, 15 deletions(-) > > diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c > index d71a680660da..1e704078664e 100644 > --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c > +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c > @@ -565,14 +565,6 @@ struct cl_env { > void *ce_debug; > }; > > -static void cl_env_inc(enum cache_stats_item item) > -{ > -} > - > -static void cl_env_dec(enum cache_stats_item item) > -{ > -} > - > static void cl_env_init0(struct cl_env *cle, void *debug) > { > LASSERT(cle->ce_ref == 0); > @@ -581,7 +573,6 @@ static void cl_env_init0(struct cl_env *cle, void *debug) > > cle->ce_ref = 1; > cle->ce_debug = debug; > - cl_env_inc(CS_busy); > } > > static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > @@ -611,9 +602,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > if (rc != 0) { > kmem_cache_free(cl_env_kmem, cle); > env = ERR_PTR(rc); > - } else { > - cl_env_inc(CS_create); > - cl_env_inc(CS_total); > } > } else { > env = ERR_PTR(-ENOMEM); > @@ -623,7 +611,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > > static void cl_env_fini(struct cl_env *cle) > { > - cl_env_dec(CS_total); > lu_context_fini(&cle->ce_lu.le_ctx); > lu_context_fini(&cle->ce_ses); > kmem_cache_free(cl_env_kmem, cle); > @@ -782,7 +769,6 @@ void cl_env_put(struct lu_env *env, u16 *refcheck) > if (--cle->ce_ref == 0) { > int cpu = get_cpu(); > > - cl_env_dec(CS_busy); > cle->ce_debug = NULL; > cl_env_exit(cle); > /* > @@ -903,7 +889,6 @@ void cl_env_percpu_put(struct lu_env *env) > cle->ce_ref--; > LASSERT(cle->ce_ref == 0); > > - cl_env_dec(CS_busy); > cle->ce_debug = NULL; > > put_cpu(); > > >
On Mon, Feb 11 2019, James Simmons wrote: >> cl_env_inc() and cl_env_dec() don't do anything, >> so discard them. > > I don't know about this one. I saw Andreas response as well. > So this was apart of "LU-744 obdclass: revise stats for cl_object cache" > In the end it was turned off by default in the OpenSFS branch since it > made performance take a hit. To enable in OpenSFS branch you have to run > ./configure --enable-pgstate-track. We have a few like this for Lustre so > I was going to bring this up. Do we want to remove this work for both the > upstream client as well as OpenSFS tree or properly implement this by > making it a Kconfig option when CONFIG_LUSTRE_DEBUG_EXPENSIVE_CHECK is > enabled? Its just a matter of porting OpenSFS commit > 5cae09a2409dcd396a1ee7be1eca7d3bbf77365e > > What do you think? How useful are these stats? Stats that are never compiled in aren't likely to tell you much :-) Has any thought been given to per-cpu stats counting? That seems to be the preferred approach for high-frequency accounting. I think having a CONFIG option to enable expensive consistency checks is a good idea - developers will enable it when they don't care about performance. Having a CONFIG option for expensive performance stats seems... weird. Who would use it, and how meaningful would the number be? NeilBrown > >> Signed-off-by: NeilBrown <neilb@suse.com> >> --- >> drivers/staging/lustre/lustre/obdclass/cl_object.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> >> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c >> index d71a680660da..1e704078664e 100644 >> --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c >> +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c >> @@ -565,14 +565,6 @@ struct cl_env { >> void *ce_debug; >> }; >> >> -static void cl_env_inc(enum cache_stats_item item) >> -{ >> -} >> - >> -static void cl_env_dec(enum cache_stats_item item) >> -{ >> -} >> - >> static void cl_env_init0(struct cl_env *cle, void *debug) >> { >> LASSERT(cle->ce_ref == 0); >> @@ -581,7 +573,6 @@ static void cl_env_init0(struct cl_env *cle, void *debug) >> >> cle->ce_ref = 1; >> cle->ce_debug = debug; >> - cl_env_inc(CS_busy); >> } >> >> static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> @@ -611,9 +602,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> if (rc != 0) { >> kmem_cache_free(cl_env_kmem, cle); >> env = ERR_PTR(rc); >> - } else { >> - cl_env_inc(CS_create); >> - cl_env_inc(CS_total); >> } >> } else { >> env = ERR_PTR(-ENOMEM); >> @@ -623,7 +611,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> >> static void cl_env_fini(struct cl_env *cle) >> { >> - cl_env_dec(CS_total); >> lu_context_fini(&cle->ce_lu.le_ctx); >> lu_context_fini(&cle->ce_ses); >> kmem_cache_free(cl_env_kmem, cle); >> @@ -782,7 +769,6 @@ void cl_env_put(struct lu_env *env, u16 *refcheck) >> if (--cle->ce_ref == 0) { >> int cpu = get_cpu(); >> >> - cl_env_dec(CS_busy); >> cle->ce_debug = NULL; >> cl_env_exit(cle); >> /* >> @@ -903,7 +889,6 @@ void cl_env_percpu_put(struct lu_env *env) >> cle->ce_ref--; >> LASSERT(cle->ce_ref == 0); >> >> - cl_env_dec(CS_busy); >> cle->ce_debug = NULL; >> >> put_cpu(); >> >> >>
> >> cl_env_inc() and cl_env_dec() don't do anything, > >> so discard them. > > > > I don't know about this one. I saw Andreas response as well. > > So this was apart of "LU-744 obdclass: revise stats for cl_object cache" > > In the end it was turned off by default in the OpenSFS branch since it > > made performance take a hit. To enable in OpenSFS branch you have to run > > ./configure --enable-pgstate-track. We have a few like this for Lustre so > > I was going to bring this up. Do we want to remove this work for both the > > upstream client as well as OpenSFS tree or properly implement this by > > making it a Kconfig option when CONFIG_LUSTRE_DEBUG_EXPENSIVE_CHECK is > > enabled? Its just a matter of porting OpenSFS commit > > 5cae09a2409dcd396a1ee7be1eca7d3bbf77365e > > > > What do you think? > > How useful are these stats? > Stats that are never compiled in aren't likely to tell you much :-) > > Has any thought been given to per-cpu stats counting? That seems to be > the preferred approach for high-frequency accounting. > > I think having a CONFIG option to enable expensive consistency checks is > a good idea - developers will enable it when they don't care about > performance. > Having a CONFIG option for expensive performance stats seems... weird. > Who would use it, and how meaningful would the number be? Which per-cpu stats are you talking about? I know perf can do this kind of profiling with performance counters but I don't think you can use those with cl_pages specifically. I know the lustre developers really dislike trace point but this could be one of those cases where it makes sense. > NeilBrown > > > > > >> Signed-off-by: NeilBrown <neilb@suse.com> > >> --- > >> drivers/staging/lustre/lustre/obdclass/cl_object.c | 15 --------------- > >> 1 file changed, 15 deletions(-) > >> > >> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c > >> index d71a680660da..1e704078664e 100644 > >> --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c > >> +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c > >> @@ -565,14 +565,6 @@ struct cl_env { > >> void *ce_debug; > >> }; > >> > >> -static void cl_env_inc(enum cache_stats_item item) > >> -{ > >> -} > >> - > >> -static void cl_env_dec(enum cache_stats_item item) > >> -{ > >> -} > >> - > >> static void cl_env_init0(struct cl_env *cle, void *debug) > >> { > >> LASSERT(cle->ce_ref == 0); > >> @@ -581,7 +573,6 @@ static void cl_env_init0(struct cl_env *cle, void *debug) > >> > >> cle->ce_ref = 1; > >> cle->ce_debug = debug; > >> - cl_env_inc(CS_busy); > >> } > >> > >> static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > >> @@ -611,9 +602,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > >> if (rc != 0) { > >> kmem_cache_free(cl_env_kmem, cle); > >> env = ERR_PTR(rc); > >> - } else { > >> - cl_env_inc(CS_create); > >> - cl_env_inc(CS_total); > >> } > >> } else { > >> env = ERR_PTR(-ENOMEM); > >> @@ -623,7 +611,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > >> > >> static void cl_env_fini(struct cl_env *cle) > >> { > >> - cl_env_dec(CS_total); > >> lu_context_fini(&cle->ce_lu.le_ctx); > >> lu_context_fini(&cle->ce_ses); > >> kmem_cache_free(cl_env_kmem, cle); > >> @@ -782,7 +769,6 @@ void cl_env_put(struct lu_env *env, u16 *refcheck) > >> if (--cle->ce_ref == 0) { > >> int cpu = get_cpu(); > >> > >> - cl_env_dec(CS_busy); > >> cle->ce_debug = NULL; > >> cl_env_exit(cle); > >> /* > >> @@ -903,7 +889,6 @@ void cl_env_percpu_put(struct lu_env *env) > >> cle->ce_ref--; > >> LASSERT(cle->ce_ref == 0); > >> > >> - cl_env_dec(CS_busy); > >> cle->ce_debug = NULL; > >> > >> put_cpu(); > >> > >> > >> >
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. Trace points or even really just blunter tracing tools can probably get the desired info in a pinch... - Patrick
On Tue, Feb 12 2019, James Simmons wrote: >> >> cl_env_inc() and cl_env_dec() don't do anything, >> >> so discard them. >> > >> > I don't know about this one. I saw Andreas response as well. >> > So this was apart of "LU-744 obdclass: revise stats for cl_object cache" >> > In the end it was turned off by default in the OpenSFS branch since it >> > made performance take a hit. To enable in OpenSFS branch you have to run >> > ./configure --enable-pgstate-track. We have a few like this for Lustre so >> > I was going to bring this up. Do we want to remove this work for both the >> > upstream client as well as OpenSFS tree or properly implement this by >> > making it a Kconfig option when CONFIG_LUSTRE_DEBUG_EXPENSIVE_CHECK is >> > enabled? Its just a matter of porting OpenSFS commit >> > 5cae09a2409dcd396a1ee7be1eca7d3bbf77365e >> > >> > What do you think? >> >> How useful are these stats? >> Stats that are never compiled in aren't likely to tell you much :-) >> >> Has any thought been given to per-cpu stats counting? That seems to be >> the preferred approach for high-frequency accounting. >> >> I think having a CONFIG option to enable expensive consistency checks is >> a good idea - developers will enable it when they don't care about >> performance. >> Having a CONFIG option for expensive performance stats seems... weird. >> Who would use it, and how meaningful would the number be? > > Which per-cpu stats are you talking about? include/linux/percpu_counter.h I guess - or something similar. NeilBrown > I know perf can do this kind of > profiling with performance counters but I don't think you can use those > with cl_pages specifically. I know the lustre developers really dislike > trace point but this could be one of those cases where it makes sense. > >> NeilBrown >> >> >> > >> >> Signed-off-by: NeilBrown <neilb@suse.com> >> >> --- >> >> drivers/staging/lustre/lustre/obdclass/cl_object.c | 15 --------------- >> >> 1 file changed, 15 deletions(-) >> >> >> >> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c >> >> index d71a680660da..1e704078664e 100644 >> >> --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c >> >> +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c >> >> @@ -565,14 +565,6 @@ struct cl_env { >> >> void *ce_debug; >> >> }; >> >> >> >> -static void cl_env_inc(enum cache_stats_item item) >> >> -{ >> >> -} >> >> - >> >> -static void cl_env_dec(enum cache_stats_item item) >> >> -{ >> >> -} >> >> - >> >> static void cl_env_init0(struct cl_env *cle, void *debug) >> >> { >> >> LASSERT(cle->ce_ref == 0); >> >> @@ -581,7 +573,6 @@ static void cl_env_init0(struct cl_env *cle, void *debug) >> >> >> >> cle->ce_ref = 1; >> >> cle->ce_debug = debug; >> >> - cl_env_inc(CS_busy); >> >> } >> >> >> >> static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> >> @@ -611,9 +602,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> >> if (rc != 0) { >> >> kmem_cache_free(cl_env_kmem, cle); >> >> env = ERR_PTR(rc); >> >> - } else { >> >> - cl_env_inc(CS_create); >> >> - cl_env_inc(CS_total); >> >> } >> >> } else { >> >> env = ERR_PTR(-ENOMEM); >> >> @@ -623,7 +611,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> >> >> >> static void cl_env_fini(struct cl_env *cle) >> >> { >> >> - cl_env_dec(CS_total); >> >> lu_context_fini(&cle->ce_lu.le_ctx); >> >> lu_context_fini(&cle->ce_ses); >> >> kmem_cache_free(cl_env_kmem, cle); >> >> @@ -782,7 +769,6 @@ void cl_env_put(struct lu_env *env, u16 *refcheck) >> >> if (--cle->ce_ref == 0) { >> >> int cpu = get_cpu(); >> >> >> >> - cl_env_dec(CS_busy); >> >> cle->ce_debug = NULL; >> >> cl_env_exit(cle); >> >> /* >> >> @@ -903,7 +889,6 @@ void cl_env_percpu_put(struct lu_env *env) >> >> cle->ce_ref--; >> >> LASSERT(cle->ce_ref == 0); >> >> >> >> - cl_env_dec(CS_busy); >> >> cle->ce_debug = NULL; >> >> >> >> put_cpu(); >> >> >> >> >> >> >>
> >> >> cl_env_inc() and cl_env_dec() don't do anything, > >> >> so discard them. > >> > > >> > I don't know about this one. I saw Andreas response as well. > >> > So this was apart of "LU-744 obdclass: revise stats for cl_object cache" > >> > In the end it was turned off by default in the OpenSFS branch since it > >> > made performance take a hit. To enable in OpenSFS branch you have to run > >> > ./configure --enable-pgstate-track. We have a few like this for Lustre so > >> > I was going to bring this up. Do we want to remove this work for both the > >> > upstream client as well as OpenSFS tree or properly implement this by > >> > making it a Kconfig option when CONFIG_LUSTRE_DEBUG_EXPENSIVE_CHECK is > >> > enabled? Its just a matter of porting OpenSFS commit > >> > 5cae09a2409dcd396a1ee7be1eca7d3bbf77365e > >> > > >> > What do you think? > >> > >> How useful are these stats? > >> Stats that are never compiled in aren't likely to tell you much :-) > >> > >> Has any thought been given to per-cpu stats counting? That seems to be > >> the preferred approach for high-frequency accounting. > >> > >> I think having a CONFIG option to enable expensive consistency checks is > >> a good idea - developers will enable it when they don't care about > >> performance. > >> Having a CONFIG option for expensive performance stats seems... weird. > >> Who would use it, and how meaningful would the number be? > > > > Which per-cpu stats are you talking about? > > include/linux/percpu_counter.h I guess - or something similar. Ouch 4K per counter. This would crush our clients. Does this scale well? I also looked at the page counter which also atomic counting orientated. I expect that too also to struggle. The page counter don't match as well as what these cl_page stats monitor. > > I know perf can do this kind of > > profiling with performance counters but I don't think you can use those > > with cl_pages specifically. I know the lustre developers really dislike > > trace point but this could be one of those cases where it makes sense. > > > >> NeilBrown > >> > >> > >> > > >> >> Signed-off-by: NeilBrown <neilb@suse.com> > >> >> --- > >> >> drivers/staging/lustre/lustre/obdclass/cl_object.c | 15 --------------- > >> >> 1 file changed, 15 deletions(-) > >> >> > >> >> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c > >> >> index d71a680660da..1e704078664e 100644 > >> >> --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c > >> >> +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c > >> >> @@ -565,14 +565,6 @@ struct cl_env { > >> >> void *ce_debug; > >> >> }; > >> >> > >> >> -static void cl_env_inc(enum cache_stats_item item) > >> >> -{ > >> >> -} > >> >> - > >> >> -static void cl_env_dec(enum cache_stats_item item) > >> >> -{ > >> >> -} > >> >> - > >> >> static void cl_env_init0(struct cl_env *cle, void *debug) > >> >> { > >> >> LASSERT(cle->ce_ref == 0); > >> >> @@ -581,7 +573,6 @@ static void cl_env_init0(struct cl_env *cle, void *debug) > >> >> > >> >> cle->ce_ref = 1; > >> >> cle->ce_debug = debug; > >> >> - cl_env_inc(CS_busy); > >> >> } > >> >> > >> >> static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > >> >> @@ -611,9 +602,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > >> >> if (rc != 0) { > >> >> kmem_cache_free(cl_env_kmem, cle); > >> >> env = ERR_PTR(rc); > >> >> - } else { > >> >> - cl_env_inc(CS_create); > >> >> - cl_env_inc(CS_total); > >> >> } > >> >> } else { > >> >> env = ERR_PTR(-ENOMEM); > >> >> @@ -623,7 +611,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) > >> >> > >> >> static void cl_env_fini(struct cl_env *cle) > >> >> { > >> >> - cl_env_dec(CS_total); > >> >> lu_context_fini(&cle->ce_lu.le_ctx); > >> >> lu_context_fini(&cle->ce_ses); > >> >> kmem_cache_free(cl_env_kmem, cle); > >> >> @@ -782,7 +769,6 @@ void cl_env_put(struct lu_env *env, u16 *refcheck) > >> >> if (--cle->ce_ref == 0) { > >> >> int cpu = get_cpu(); > >> >> > >> >> - cl_env_dec(CS_busy); > >> >> cle->ce_debug = NULL; > >> >> cl_env_exit(cle); > >> >> /* > >> >> @@ -903,7 +889,6 @@ void cl_env_percpu_put(struct lu_env *env) > >> >> cle->ce_ref--; > >> >> LASSERT(cle->ce_ref == 0); > >> >> > >> >> - cl_env_dec(CS_busy); > >> >> cle->ce_debug = NULL; > >> >> > >> >> put_cpu(); > >> >> > >> >> > >> >> > >> >
On Wed, Feb 13 2019, James Simmons wrote: >> >> >> cl_env_inc() and cl_env_dec() don't do anything, >> >> >> so discard them. >> >> > >> >> > I don't know about this one. I saw Andreas response as well. >> >> > So this was apart of "LU-744 obdclass: revise stats for cl_object cache" >> >> > In the end it was turned off by default in the OpenSFS branch since it >> >> > made performance take a hit. To enable in OpenSFS branch you have to run >> >> > ./configure --enable-pgstate-track. We have a few like this for Lustre so >> >> > I was going to bring this up. Do we want to remove this work for both the >> >> > upstream client as well as OpenSFS tree or properly implement this by >> >> > making it a Kconfig option when CONFIG_LUSTRE_DEBUG_EXPENSIVE_CHECK is >> >> > enabled? Its just a matter of porting OpenSFS commit >> >> > 5cae09a2409dcd396a1ee7be1eca7d3bbf77365e >> >> > >> >> > What do you think? >> >> >> >> How useful are these stats? >> >> Stats that are never compiled in aren't likely to tell you much :-) >> >> >> >> Has any thought been given to per-cpu stats counting? That seems to be >> >> the preferred approach for high-frequency accounting. >> >> >> >> I think having a CONFIG option to enable expensive consistency checks is >> >> a good idea - developers will enable it when they don't care about >> >> performance. >> >> Having a CONFIG option for expensive performance stats seems... weird. >> >> Who would use it, and how meaningful would the number be? >> > >> > Which per-cpu stats are you talking about? >> >> include/linux/percpu_counter.h I guess - or something similar. > > Ouch 4K per counter. This would crush our clients. Does this scale well? > I also looked at the page counter which also atomic counting orientated. > I expect that too also to struggle. The page counter don't match as well > as what these cl_page stats monitor. Why? How many counters are there? enum cl_page_state defines 5 per "site". enum cache_stats_item defines 5, which think are global. That makes 4, to order of 16K. That isn't all that much. NeilBrown > >> > I know perf can do this kind of >> > profiling with performance counters but I don't think you can use those >> > with cl_pages specifically. I know the lustre developers really dislike >> > trace point but this could be one of those cases where it makes sense. >> > >> >> NeilBrown >> >> >> >> >> >> > >> >> >> Signed-off-by: NeilBrown <neilb@suse.com> >> >> >> --- >> >> >> drivers/staging/lustre/lustre/obdclass/cl_object.c | 15 --------------- >> >> >> 1 file changed, 15 deletions(-) >> >> >> >> >> >> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c >> >> >> index d71a680660da..1e704078664e 100644 >> >> >> --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c >> >> >> +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c >> >> >> @@ -565,14 +565,6 @@ struct cl_env { >> >> >> void *ce_debug; >> >> >> }; >> >> >> >> >> >> -static void cl_env_inc(enum cache_stats_item item) >> >> >> -{ >> >> >> -} >> >> >> - >> >> >> -static void cl_env_dec(enum cache_stats_item item) >> >> >> -{ >> >> >> -} >> >> >> - >> >> >> static void cl_env_init0(struct cl_env *cle, void *debug) >> >> >> { >> >> >> LASSERT(cle->ce_ref == 0); >> >> >> @@ -581,7 +573,6 @@ static void cl_env_init0(struct cl_env *cle, void *debug) >> >> >> >> >> >> cle->ce_ref = 1; >> >> >> cle->ce_debug = debug; >> >> >> - cl_env_inc(CS_busy); >> >> >> } >> >> >> >> >> >> static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> >> >> @@ -611,9 +602,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> >> >> if (rc != 0) { >> >> >> kmem_cache_free(cl_env_kmem, cle); >> >> >> env = ERR_PTR(rc); >> >> >> - } else { >> >> >> - cl_env_inc(CS_create); >> >> >> - cl_env_inc(CS_total); >> >> >> } >> >> >> } else { >> >> >> env = ERR_PTR(-ENOMEM); >> >> >> @@ -623,7 +611,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) >> >> >> >> >> >> static void cl_env_fini(struct cl_env *cle) >> >> >> { >> >> >> - cl_env_dec(CS_total); >> >> >> lu_context_fini(&cle->ce_lu.le_ctx); >> >> >> lu_context_fini(&cle->ce_ses); >> >> >> kmem_cache_free(cl_env_kmem, cle); >> >> >> @@ -782,7 +769,6 @@ void cl_env_put(struct lu_env *env, u16 *refcheck) >> >> >> if (--cle->ce_ref == 0) { >> >> >> int cpu = get_cpu(); >> >> >> >> >> >> - cl_env_dec(CS_busy); >> >> >> cle->ce_debug = NULL; >> >> >> cl_env_exit(cle); >> >> >> /* >> >> >> @@ -903,7 +889,6 @@ void cl_env_percpu_put(struct lu_env *env) >> >> >> cle->ce_ref--; >> >> >> LASSERT(cle->ce_ref == 0); >> >> >> >> >> >> - cl_env_dec(CS_busy); >> >> >> cle->ce_debug = NULL; >> >> >> >> >> >> put_cpu(); >> >> >> >> >> >> >> >> >> >> >> >>
diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c index d71a680660da..1e704078664e 100644 --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c @@ -565,14 +565,6 @@ struct cl_env { void *ce_debug; }; -static void cl_env_inc(enum cache_stats_item item) -{ -} - -static void cl_env_dec(enum cache_stats_item item) -{ -} - static void cl_env_init0(struct cl_env *cle, void *debug) { LASSERT(cle->ce_ref == 0); @@ -581,7 +573,6 @@ static void cl_env_init0(struct cl_env *cle, void *debug) cle->ce_ref = 1; cle->ce_debug = debug; - cl_env_inc(CS_busy); } static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) @@ -611,9 +602,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) if (rc != 0) { kmem_cache_free(cl_env_kmem, cle); env = ERR_PTR(rc); - } else { - cl_env_inc(CS_create); - cl_env_inc(CS_total); } } else { env = ERR_PTR(-ENOMEM); @@ -623,7 +611,6 @@ static struct lu_env *cl_env_new(u32 ctx_tags, u32 ses_tags, void *debug) static void cl_env_fini(struct cl_env *cle) { - cl_env_dec(CS_total); lu_context_fini(&cle->ce_lu.le_ctx); lu_context_fini(&cle->ce_ses); kmem_cache_free(cl_env_kmem, cle); @@ -782,7 +769,6 @@ void cl_env_put(struct lu_env *env, u16 *refcheck) if (--cle->ce_ref == 0) { int cpu = get_cpu(); - cl_env_dec(CS_busy); cle->ce_debug = NULL; cl_env_exit(cle); /* @@ -903,7 +889,6 @@ void cl_env_percpu_put(struct lu_env *env) cle->ce_ref--; LASSERT(cle->ce_ref == 0); - cl_env_dec(CS_busy); cle->ce_debug = NULL; put_cpu();
cl_env_inc() and cl_env_dec() don't do anything, so discard them. Signed-off-by: NeilBrown <neilb@suse.com> --- drivers/staging/lustre/lustre/obdclass/cl_object.c | 15 --------------- 1 file changed, 15 deletions(-)