[11/21] lustre: cl_object: remove vestigial debugging.
diff mbox series

Message ID 154949781313.10620.6363156783936746853.stgit@noble.brown
State New
Headers show
Series
  • lustre: Assorted cleanups for obdclass
Related show

Commit Message

NeilBrown Feb. 7, 2019, 12:03 a.m. UTC
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(-)

Comments

Andreas Dilger Feb. 8, 2019, 1:31 a.m. UTC | #1
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
NeilBrown Feb. 11, 2019, 12:48 a.m. UTC | #2
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
James Simmons Feb. 11, 2019, 2:04 a.m. UTC | #3
> 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();
> 
> 
>
NeilBrown Feb. 11, 2019, 3:25 a.m. UTC | #4
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();
>> 
>> 
>>
James Simmons Feb. 12, 2019, 5:19 a.m. UTC | #5
> >> 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();
> >> 
> >> 
> >> 
>
Patrick Farrell Feb. 12, 2019, 1:56 p.m. UTC | #6
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
NeilBrown Feb. 12, 2019, 10:12 p.m. UTC | #7
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();
>> >> 
>> >> 
>> >> 
>>
James Simmons Feb. 13, 2019, 12:19 a.m. UTC | #8
> >> >> 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();
> >> >> 
> >> >> 
> >> >> 
> >> 
>
NeilBrown Feb. 13, 2019, 12:29 a.m. UTC | #9
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();
>> >> >> 
>> >> >> 
>> >> >> 
>> >> 
>>

Patch
diff mbox series

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();