Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1370726 > unrolled thread

Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper functions

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Tejun Heo <tj@kernel.org> - 2016-04-04 18:10 +0200
    Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Waiman Long <waiman.long@hpe.com> - 2016-04-06 22:10 +0200
    Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Waiman Long <waiman.long@hpe.com> - 2016-04-07 00:00 +0200
      Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Tejun Heo <tj@kernel.org> - 2016-04-07 01:00 +0200
        Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Waiman Long <waiman.long@hpe.com> - 2016-04-07 18:00 +0200
          Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Tejun Heo <tj@kernel.org> - 2016-04-07 18:10 +0200
            Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Tejun Heo <tj@kernel.org> - 2016-04-07 21:00 +0200
              Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Waiman Long <waiman.long@hpe.com> - 2016-04-07 22:40 +0200
                Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Tejun Heo <tj@kernel.org> - 2016-04-07 22:50 +0200
                  Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Waiman Long <waiman.long@hpe.com> - 2016-04-07 23:40 +0200
            Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper  functions Waiman Long <waiman.long@hpe.com> - 2016-04-07 21:10 +0200

#1370726 — Re: [PATCH 2/3] percpu_stats: Simple per-cpu statistics count helper functions

Hello,

On Fri, Apr 01, 2016 at 11:09:37PM -0400, Waiman Long wrote:
...
> +struct percpu_stats {
> +	unsigned long __percpu *stats;

I'm not sure ulong is the best choice here.  Atomic reads on 32bit are
nice but people often need 64bit counters for stats.  It probably is a
better idea to use u64_stats_sync.

> +/*
> + * Reset the all statistics counts to 0 in the percpu_stats structure

Proper function description please.

> + */
> +static inline void percpu_stats_reset(struct percpu_stats *pcs)

Why is this function inline?

> +{
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		unsigned long *pstats =  per_cpu_ptr(pcs->stats, cpu);
                                       ^^
> +		int stat;
> +
> +		for (stat = 0; stat < pcs->nstats; stat++, pstats++)
> +			*pstats = 0;
> +	}
> +
> +	/*
> +	 * If a statistics count is in the middle of being updated, it
> +	 * is possible that the above clearing may not work. So we need
> +	 * to double check again to make sure that the counters are really
> +	 * cleared. Still there is a still a very small chance that the
> +	 * second clearing does not work.
> +	 */
> +	for_each_possible_cpu(cpu) {
> +		unsigned long *pstats =  per_cpu_ptr(pcs->stats, cpu);
> +		int stat;
> +
> +		for (stat = 0; stat < pcs->nstats; stat++, pstats++)
> +			if (*pstats)
> +				*pstats = 0;
> +	}

I don't think this is acceptable.

> +}
> +
> +static inline int percpu_stats_init(struct percpu_stats *pcs, int num)
> +{
> +	pcs->nstats = num;
> +	pcs->stats  = __alloc_percpu(sizeof(unsigned long) * num,
> +				     __alignof__(unsigned long));
> +	if (!pcs->stats)
> +		return -ENOMEM;
> +
> +	percpu_stats_reset(pcs);
> +	return 0;
> +}
> +
> +static inline void percpu_stats_destroy(struct percpu_stats *pcs)
> +{
> +	free_percpu(pcs->stats);
> +	pcs->stats  = NULL;
> +	pcs->nstats = 0;
> +}

Why inline the above functions?

> +static inline void
> +__percpu_stats_add(struct percpu_stats *pcs, int stat, int cnt)
> +{
> +	unsigned long *pstat;
> +
> +	if ((unsigned int)stat >= pcs->nstats)
> +		return;

This is a critical bug.  Please don't fail silently.  BUG_ON(),
please.

> +	preempt_disable();
> +	pstat = this_cpu_ptr(&pcs->stats[stat]);
> +	*pstat += cnt;
> +	preempt_enable();
> +}

this_cpu_add() is atomic w.r.t. local operations.

> +static inline unsigned long
> +percpu_stats_sum(struct percpu_stats *pcs, int stat)
> +{
> +	int cpu;
> +	unsigned long sum = 0;
> +
> +	if ((unsigned int)stat >= pcs->nstats)
> +		return sum;

Ditto.

> +	for_each_possible_cpu(cpu)
> +		sum += per_cpu(pcs->stats[stat], cpu);
> +	return sum;
> +}

Thanks.

