[lustre-devel] [PATCH 11/21] lustre: cl_object: remove vestigial debugging.
NeilBrown
neilb at suse.com
Tue Feb 12 16:29:59 PST 2019
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 at 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();
>> >> >>
>> >> >>
>> >> >>
>> >>
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20190213/97d7afdd/attachment.sig>
More information about the lustre-devel
mailing list