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

Groups > linux.kernel > #1240022 > unrolled thread

Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare()

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] percpu_counter: return precise count from __percpu_counter_compare() Waiman Long <waiman.long@hpe.com> - 2015-10-06 01:10 +0200
    Re: [PATCH] percpu_counter: return precise count from  __percpu_counter_compare() Dave Chinner <david@fromorbit.com> - 2015-10-06 02:30 +0200
      Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare() Waiman Long <waiman.long@hpe.com> - 2015-10-06 19:40 +0200
        Re: [PATCH] percpu_counter: return precise count from  __percpu_counter_compare() Dave Chinner <david@fromorbit.com> - 2015-10-06 23:40 +0200
          Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare() Waiman Long <waiman.long@hpe.com> - 2015-10-07 22:10 +0200
            Re: [PATCH] percpu_counter: return precise count from  __percpu_counter_compare() Dave Chinner <david@fromorbit.com> - 2015-10-08 01:10 +0200
              Re: [PATCH] percpu_counter: return precise count from  __percpu_counter_compare() Tejun Heo <tj@kernel.org> - 2015-10-08 01:30 +0200
                Re: [PATCH] percpu_counter: return precise count from  __percpu_counter_compare() Dave Chinner <david@fromorbit.com> - 2015-10-08 03:10 +0200
                  Re: [PATCH] percpu_counter: return precise count from  __percpu_counter_compare() Tejun Heo <tj@kernel.org> - 2015-10-08 03:10 +0200
                  Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare() Waiman Long <waiman.long@hpe.com> - 2015-10-08 18:10 +0200
              Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare() Waiman Long <waiman.long@hpe.com> - 2015-10-08 18:10 +0200

#1240022 — Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare()

On 10/02/2015 06:16 PM, Dave Chinner wrote:
> On Fri, Oct 02, 2015 at 01:29:57PM -0400, Waiman Long wrote:
>> In __percpu_counter_compare(), if the current imprecise count is
>> within (batch*nr_cpus) of the input value to be compared, a call
>> to percpu_counter_sum() will be made to get the precise count. The
>> percpu_counter_sum() call, however, can be expensive especially on
>> large systems where there are a lot of CPUs. Large systems also make
>> it more likely that percpu_counter_sum() will be called.
>>
>> The xfs_mod_fdblocks() function calls __percpu_counter_compare()
>> twice. First to see if a smaller batch size should be used for
>> __percpu_counter_add() and the second call to compare the actual
>> size needed. This can potentially lead to 2 calls to the expensive
>> percpu_counter_sum() function.
> There should not be that much overhead in __percpu_counter_compare()
> through this path in normal operation. The slow path is only taken
> as you near ENOSPC...

Yes, it is for optimizing the case there the filesystem is near ENOSP.

>> This patch added an extra argument to __percpu_counter_compare()
>> to return the precise count, if computed. The caller will need to
>> initialize it to an invalid value that it can tell if the precise
>> count is being returned.
> This doesn't work. ENOSPC detection is a lockless algorithm that
> requires absolute precision. Assuming the XFS_ALLOC_SET_ASIDE()
> definition of ENOSPC is 0 blocks free, your change allows this race:
>
> free space: 1 block
>
> thread 1		thread 2			free space
> allocate 1 block	allocate 1 block		  1
> sample pcount = 1					  1
> 			sample pcount = 1		  1
> add fdblocks, -1, 1)					  0
> 			add fdblocks, -1, 1)		  -1
> if (pcount - 1>= 0)	if (pcount - 1>= 0)
>     OK!			    OK!				  -1
>
> So, we've just failed to detect ENOSPC correct. One of those two
> threads should have returned ENOSPC and failed the allocation,
> but instead we've just allowed XFS to allocate a block that doesn't
> exist. Hence we have to resample the percpu counter after the
> modification to ensure that we don't miss this race condition.
>
> Sure, the curent code could race on the second comparisions and
> return ENOSPC to both threads, but that is a perfectly OK thing
> to do. It is vitally important that we don't oversubscribe
> filesystem space, because that will lead to all sorts of other
> problems (deadlocks, hangs, shutdowns, etc) that are very difficult
> to identify the cause of.
>
> FWIW, I'm guessing that you didn't run this patch through xfstests?
> xfstests will find these ENOSPC accounting bugs, and usually quite
> quickly...

Thanks for the review. I did miss the case that there was a race 
condition here. I also haven't run xfstests with this patch. I will do 
so next time.

>> Running the AIM7 disk workload with XFS filesystem, the jobs/min
>> on a 40-core 80-thread 4-socket Haswell-EX system increases from
>> 3805k to 4276k (12% increase) with this patch applied. As measured
>> by the perf tool, the %CPU cycle consumed by __percpu_counter_sum()
>> decreases from 12.64% to 7.08%.
> XFS should only hit the slow __percpu_counter_sum() path patch as
> the fs gets close to ENOSPC, which for your system will be less
> than:
>
> threshold = num_online_cpus * XFS_FDBLOCKS_BATCH * 2 blocks
> 	  = 80 * 1024 * 2 blocks
> 	  = 160,000 blocks
> 	  = 640MB of disk space.
>
> Having less than 1GB of free space in an XFS filesystem is
> considered to be "almost ENOSPC" - when you have TB to PB of space,
> less than 1GB really "moments before ENOSPC".

We have systems with more than 500 CPUs (HT on). I think SGI has systems 
with thousands of CPUs. For those large system, the slowpath will be 
triggered if there is less than 4G or 10G for those thousand CPUs 
systems. What I am trying to do with my patch is to reduce the 
performance overhead in those cases. I have no worry for systems that 
have only a few CPUs. In essence, the per-cpu counter code doesn't scale 
well for systems with large number of CPUs.

> XFS trades off low overhead for fast path allocation  with slowdowns
> as we near ENOSPC in allocation routines. It gets harder to find
> contiguous free space, files get more fragmented, IO takes longer
> because we seek more, etc. Hence we accept that performance slows
> down as as the need for precision increases as we near ENOSPC.
>
> I'd suggest you retry your benchmark with larger filesystems, and
> see what happens...