-- 
tejun

[toc] | [next] | [standalone]

#1372791

On 04/04/2016 12:02 PM, Tejun Heo wrote:
> Hello,
>
> On Fri, Apr 01, 2016 at 11:09:37PM -0400, Waiman Long wrote:
> ...
>> +struct percpu_stats {
>> +	unsigned long __percpu *stats;
> I'm not sure ulong is the best choice here.  Atomic reads on 32bit are
> nice but people often need 64bit counters for stats.  It probably is a
> better idea to use u64_stats_sync.
>
>> +/*
>> + * Reset the all statistics counts to 0 in the percpu_stats structure
> Proper function description please.
>
>> + */
>> +static inline void percpu_stats_reset(struct percpu_stats *pcs)
> Why is this function inline?
>
>> +{
>> +	int cpu;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		unsigned long *pstats =  per_cpu_ptr(pcs->stats, cpu);
>                                         ^^
>> +		int stat;
>> +
>> +		for (stat = 0; stat<  pcs->nstats; stat++, pstats++)
>> +			*pstats = 0;
>> +	}
>> +
>> +	/*
>> +	 * If a statistics count is in the middle of being updated, it
>> +	 * is possible that the above clearing may not work. So we need
>> +	 * to double check again to make sure that the counters are really
>> +	 * cleared. Still there is a still a very small chance that the
>> +	 * second clearing does not work.
>> +	 */
>> +	for_each_possible_cpu(cpu) {
>> +		unsigned long *pstats =  per_cpu_ptr(pcs->stats, cpu);
>> +		int stat;
>> +
>> +		for (stat = 0; stat<  pcs->nstats; stat++, pstats++)
>> +			if (*pstats)
>> +				*pstats = 0;
>> +	}
> I don't think this is acceptable.
>
>> +}
>> +
>> +static inline int percpu_stats_init(struct percpu_stats *pcs, int num)
>> +{
>> +	pcs->nstats = num;
>> +	pcs->stats  = __alloc_percpu(sizeof(unsigned long) * num,
>> +				     __alignof__(unsigned long));
>> +	if (!pcs->stats)
>> +		return -ENOMEM;
>> +
>> +	percpu_stats_reset(pcs);
>> +	return 0;
>> +}
>> +
>> +static inline void percpu_stats_destroy(struct percpu_stats *pcs)
>> +{
>> +	free_percpu(pcs->stats);
>> +	pcs->stats  = NULL;
>> +	pcs->nstats = 0;
>> +}
> Why inline the above functions?
>
>> +static inline void
>> +__percpu_stats_add(struct percpu_stats *pcs, int stat, int cnt)
>> +{
>> +	unsigned long *pstat;
>> +
>> +	if ((unsigned int)stat>= pcs->nstats)
>> +		return;
> This is a critical bug.  Please don't fail silently.  BUG_ON(),
> please.

Sure.

>
>> +	preempt_disable();
>> +	pstat = this_cpu_ptr(&pcs->stats[stat]);
>> +	*pstat += cnt;
>> +	preempt_enable();
>> +}
> this_cpu_add() is atomic w.r.t. local operations.

Will use this_cpu_add().

>> +static inline unsigned long
>> +percpu_stats_sum(struct percpu_stats *pcs, int stat)
>> +{
>> +	int cpu;
>> +	unsigned long sum = 0;
>> +
>> +	if ((unsigned int)stat>= pcs->nstats)
>> +		return sum;
> Ditto.
>
>> +	for_each_possible_cpu(cpu)
>> +		sum += per_cpu(pcs->stats[stat], cpu);
>> +	return sum;
>> +}
> Thanks.
>

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1372883