I don't think I am going to see the slowdown that I observed on larger 
filesystems with more free space. However, I still think that doing 2 
precise count computations is wasteful. I am planning to rework my patch 
to disable precise count for the first comparison in xfs_mod_fdblocks as 
that comparison is used to gauge how far it is from ENOSPC. So we don't 
really need to get the precise count as long as number of CPUs are taken 
into consideration in the comparison. This change should enable the new 
patch to have similar performance overhead reduction effect as the old 
one without the racing condition you mentioned above.

Cheers,
Longman

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1240043 — Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare()

On Mon, Oct 05, 2015 at 07:02:21PM -0400, Waiman Long wrote:
> On 10/02/2015 06:16 PM, Dave Chinner wrote:
> >On Fri, Oct 02, 2015 at 01:29:57PM -0400, Waiman Long wrote:
> >>In __percpu_counter_compare(), if the current imprecise count is
> >>within (batch*nr_cpus) of the input value to be compared, a call
> >>to percpu_counter_sum() will be made to get the precise count. The
> >>percpu_counter_sum() call, however, can be expensive especially on
> >>large systems where there are a lot of CPUs. Large systems also make
> >>it more likely that percpu_counter_sum() will be called.
> >>
> >>The xfs_mod_fdblocks() function calls __percpu_counter_compare()
> >>twice. First to see if a smaller batch size should be used for
> >>__percpu_counter_add() and the second call to compare the actual
> >>size needed. This can potentially lead to 2 calls to the expensive
> >>percpu_counter_sum() function.
> >There should not be that much overhead in __percpu_counter_compare()
> >through this path in normal operation. The slow path is only taken
> >as you near ENOSPC...
> 
> Yes, it is for optimizing the case there the filesystem is near ENOSP.
....
> >Having less than 1GB of free space in an XFS filesystem is
> >considered to be "almost ENOSPC" - when you have TB to PB of space,
> >less than 1GB really "moments before ENOSPC".
> 
> We have systems with more than 500 CPUs (HT on). I think SGI has
> systems with thousands of CPUs. For those large system, the slowpath
> will be triggered if there is less than 4G or 10G for those thousand
> CPUs systems.

yes, I'm well aware of this. But systems with hundreds to thousands
of CPUs simply do not operate their storage at this capacity.
They'll have hundreds of TB or PBs of storage attached, so if we
trigger the slow path at 10GB of free space, we are talking about
having already used > 99.9% of that capacity.

In which case, they are already in a world of pain because
filesystem allocation performance starts to degrade at >90%
capacity, and we start cutting back preallocations at >95% capacity,
and we really start to throttle ispace allocations to their
minimum possible sizes at >99% capacity. IOWs, hitting this slow
path at >99.9% capacity is really irrelevant....


> What I am trying to do with my patch is to reduce the
> performance overhead in those cases. I have no worry for systems
> that have only a few CPUs. In essence, the per-cpu counter code
> doesn't scale well for systems with large number of CPUs.

Maybe so, but we don't tend ot optimise slow paths - we trade off a
really fast fast path for a slow, more complex slow path all over
the place. Not just in XFS, but all over the kernel.

> >XFS trades off low overhead for fast path allocation  with slowdowns
> >as we near ENOSPC in allocation routines. It gets harder to find
> >contiguous free space, files get more fragmented, IO takes longer
> >because we seek more, etc. Hence we accept that performance slows
> >down as as the need for precision increases as we near ENOSPC.
> >
> >I'd suggest you retry your benchmark with larger filesystems, and
> >see what happens...
> 
> I don't think I am going to see the slowdown that I observed on
> larger filesystems with more free space.

So there is no problem that needs fixing.... ;)

> However, I still think that
> doing 2 precise count computations is wasteful.

I really don't care about the CPU overhead, because it's far more
important that:

	1) the zero threshold detection is precise and correct;
	2) the fast path is really fast; and
	3) I understand the code well enough to be able to debug
	   and maintain it.

> I am planning to rework my patch to disable precise count for the
> first comparison in xfs_mod_fdblocks as that comparison is used to
> gauge how far it is from ENOSPC.  So we don't really need to get
> the precise count as long as number of CPUs are taken into
> consideration in the comparison.

I think you are looking in the wrong place. There is nothing
wrong with XFS doing two compares here. If we are hitting the
__percpu_counter_compare() slow path too much, then we should be
understanding exactly why that slow path is being hit so hard so
often. I don't see any analysis of the actual per-cpu counter
behaviour and why the slow path is being taken so often....

Indeed, have you considered using something like this in the precise
path of __percpu_counter_compare() rather than percpu_counter_sum():

/*
 * Aggregate the per-cpu counter magazines back into the global
 * counter. This avoids the need for repeated compare operations to
 * run the slow path when the majority of the counter value is held
 * in the per-cpu magazines. Folding them back into the global
 * counter means we will continue to hit the fast
 * percpu_counter_read() path until the counter value falls
 * completely within the comparison limit passed to
 * __percpu_counter_compare().
 */
static s64 percpu_counter_aggregate(struct percpu_counter *fbc)
{
	s64 ret;
	int cpu;
	unsigned long flags;

	raw_spin_lock_irqsave(&fbc->lock, flags);
	ret = fbc->count;
	for_each_online_cpu(cpu) {
		s32 count = __this_cpu_read(*fbc->counters);
                ret += count;
		__this_cpu_sub(*fbc->counters, count)
	}
	fbc->count = ret;
	raw_spin_unlock_irqrestore(&fbc->lock, flags);
	return ret;
}


Some perspective: you wouldn't have seen this behaviour with the
previous per-cpu counter code in XFS near ENOSPC. By the time it got
this close to ENOSPC it was completely serialising all access to the
free space counters with a mutex and then doing per-cpu sums under
that mutex (see commit 20b6428 ("[XFS] Reduction global superblock
lock contention near ENOSPC.").  Hence it wouldn't have appeared in
your profiles, even though it was much worse in terms of contention
and lock hold times than the current code is.

This looks to be the same fundamental problem - the per-cpu counter
values are not being managed in a way that reduces minimises precise
comparison overhead. Making the above change will tell us whether
this is the case or not...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1240791

On 10/05/2015 08:25 PM, Dave Chinner wrote:
> On Mon, Oct 05, 2015 at 07:02:21PM -0400, Waiman Long wrote:
> ....
>>> Having less than 1GB of free space in an XFS filesystem is
>>> considered to be "almost ENOSPC" - when you have TB to PB of space,
>>> less than 1GB really "moments before ENOSPC".
>> We have systems with more than 500 CPUs (HT on). I think SGI has
>> systems with thousands of CPUs. For those large system, the slowpath
>> will be triggered if there is less than 4G or 10G for those thousand
>> CPUs systems.
> yes, I'm well aware of this. But systems with hundreds to thousands
> of CPUs simply do not operate their storage at this capacity.
> They'll have hundreds of TB or PBs of storage attached, so if we
> trigger the slow path at 10GB of free space, we are talking about
> having already used>  99.9% of that capacity.
>
> In which case, they are already in a world of pain because
> filesystem allocation performance starts to degrade at>90%
> capacity, and we start cutting back preallocations at>95% capacity,
> and we really start to throttle ispace allocations to their
> minimum possible sizes at>99% capacity. IOWs, hitting this slow
> path at>99.9% capacity is really irrelevant....
>

In the production environment, we do expect to see large storage 
attached to those big machines. However, in the testing environment, we 
may have such large pool of disks to be used. Alternatively, a user may 
create a small filesystem partition for certain specific use. Under 
those circumstances, the slowpath may be triggered.

>> What I am trying to do with my patch is to reduce the
>> performance overhead in those cases. I have no worry for systems
>> that have only a few CPUs. In essence, the per-cpu counter code
>> doesn't scale well for systems with large number of CPUs.
> Maybe so, but we don't tend ot optimise slow paths - we trade off a
> really fast fast path for a slow, more complex slow path all over
> the place. Not just in XFS, but all over the kernel.

I am not proposing any change to the fast path and they should have the 
same performance as before.

>>> XFS trades off low overhead for fast path allocation  with slowdowns
>>> as we near ENOSPC in allocation routines. It gets harder to find
>>> contiguous free space, files get more fragmented, IO takes longer
>>> because we seek more, etc. Hence we accept that performance slows
>>> down as as the need for precision increases as we near ENOSPC.
>>>
>>> I'd suggest you retry your benchmark with larger filesystems, and
>>> see what happens...
>> I don't think I am going to see the slowdown that I observed on
>> larger filesystems with more free space.
> So there is no problem that needs fixing.... ;)
>

Well, I am still worrying that corner cases when the slowpath is 
triggered. I would like to make it perform better in those cases.


>> However, I still think that
>> doing 2 precise count computations is wasteful.
> I really don't care about the CPU overhead, because it's far more
> important that:
>
> 	1) the zero threshold detection is precise and correct;
> 	2) the fast path is really fast; and
> 	3) I understand the code well enough to be able to debug
> 	   and maintain it.

I completely agree with that:-)

>> I am planning to rework my patch to disable precise count for the
>> first comparison in xfs_mod_fdblocks as that comparison is used to
>> gauge how far it is from ENOSPC.  So we don't really need to get
>> the precise count as long as number of CPUs are taken into
>> consideration in the comparison.
> I think you are looking in the wrong place. There is nothing
> wrong with XFS doing two compares here. If we are hitting the
> __percpu_counter_compare() slow path too much, then we should be
> understanding exactly why that slow path is being hit so hard so
> often. I don't see any analysis of the actual per-cpu counter
> behaviour and why the slow path is being taken so often....