On 04/04/2016 12:02 PM, Tejun Heo wrote:
> Hello,
>
> On Fri, Apr 01, 2016 at 11:09:37PM -0400, Waiman Long wrote:
> ...
>> +struct percpu_stats {
>> +	unsigned long __percpu *stats;
> I'm not sure ulong is the best choice here.  Atomic reads on 32bit are
> nice but people often need 64bit counters for stats.  It probably is a
> better idea to use u64_stats_sync.

Got that, will incorporate 64-bit counter support for 32-bit architecture.

>> +/*
>> + * Reset the all statistics counts to 0 in the percpu_stats structure
> Proper function description please.

Sure. Will do that for all the functions.

>> + */
>> +static inline void percpu_stats_reset(struct percpu_stats *pcs)
> Why is this function inline?

It doesn't need to be inlined, but I need to add a lib/percpu_stats.c 
file to hold the function which I will do in my v2 patch.

>
>> +{
>> +	int cpu;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		unsigned long *pstats =  per_cpu_ptr(pcs->stats, cpu);
>                                         ^^
>> +		int stat;
>> +
>> +		for (stat = 0; stat<  pcs->nstats; stat++, pstats++)
>> +			*pstats = 0;
>> +	}
>> +
>> +	/*
>> +	 * If a statistics count is in the middle of being updated, it
>> +	 * is possible that the above clearing may not work. So we need
>> +	 * to double check again to make sure that the counters are really
>> +	 * cleared. Still there is a still a very small chance that the
>> +	 * second clearing does not work.
>> +	 */
>> +	for_each_possible_cpu(cpu) {
>> +		unsigned long *pstats =  per_cpu_ptr(pcs->stats, cpu);
>> +		int stat;
>> +
>> +		for (stat = 0; stat<  pcs->nstats; stat++, pstats++)
>> +			if (*pstats)
>> +				*pstats = 0;
>> +	}
> I don't think this is acceptable.

I am not sure what you mean here by not acceptable. Please enlighten me 
on that.

>> +}
>> +
>> +static inline int percpu_stats_init(struct percpu_stats *pcs, int num)
>> +{
>> +	pcs->nstats = num;
>> +	pcs->stats  = __alloc_percpu(sizeof(unsigned long) * num,
>> +				     __alignof__(unsigned long));
>> +	if (!pcs->stats)
>> +		return -ENOMEM;
>> +
>> +	percpu_stats_reset(pcs);
>> +	return 0;
>> +}
>> +
>> +static inline void percpu_stats_destroy(struct percpu_stats *pcs)
>> +{
>> +	free_percpu(pcs->stats);
>> +	pcs->stats  = NULL;
>> +	pcs->nstats = 0;
>> +}
> Why inline the above functions?

Will move this function to lib/percpu_stats.c.

>> +static inline void
>> +__percpu_stats_add(struct percpu_stats *pcs, int stat, int cnt)
>> +{
>> +	unsigned long *pstat;
>> +
>> +	if ((unsigned int)stat>= pcs->nstats)
>> +		return;
> This is a critical bug.  Please don't fail silently.  BUG_ON(),
> please.

Sure.

>
>> +	preempt_disable();
>> +	pstat = this_cpu_ptr(&pcs->stats[stat]);
>> +	*pstat += cnt;
>> +	preempt_enable();
>> +}
> this_cpu_add() is atomic w.r.t. local operations.

Will use this_cpu_add().

>> +static inline unsigned long
>> +percpu_stats_sum(struct percpu_stats *pcs, int stat)
>> +{
>> +	int cpu;
>> +	unsigned long sum = 0;
>> +
>> +	if ((unsigned int)stat>= pcs->nstats)
>> +		return sum;
> Ditto.
>
>> +	for_each_possible_cpu(cpu)
>> +		sum += per_cpu(pcs->stats[stat], cpu);
>> +	return sum;
>> +}
> Thanks.
>

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1372918

Hello,

On Wed, Apr 06, 2016 at 05:51:45PM -0400, Waiman Long wrote:
> >>+	/*
> >>+	 * If a statistics count is in the middle of being updated, it
> >>+	 * is possible that the above clearing may not work. So we need
> >>+	 * to double check again to make sure that the counters are really
> >>+	 * cleared. Still there is a still a very small chance that the
> >>+	 * second clearing does not work.
> >>+	 */
> >>+	for_each_possible_cpu(cpu) {
> >>+		unsigned long *pstats =  per_cpu_ptr(pcs->stats, cpu);
> >>+		int stat;
> >>+
> >>+		for (stat = 0; stat<  pcs->nstats; stat++, pstats++)
> >>+			if (*pstats)
> >>+				*pstats = 0;
> >>+	}
> >I don't think this is acceptable.
> 
> I am not sure what you mean here by not acceptable. Please enlighten me on
> that.

Hmmm... I thought that was pretty clear.  Try-twice-and-we-are-probably-okay
is simply not acceptable.  Please make it watertight.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1373502