I am thinking of making the following changes:

  fs/xfs/xfs_mount.c |   11 ++++++-----
  1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index bf92e0c..bb2e0ef 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1183,12 +1183,13 @@ xfs_mod_fdblocks(
       * Taking blocks away, need to be more accurate the closer we
       * are to zero.
       *
-     * If the counter has a value of less than 2 * max batch size,
-     * then make everything serialise as we are real close to
-     * ENOSPC.
+     * The maximum error of imprecise counter is (nr_cpus * batch size).
+     * If the imprecise counter has a value less than (nr_cpus + 2) *
+     * max batch size, then make everything serialise as we may be real
+     * close to ENOSPC.
       */
-    if (__percpu_counter_compare(&mp->m_fdblocks, 2 * XFS_FDBLOCKS_BATCH,
-                     XFS_FDBLOCKS_BATCH) < 0)
+    if (percpu_counter_read(&mp->m_fdblocks) <
+       (num_online_cpus() + 2) * XFS_FDBLOCKS_BATCH)
          batch = 1;
      else
          batch = XFS_FDBLOCKS_BATCH;
-- 

Please let me know if you think that is acceptable to you.

> Indeed, have you considered using something like this in the precise
> path of __percpu_counter_compare() rather than percpu_counter_sum():
>
> /*
>   * Aggregate the per-cpu counter magazines back into the global
>   * counter. This avoids the need for repeated compare operations to
>   * run the slow path when the majority of the counter value is held
>   * in the per-cpu magazines. Folding them back into the global
>   * counter means we will continue to hit the fast
>   * percpu_counter_read() path until the counter value falls
>   * completely within the comparison limit passed to
>   * __percpu_counter_compare().
>   */
> static s64 percpu_counter_aggregate(struct percpu_counter *fbc)
> {
> 	s64 ret;
> 	int cpu;
> 	unsigned long flags;
>
> 	raw_spin_lock_irqsave(&fbc->lock, flags);
> 	ret = fbc->count;
> 	for_each_online_cpu(cpu) {
> 		s32 count = __this_cpu_read(*fbc->counters);
>                  ret += count;
> 		__this_cpu_sub(*fbc->counters, count)
> 	}
> 	fbc->count = ret;
> 	raw_spin_unlock_irqrestore(&fbc->lock, flags);
> 	return ret;
> }

I don't think that will work as some other CPUs may change the percpu 
counters values between percpu_counter_aggregate() and 
__percpu_counter_compare(). To be safe, the precise counter has to be 
compted whenever the comparison value difference is less than nr_cpus * 
batch size.

> Some perspective: you wouldn't have seen this behaviour with the
> previous per-cpu counter code in XFS near ENOSPC. By the time it got
> this close to ENOSPC it was completely serialising all access to the
> free space counters with a mutex and then doing per-cpu sums under
> that mutex (see commit 20b6428 ("[XFS] Reduction global superblock
> lock contention near ENOSPC.").  Hence it wouldn't have appeared in
> your profiles, even though it was much worse in terms of contention
> and lock hold times than the current code is.
>
> This looks to be the same fundamental problem - the per-cpu counter
> values are not being managed in a way that reduces minimises precise
> comparison overhead. Making the above change will tell us whether
> this is the case or not...

I have some thoughts on how to reduce precise comparison overhead, but I 
need more time to work out the details.

Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1241012 — Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare()

On Tue, Oct 06, 2015 at 01:33:21PM -0400, Waiman Long wrote:
> On 10/05/2015 08:25 PM, Dave Chinner wrote:
> >On Mon, Oct 05, 2015 at 07:02:21PM -0400, Waiman Long wrote:
> >....
> >>>Having less than 1GB of free space in an XFS filesystem is
> >>>considered to be "almost ENOSPC" - when you have TB to PB of space,
> >>>less than 1GB really "moments before ENOSPC".
> >>We have systems with more than 500 CPUs (HT on). I think SGI has
> >>systems with thousands of CPUs. For those large system, the slowpath
> >>will be triggered if there is less than 4G or 10G for those thousand
> >>CPUs systems.
> >yes, I'm well aware of this. But systems with hundreds to thousands
> >of CPUs simply do not operate their storage at this capacity.
> >They'll have hundreds of TB or PBs of storage attached, so if we
> >trigger the slow path at 10GB of free space, we are talking about
> >having already used>  99.9% of that capacity.
> >
> >In which case, they are already in a world of pain because
> >filesystem allocation performance starts to degrade at>90%
> >capacity, and we start cutting back preallocations at>95% capacity,
> >and we really start to throttle ispace allocations to their
> >minimum possible sizes at>99% capacity. IOWs, hitting this slow
> >path at>99.9% capacity is really irrelevant....
> >
> 
> In the production environment, we do expect to see large storage
> attached to those big machines. However, in the testing environment,
> we may have such large pool of disks to be used. Alternatively, a
> user may create a small filesystem partition for certain specific
> use. Under those circumstances, the slowpath may be triggered.

Yes, it may be, but that does not mean we should optimise for it.
If you are doing filesystem scalability testing on small filesystems
near capacity, then your testing methodology is needs fixing. Not
the code.

> >>>XFS trades off low overhead for fast path allocation  with slowdowns
> >>>as we near ENOSPC in allocation routines. It gets harder to find
> >>>contiguous free space, files get more fragmented, IO takes longer
> >>>because we seek more, etc. Hence we accept that performance slows
> >>>down as as the need for precision increases as we near ENOSPC.
> >>>
> >>>I'd suggest you retry your benchmark with larger filesystems, and
> >>>see what happens...
> >>I don't think I am going to see the slowdown that I observed on
> >>larger filesystems with more free space.
> >So there is no problem that needs fixing.... ;)
> 
> Well, I am still worrying that corner cases when the slowpath is
> triggered. I would like to make it perform better in those cases.

It's a pretty damn small slowdown in your somewhat extreme,
artificial test. Show me a real world production system that runs
small fileystems permanently at >99% filesystem capacity, and them
maybe vwe've got something that needs changing.

> >>gauge how far it is from ENOSPC.  So we don't really need to get
> >>the precise count as long as number of CPUs are taken into
> >>consideration in the comparison.
> >I think you are looking in the wrong place. There is nothing
> >wrong with XFS doing two compares here. If we are hitting the
> >__percpu_counter_compare() slow path too much, then we should be
> >understanding exactly why that slow path is being hit so hard so
> >often. I don't see any analysis of the actual per-cpu counter
> >behaviour and why the slow path is being taken so often....
> 
> I am thinking of making the following changes:

No. Please test the change to the per-cpu counters that I suggested:

> >/*
> >  * Aggregate the per-cpu counter magazines back into the global
> >  * counter. This avoids the need for repeated compare operations to
> >  * run the slow path when the majority of the counter value is held
> >  * in the per-cpu magazines. Folding them back into the global
> >  * counter means we will continue to hit the fast
> >  * percpu_counter_read() path until the counter value falls
> >  * completely within the comparison limit passed to
> >  * __percpu_counter_compare().
> >  */
> >static s64 percpu_counter_aggregate(struct percpu_counter *fbc)
> >{
> >	s64 ret;
> >	int cpu;
> >	unsigned long flags;
> >
> >	raw_spin_lock_irqsave(&fbc->lock, flags);
> >	ret = fbc->count;
> >	for_each_online_cpu(cpu) {
> >		s32 count = __this_cpu_read(*fbc->counters);
> >                 ret += count;
> >		__this_cpu_sub(*fbc->counters, count)
> >	}
> >	fbc->count = ret;
> >	raw_spin_unlock_irqrestore(&fbc->lock, flags);
> >	return ret;
> >}
> 
> I don't think that will work as some other CPUs may change the
> percpu counters values between percpu_counter_aggregate() and
> __percpu_counter_compare().  To be safe, the precise counter has to
> be compted whenever the comparison value difference is less than
> nr_cpus * batch size.

Well, yes. Why do you think the above function does the same
function as percpu_counter_sum()? So that the percpu_counter_sum()
call *inside* __percpu_counter_compare() can be replaced by this
call. i.e.

			return -1;
	}
	/* Need to use precise count */
-	count = percpu_counter_sum(fbc);
+	count = percpu_counter_aggregate(fbc);
	if (count > rhs)
		return 1;
	else if (count < rhs)

Please think about what I'm saying rather than dismissing it without
first understanding my suggestions.

> I have some thoughts on how to reduce precise comparison overhead,
> but I need more time to work out the details.

We don't need something "smart" that only 2 people understand
properly that turns the per-cpu counters into a regression-prone,
unmaintainable mess like all the locking code has become.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1241774

On 10/06/2015 05:30 PM, Dave Chinner wrote:
> On Tue, Oct 06, 2015 at 01:33:21PM -0400, Waiman Long wrote:
> Yes, it may be, but that does not mean we should optimise for it.
> If you are doing filesystem scalability testing on small filesystems
> near capacity, then your testing methodology is needs fixing. Not
> the code.
>
>>>>> XFS trades off low overhead for fast path allocation  with slowdowns
>>>>> as we near ENOSPC in allocation routines. It gets harder to find
>>>>> contiguous free space, files get more fragmented, IO takes longer
>>>>> because we seek more, etc. Hence we accept that performance slows
>>>>> down as as the need for precision increases as we near ENOSPC.
>>>>>
>>>>> I'd suggest you retry your benchmark with larger filesystems, and
>>>>> see what happens...
>>>> I don't think I am going to see the slowdown that I observed on
>>>> larger filesystems with more free space.
>>> So there is no problem that needs fixing.... ;)
>> Well, I am still worrying that corner cases when the slowpath is
>> triggered. I would like to make it perform better in those cases.
> It's a pretty damn small slowdown in your somewhat extreme,
> artificial test. Show me a real world production system that runs
> small fileystems permanently at>99% filesystem capacity, and them
> maybe vwe've got something that needs changing.
>
>>>> gauge how far it is from ENOSPC.  So we don't really need to get
>>>> the precise count as long as number of CPUs are taken into
>>>> consideration in the comparison.
>>> I think you are looking in the wrong place. There is nothing
>>> wrong with XFS doing two compares here. If we are hitting the
>>> __percpu_counter_compare() slow path too much, then we should be
>>> understanding exactly why that slow path is being hit so hard so
>>> often. I don't see any analysis of the actual per-cpu counter
>>> behaviour and why the slow path is being taken so often....
>> I am thinking of making the following changes:
> No. Please test the change to the per-cpu counters that I suggested:
>
>>> /*
>>>   * Aggregate the per-cpu counter magazines back into the global
>>>   * counter. This avoids the need for repeated compare operations to
>>>   * run the slow path when the majority of the counter value is held
>>>   * in the per-cpu magazines. Folding them back into the global
>>>   * counter means we will continue to hit the fast
>>>   * percpu_counter_read() path until the counter value falls
>>>   * completely within the comparison limit passed to
>>>   * __percpu_counter_compare().
>>>   */
>>> static s64 percpu_counter_aggregate(struct percpu_counter *fbc)
>>> {
>>> 	s64 ret;
>>> 	int cpu;
>>> 	unsigned long flags;
>>>
>>> 	raw_spin_lock_irqsave(&fbc->lock, flags);
>>> 	ret = fbc->count;
>>> 	for_each_online_cpu(cpu) {
>>> 		s32 count = __this_cpu_read(*fbc->counters);
>>>                  ret += count;
>>> 		__this_cpu_sub(*fbc->counters, count)
>>> 	}
>>> 	fbc->count = ret;
>>> 	raw_spin_unlock_irqrestore(&fbc->lock, flags);
>>> 	return ret;
>>> }
>> I don't think that will work as some other CPUs may change the
>> percpu counters values between percpu_counter_aggregate() and
>> __percpu_counter_compare().  To be safe, the precise counter has to
>> be compted whenever the comparison value difference is less than
>> nr_cpus * batch size.
> Well, yes. Why do you think the above function does the same
> function as percpu_counter_sum()? So that the percpu_counter_sum()
> call *inside* __percpu_counter_compare() can be replaced by this
> call. i.e.
>
> 			return -1;
> 	}
> 	/* Need to use precise count */
> -	count = percpu_counter_sum(fbc);
> +	count = percpu_counter_aggregate(fbc);
> 	if (count>  rhs)
> 		return 1;
> 	else if (count<  rhs)
>
> Please think about what I'm saying rather than dismissing it without
> first understanding my suggestions.

I understood what you were saying. However, the per-cpu counter isn't 
protected by the spinlock. Reading it is OK, but writing may cause race 
if that counter is modified by a CPU other than its owning CPU.

The slow performance of percpu_counter_sum() is due to its need to 
access n different (likely cold) cachelines where n is the number of 
CPUs in the system. So the larger the system, the more problematic it 
will be. My main concern about xfs_mod_fdblocks() is that it can 
potentially call percpu_counter_sum() twice which is what I want to 
prevent. It is OK if you don't think that change is necessary. However, 
I will come back if I find more evidence that this can be an issue.

Cheers,
Longman



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1241846 — Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare()

On Wed, Oct 07, 2015 at 04:00:42PM -0400, Waiman Long wrote:
> On 10/06/2015 05:30 PM, Dave Chinner wrote:
> >>>/*
> >>>  * Aggregate the per-cpu counter magazines back into the global
> >>>  * counter. This avoids the need for repeated compare operations to
> >>>  * run the slow path when the majority of the counter value is held
> >>>  * in the per-cpu magazines. Folding them back into the global
> >>>  * counter means we will continue to hit the fast
> >>>  * percpu_counter_read() path until the counter value falls
> >>>  * completely within the comparison limit passed to
> >>>  * __percpu_counter_compare().
> >>>  */
> >>>static s64 percpu_counter_aggregate(struct percpu_counter *fbc)
> >>>{
> >>>	s64 ret;
> >>>	int cpu;
> >>>	unsigned long flags;
> >>>
> >>>	raw_spin_lock_irqsave(&fbc->lock, flags);
> >>>	ret = fbc->count;
> >>>	for_each_online_cpu(cpu) {
> >>>		s32 count = __this_cpu_read(*fbc->counters);
> >>>                 ret += count;
> >>>		__this_cpu_sub(*fbc->counters, count)
> >>>	}
> >>>	fbc->count = ret;
> >>>	raw_spin_unlock_irqrestore(&fbc->lock, flags);
> >>>	return ret;
> >>>}
> >>I don't think that will work as some other CPUs may change the
> >>percpu counters values between percpu_counter_aggregate() and
> >>__percpu_counter_compare().  To be safe, the precise counter has to
> >>be compted whenever the comparison value difference is less than
> >>nr_cpus * batch size.
> >Well, yes. Why do you think the above function does the same
> >function as percpu_counter_sum()? So that the percpu_counter_sum()
> >call *inside* __percpu_counter_compare() can be replaced by this
> >call. i.e.
> >
> >			return -1;
> >	}
> >	/* Need to use precise count */
> >-	count = percpu_counter_sum(fbc);
> >+	count = percpu_counter_aggregate(fbc);
> >	if (count>  rhs)
> >		return 1;
> >	else if (count<  rhs)
> >
> >Please think about what I'm saying rather than dismissing it without
> >first understanding my suggestions.
> 
> I understood what you were saying. However, the per-cpu counter
> isn't protected by the spinlock. Reading it is OK, but writing may
> cause race if that counter is modified by a CPU other than its
> owning CPU.