On 04/06/2016 06:54 PM, Tejun Heo wrote:
> Hello,
>
> On Wed, Apr 06, 2016 at 05:51:45PM -0400, Waiman Long wrote:
>>>> +	/*
>>>> +	 * If a statistics count is in the middle of being updated, it
>>>> +	 * is possible that the above clearing may not work. So we need
>>>> +	 * to double check again to make sure that the counters are really
>>>> +	 * cleared. Still there is a still a very small chance that the
>>>> +	 * second clearing does not work.
>>>> +	 */
>>>> +	for_each_possible_cpu(cpu) {
>>>> +		unsigned long *pstats =  per_cpu_ptr(pcs->stats, cpu);
>>>> +		int stat;
>>>> +
>>>> +		for (stat = 0; stat<   pcs->nstats; stat++, pstats++)
>>>> +			if (*pstats)
>>>> +				*pstats = 0;
>>>> +	}
>>> I don't think this is acceptable.
>> I am not sure what you mean here by not acceptable. Please enlighten me on
>> that.
> Hmmm... I thought that was pretty clear.  Try-twice-and-we-are-probably-okay
> is simply not acceptable.  Please make it watertight.
>
> Thanks.

OK, I got it now.

We can certainly make it watertight. However, that will certainly 
require adding performance overhead in the percpu stats update fast path 
which I am not willing to pay.

The purpose of this stat counters reset functionality is to allow 
developers to reset the stat counters, run certain workload and see how 
things are going in the kernel when the workload completes assuming that 
those stat counters are exposed via sysfs, debugfs, etc. The developers 
can certainly check the stat counters after the reset to make sure that 
they are properly reset. So I don't think we need an airtight way of 
doing it. If you have scenarios in your mind that require airtight reset 
of the stat counters, please let me know and I will see what I can do 
about it.

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1373507

Hello, Waiman.

On Thu, Apr 07, 2016 at 11:58:13AM -0400, Waiman Long wrote:
> We can certainly make it watertight. However, that will certainly require
> adding performance overhead in the percpu stats update fast path which I am
> not willing to pay.

There are multiple options depending on the specific balance you want
to hit.  Reset can be made very heavy (involving RCU sync operation)
to make hot path overhead minimal, local locking or atomic ops can
also be used which while more expensive than this_cpu_*() ops still
avoids cacheline bouncing.

> The purpose of this stat counters reset functionality is to allow developers
> to reset the stat counters, run certain workload and see how things are
> going in the kernel when the workload completes assuming that those stat
> counters are exposed via sysfs, debugfs, etc. The developers can certainly
> check the stat counters after the reset to make sure that they are properly
> reset. So I don't think we need an airtight way of doing it. If you have
> scenarios in your mind that require airtight reset of the stat counters,
> please let me know and I will see what I can do about it.

No matter what, don't create something which can yield a completely
surprising result once in a blue moon.  You might think it's okay
because the likelihood is low but that just means that the resulting
malfunctions will be that much more obscure and difficult to
reproduce.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1373677

Hello, Waiman.

On Thu, Apr 07, 2016 at 02:52:33PM -0400, Waiman Long wrote:
> As long as atomic reset is an optional feature that caller can choose at
> init time, I am OK to provide this functionality. I just don't want it to be
> the default because of the performance overhead.

Please take a look at how percpu-ref coordinates global
synchronization.  The hot path overhead is one branch which is
extremely easy to predict and shouldn't show up anywhere.  If you're
gonna provide reset at all (which btw always kinda baffles me, what's
wrong with taking a snapshot value and taking delta from there?), you
need to make it actually work reliably.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1373731

On 04/07/2016 02:58 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Thu, Apr 07, 2016 at 02:52:33PM -0400, Waiman Long wrote:
>> As long as atomic reset is an optional feature that caller can choose at
>> init time, I am OK to provide this functionality. I just don't want it to be
>> the default because of the performance overhead.
> Please take a look at how percpu-ref coordinates global
> synchronization.  The hot path overhead is one branch which is
> extremely easy to predict and shouldn't show up anywhere.  If you're
> gonna provide reset at all (which btw always kinda baffles me, what's
> wrong with taking a snapshot value and taking delta from there?), you
> need to make it actually work reliably.
>
> Thanks.
>