<sigh>

You're still trying to pick apart the code without considering what
we need to acheive.  We don't need to the code to be bullet proof to
test whether this hypothesis is correct or not - we just need
something that is "near-enough" to give us the data point to tell us
where we should focus our efforts. If optimising the counter like
above does not reduce the overhead, then we may have to change XFS.
If it does reduce the overhead, then the XFS code remains unchanged
and we focus on optimising the counter code.

But we *need to test the hypothesis first*.

As it is, the update race you pointed out is easy to solve with
__this_cpu_cmpxchg rather than _this_cpu_sub (similar to mod_state()
in the MM percpu counter stats code, perhaps).

So please test the above change and stop quibbling over details
that just don't matter until we know which way we need to go.

> The slow performance of percpu_counter_sum() is due to its need to
> access n different (likely cold) cachelines where n is the number of
> CPUs in the system. So the larger the system, the more problematic
> it will be.

Welcome to today's lecture titled "Per-cpu Counters 101". :/

Have a think about who you are lecturing(*).  Please don't treat me
as you would university intern who's just learning about
multithreaded programming, because a) it's disrepectful, and b)
you'll jump to the wrong conclusions because you may not immediately
understand what I'm saying or why I'm asking you to do something.

I always start by assuming participants understand the topic being
discussed.  I can quickly tell if a person has deep knowledge of the
topic from their responses and the above "explain the basics"
response is a key indicator.  If you assume that the other person
understands the topic as well or better than you do then you won't
make this mistake and, better yet, we might all learn something in
the ensuing discussion.

Cheers,

Dave.

(*) XFS had custom per-cpu counters because there was no generic
infrastructure in linux for this ten years ago:

commit 8d280b98cfe3c0b69c37d355218975c1c0279bb0
Author: David Chinner <dgc@sgi.com>
Date:   Tue Mar 14 13:13:09 2006 +1100

    [XFS] On machines with more than 8 cpus, when running parallel I/O
    threads, the incore superblock lock becomes the limiting factor for
    buffered write throughput. Make the contended fields in the incore
    superblock use per-cpu counters so that there is no global lock to limit
    scalability.
    
    SGI-PV: 946630
    SGI-Modid: xfs-linux-melb:xfs-kern:25106a
    
    Signed-off-by: David Chinner <dgc@sgi.com>
    Signed-off-by: Nathan Scott <nathans@sgi.com>

-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1241854 — Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare()

Hello, Dave.

On Thu, Oct 08, 2015 at 10:04:42AM +1100, Dave Chinner wrote:
...
> As it is, the update race you pointed out is easy to solve with
> __this_cpu_cmpxchg rather than _this_cpu_sub (similar to mod_state()
> in the MM percpu counter stats code, perhaps).

percpu cmpxchg is no different from sub or any other operations
regarding cross-CPU synchronization.  They're safe iff the operations
are on the local CPU.  They have to be made atomics if they need to be
manipulated from remote CPUs.

That said, while we can't manipulate the percpu counters directly, we
can add a separate global counter to cache sum result from the
previous run which gets automatically invalidated when any percpu
counter overflows.  That should give better and in case of
back-to-back invocations pretty good precision compared to just
returning the global overflow counter.  Interface-wise, that'd be a
lot easier to deal with although I have no idea whether it'd fit this
particular use case or whether this use case even exists.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1241917 — Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare()

On Wed, Oct 07, 2015 at 04:20:10PM -0700, Tejun Heo wrote:
> Hello, Dave.
> 
> On Thu, Oct 08, 2015 at 10:04:42AM +1100, Dave Chinner wrote:
> ...
> > As it is, the update race you pointed out is easy to solve with
> > __this_cpu_cmpxchg rather than _this_cpu_sub (similar to mod_state()
> > in the MM percpu counter stats code, perhaps).
> 
> percpu cmpxchg is no different from sub or any other operations
> regarding cross-CPU synchronization.  They're safe iff the operations
> are on the local CPU.  They have to be made atomics if they need to be
> manipulated from remote CPUs.

Again, another trivially solvable problem, but still irrelevant
because we don't have the data that tells us whether changing the
counter behaviour solves the problem....

> That said, while we can't manipulate the percpu counters directly, we
> can add a separate global counter to cache sum result from the
> previous run which gets automatically invalidated when any percpu
> counter overflows.
>
> That should give better and in case of
> back-to-back invocations pretty good precision compared to just
> returning the global overflow counter.  Interface-wise, that'd be a
> lot easier to deal with although I have no idea whether it'd fit this
> particular use case or whether this use case even exists.

No, it doesn't help - it's effectively what Waiman's original patch
did by returning the count from the initial comparison and using
that for ENOSPC detection instead of doing a second comparison...

FWIW, XFS has done an expensive per-cpu counter sum in this ENOSPC
situation since 2006, but in 2007 ENOSPC was wrapped in a mutex to
prevent spinlock contention on the aggregated global counter:

commit 20b642858b6bb413976ff13ae6a35cc596967bab
Author: David Chinner <dgc@sgi.com>
Date:   Sat Feb 10 18:35:09 2007 +1100

    [XFS] Reduction global superblock lock contention near ENOSPC.
    
    The existing per-cpu superblock counter code uses the global superblock
    spin lock when we approach ENOSPC for global synchronisation. On larger
    machines than this code was originally tested on this can still get
    catastrophic spinlock contention due increasing rebalance frequency near
    ENOSPC.
    
    By introducing a sleeping lock that is used to serialise balances and
    modifications near ENOSPC we prevent contention from needlessly from
    wasting the CPU time of potentially hundreds of CPUs.
    
    To reduce the number of balances occuring, we separate the need rebalance
    case from the slow allocate case. Now, a counter running dry will trigger
    a rebalance during which counters are disabled. Any thread that sees a
    disabled counter enters a different path where it waits on the new mutex.
    When it gets the new mutex, it checks if the counter is disabled. If the
    counter is disabled, then we _know_ that we have to use the global counter
    and lock and it is safe to do so immediately. Otherwise, we drop the mutex
    and go back to trying the per-cpu counters which we know were re-enabled.
    
    SGI-PV: 952227
    SGI-Modid: xfs-linux-melb:xfs-kern:27612a
    
    Signed-off-by: David Chinner <dgc@sgi.com>
    Signed-off-by: Lachlan McIlroy <lachlan@sgi.com>
    Signed-off-by: Tim Shimmin <tes@sgi.com>

This is effectively the same symptoms that what we are seeing with
the new "lockless" generic percpu counteri algorithm, which is why
I'm trying to find out if it an issue with the counter
implementation before I do anything else...

FWIW, the first comparison doesn't need to be that precise as it
just changes the batch passed to percpu_counter_add() to get the
value folded back into the global counter immediately near ENOSPC.
This is done so percpu_counter_read() becomes more accurate as
ENOSPC is approached as that is used for monitoring and reporting
(e.g. via vfsstat). If we want to avoid a counter sum, then this
is the comparison we will need to modify in XFS.

However, the second comparison needs to be precise as that's the one
that does the ENOSPC detection. That sum needs to be done after the
counter add that "uses" the space and so there is no avoiding having
an expensive counter sum as we near ENOSPC....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1241919 — Re: [PATCH] percpu_counter: return precise count from __percpu_counter_compare()

Hello, Dave.

On Thu, Oct 08, 2015 at 12:02:18PM +1100, Dave Chinner wrote:
> > percpu cmpxchg is no different from sub or any other operations
> > regarding cross-CPU synchronization.  They're safe iff the operations
> > are on the local CPU.  They have to be made atomics if they need to be
> > manipulated from remote CPUs.
> 
> Again, another trivially solvable problem, but still irrelevant
> because we don't have the data that tells us whether changing the
> counter behaviour solves the problem....

Dude, it isn't trivially solvable.  You either can't do it or have to
pay the overhead during local access to get around it.

> > That said, while we can't manipulate the percpu counters directly, we
> > can add a separate global counter to cache sum result from the
> > previous run which gets automatically invalidated when any percpu
> > counter overflows.
> >
> > That should give better and in case of
> > back-to-back invocations pretty good precision compared to just
> > returning the global overflow counter.  Interface-wise, that'd be a
> > lot easier to deal with although I have no idea whether it'd fit this
> > particular use case or whether this use case even exists.
> 
> No, it doesn't help - it's effectively what Waiman's original patch
> did by returning the count from the initial comparison and using
> that for ENOSPC detection instead of doing a second comparison...

Just chipping in purely from percpu side.  If what Waiman suggested is
something useable, caching the result inside percpu_counter would be a
better interface.  If not, no idea.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1242574