I would say that because I am lazy, I don't want compute the deltas 
every time I want to see the effect of running a certain type of 
workload on the statistics counts. I have use case that I need to track 
10 or so statistics counts and monitor their changes after running a 
job. It is much more convenient to do a reset and see what you get than 
doing manual subtractions to find out.

I had taken a look at percpu-refcount.[ch]. I think the synchronization 
code is a bit overkill for this purpose as no one really need a very 
precise statistics counts nor precise atomic reset. I would prefer 
providing an optional atomic reset feature with slower statistics count 
update path for the time being. If we come across a use case where we 
need atomic reset with negligible slowdown, we could then refactor the 
code to use something similar to  what the percpu-refcount code is doing.

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1373743

Hello, Waiman.

On Thu, Apr 07, 2016 at 04:37:06PM -0400, Waiman Long wrote:
> I would say that because I am lazy, I don't want compute the deltas every
> time I want to see the effect of running a certain type of workload on the
> statistics counts. I have use case that I need to track 10 or so statistics
> counts and monitor their changes after running a job. It is much more
> convenient to do a reset and see what you get than doing manual subtractions
> to find out.

I don't know.  Write a simple script?  Even if you wanna keep it in
kernel, you can just have a base counter which offsets the summed up
value on read.

> I had taken a look at percpu-refcount.[ch]. I think the synchronization code
> is a bit overkill for this purpose as no one really need a very precise
> statistics counts nor precise atomic reset. I would prefer providing an
> optional atomic reset feature with slower statistics count update path for
> the time being. If we come across a use case where we need atomic reset with
> negligible slowdown, we could then refactor the code to use something
> similar to  what the percpu-refcount code is doing.

Please either drop reset or make it actually work; otherwise, I don't
think this should go in.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1373798

On 04/07/2016 04:41 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Thu, Apr 07, 2016 at 04:37:06PM -0400, Waiman Long wrote:
>> I would say that because I am lazy, I don't want compute the deltas every
>> time I want to see the effect of running a certain type of workload on the
>> statistics counts. I have use case that I need to track 10 or so statistics
>> counts and monitor their changes after running a job. It is much more
>> convenient to do a reset and see what you get than doing manual subtractions
>> to find out.
> I don't know.  Write a simple script?  Even if you wanna keep it in
> kernel, you can just have a base counter which offsets the summed up
> value on read.
>
>> I had taken a look at percpu-refcount.[ch]. I think the synchronization code
>> is a bit overkill for this purpose as no one really need a very precise
>> statistics counts nor precise atomic reset. I would prefer providing an
>> optional atomic reset feature with slower statistics count update path for
>> the time being. If we come across a use case where we need atomic reset with
>> negligible slowdown, we could then refactor the code to use something
>> similar to  what the percpu-refcount code is doing.
> Please either drop reset or make it actually work; otherwise, I don't
> think this should go in.
>
> Thanks.
>

In this case, I think I will drop this reset functionality. It is not 
really needed for this patchset.

Thanks for the feedback!

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1373685

On 04/07/2016 12:06 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Thu, Apr 07, 2016 at 11:58:13AM -0400, Waiman Long wrote:
>> We can certainly make it watertight. However, that will certainly require
>> adding performance overhead in the percpu stats update fast path which I am
>> not willing to pay.
> There are multiple options depending on the specific balance you want
> to hit.  Reset can be made very heavy (involving RCU sync operation)
> to make hot path overhead minimal, local locking or atomic ops can
> also be used which while more expensive than this_cpu_*() ops still
> avoids cacheline bouncing.
>
>> The purpose of this stat counters reset functionality is to allow developers
>> to reset the stat counters, run certain workload and see how things are
>> going in the kernel when the workload completes assuming that those stat
>> counters are exposed via sysfs, debugfs, etc. The developers can certainly
>> check the stat counters after the reset to make sure that they are properly
>> reset. So I don't think we need an airtight way of doing it. If you have
>> scenarios in your mind that require airtight reset of the stat counters,
>> please let me know and I will see what I can do about it.
> No matter what, don't create something which can yield a completely
> surprising result once in a blue moon.  You might think it's okay
> because the likelihood is low but that just means that the resulting
> malfunctions will be that much more obscure and difficult to
> reproduce.
>
> Thanks.
>

As long as atomic reset is an optional feature that caller can choose at 
init time, I am OK to provide this functionality. I just don't want it 
to be the default because of the performance overhead.

Cheers,
Longman

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web