On 10/07/2015 09:02 PM, Dave Chinner wrote:
> On Wed, Oct 07, 2015 at 04:20:10PM -0700, Tejun Heo wrote:
>> Hello, Dave.
>>
>> On Thu, Oct 08, 2015 at 10:04:42AM +1100, Dave Chinner wrote:
>> ...
>>> As it is, the update race you pointed out is easy to solve with
>>> __this_cpu_cmpxchg rather than _this_cpu_sub (similar to mod_state()
>>> in the MM percpu counter stats code, perhaps).
>> percpu cmpxchg is no different from sub or any other operations
>> regarding cross-CPU synchronization.  They're safe iff the operations
>> are on the local CPU.  They have to be made atomics if they need to be
>> manipulated from remote CPUs.
> Again, another trivially solvable problem, but still irrelevant
> because we don't have the data that tells us whether changing the
> counter behaviour solves the problem....
>
>> That said, while we can't manipulate the percpu counters directly, we
>> can add a separate global counter to cache sum result from the
>> previous run which gets automatically invalidated when any percpu
>> counter overflows.
>>
>> That should give better and in case of
>> back-to-back invocations pretty good precision compared to just
>> returning the global overflow counter.  Interface-wise, that'd be a
>> lot easier to deal with although I have no idea whether it'd fit this
>> particular use case or whether this use case even exists.
> No, it doesn't help - it's effectively what Waiman's original patch
> did by returning the count from the initial comparison and using
> that for ENOSPC detection instead of doing a second comparison...
>
> FWIW, XFS has done an expensive per-cpu counter sum in this ENOSPC
> situation since 2006, but in 2007 ENOSPC was wrapped in a mutex to
> prevent spinlock contention on the aggregated global counter:
>
> commit 20b642858b6bb413976ff13ae6a35cc596967bab
> Author: David Chinner<dgc@sgi.com>
> Date:   Sat Feb 10 18:35:09 2007 +1100
>
>      [XFS] Reduction global superblock lock contention near ENOSPC.
>
>      The existing per-cpu superblock counter code uses the global superblock
>      spin lock when we approach ENOSPC for global synchronisation. On larger
>      machines than this code was originally tested on this can still get
>      catastrophic spinlock contention due increasing rebalance frequency near
>      ENOSPC.
>
>      By introducing a sleeping lock that is used to serialise balances and
>      modifications near ENOSPC we prevent contention from needlessly from
>      wasting the CPU time of potentially hundreds of CPUs.
>
>      To reduce the number of balances occuring, we separate the need rebalance
>      case from the slow allocate case. Now, a counter running dry will trigger
>      a rebalance during which counters are disabled. Any thread that sees a
>      disabled counter enters a different path where it waits on the new mutex.
>      When it gets the new mutex, it checks if the counter is disabled. If the
>      counter is disabled, then we _know_ that we have to use the global counter
>      and lock and it is safe to do so immediately. Otherwise, we drop the mutex
>      and go back to trying the per-cpu counters which we know were re-enabled.
>
>      SGI-PV: 952227
>      SGI-Modid: xfs-linux-melb:xfs-kern:27612a
>
>      Signed-off-by: David Chinner<dgc@sgi.com>
>      Signed-off-by: Lachlan McIlroy<lachlan@sgi.com>
>      Signed-off-by: Tim Shimmin<tes@sgi.com>
>
> This is effectively the same symptoms that what we are seeing with
> the new "lockless" generic percpu counteri algorithm, which is why
> I'm trying to find out if it an issue with the counter
> implementation before I do anything else...
>
> FWIW, the first comparison doesn't need to be that precise as it
> just changes the batch passed to percpu_counter_add() to get the
> value folded back into the global counter immediately near ENOSPC.
> This is done so percpu_counter_read() becomes more accurate as
> ENOSPC is approached as that is used for monitoring and reporting
> (e.g. via vfsstat). If we want to avoid a counter sum, then this
> is the comparison we will need to modify in XFS.

That is what I have advocated in the in the inlined patch that I sent 
you in a previous mail. That patch modified the first comparison, but 
leave the 2nd comparison intact. We will still see bad performance near 
ENOSPC, but it will be better than before.

> However, the second comparison needs to be precise as that's the one
> that does the ENOSPC detection. That sum needs to be done after the
> counter add that "uses" the space and so there is no avoiding having
> an expensive counter sum as we near ENOSPC....
>
> Cheers,
>
> Dave.

Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1242575

On 10/07/2015 07:04 PM, Dave Chinner wrote:
> On Wed, Oct 07, 2015 at 04:00:42PM -0400, Waiman Long wrote:
>> On 10/06/2015 05:30 PM, Dave Chinner wrote:
>>>>> /*
>>>>>   * Aggregate the per-cpu counter magazines back into the global
>>>>>   * counter. This avoids the need for repeated compare operations to
>>>>>   * run the slow path when the majority of the counter value is held
>>>>>   * in the per-cpu magazines. Folding them back into the global
>>>>>   * counter means we will continue to hit the fast
>>>>>   * percpu_counter_read() path until the counter value falls
>>>>>   * completely within the comparison limit passed to
>>>>>   * __percpu_counter_compare().
>>>>>   */
>>>>> static s64 percpu_counter_aggregate(struct percpu_counter *fbc)
>>>>> {
>>>>> 	s64 ret;
>>>>> 	int cpu;
>>>>> 	unsigned long flags;
>>>>>
>>>>> 	raw_spin_lock_irqsave(&fbc->lock, flags);
>>>>> 	ret = fbc->count;
>>>>> 	for_each_online_cpu(cpu) {
>>>>> 		s32 count = __this_cpu_read(*fbc->counters);
>>>>>                  ret += count;
>>>>> 		__this_cpu_sub(*fbc->counters, count)
>>>>> 	}
>>>>> 	fbc->count = ret;
>>>>> 	raw_spin_unlock_irqrestore(&fbc->lock, flags);
>>>>> 	return ret;
>>>>> }
>>>> I don't think that will work as some other CPUs may change the
>>>> percpu counters values between percpu_counter_aggregate() and
>>>> __percpu_counter_compare().  To be safe, the precise counter has to
>>>> be compted whenever the comparison value difference is less than
>>>> nr_cpus * batch size.
>>> Well, yes. Why do you think the above function does the same
>>> function as percpu_counter_sum()? So that the percpu_counter_sum()
>>> call *inside* __percpu_counter_compare() can be replaced by this
>>> call. i.e.
>>>
>>> 			return -1;
>>> 	}
>>> 	/* Need to use precise count */
>>> -	count = percpu_counter_sum(fbc);
>>> +	count = percpu_counter_aggregate(fbc);
>>> 	if (count>   rhs)
>>> 		return 1;
>>> 	else if (count<   rhs)
>>>
>>> Please think about what I'm saying rather than dismissing it without
>>> first understanding my suggestions.
>> I understood what you were saying. However, the per-cpu counter
>> isn't protected by the spinlock. Reading it is OK, but writing may
>> cause race if that counter is modified by a CPU other than its
>> owning CPU.
> <sigh>
>
> You're still trying to pick apart the code without considering what
> we need to acheive.  We don't need to the code to be bullet proof to
> test whether this hypothesis is correct or not - we just need
> something that is "near-enough" to give us the data point to tell us
> where we should focus our efforts. If optimising the counter like
> above does not reduce the overhead, then we may have to change XFS.
> If it does reduce the overhead, then the XFS code remains unchanged
> and we focus on optimising the counter code.

What determine if a precise sum is to be computed is the following code:

         if (abs(count - rhs) > (batch * num_online_cpus())) {

So even if we make the global count more accurate using 
percpu_counter_aggregate(), it won't have too much effect in reducing 
the chance where the precise count needs to be calculated. That is why I 
don't bother testing it with the modified code.

Cheers,
Longman

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web