[PATCH 1/2] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[PATCH 1/2] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[Nikolay Borisov]

252e0ba6b77d ("lib: percpu_counter variable batch") added a batched version
of percpu_counter_add. However, one problem with this patch is the fact that it
overloads the meaning of double underscore, which in kernel-land are taken
to implicitly mean there is no preempt protection for the API. Currently, in
both !SMP and SMP configs percpu_counter_add calls __percpu_counter_add which
is preempt safe due to explicit calls to preempt_disable. This state of play
creates the false sense that __percpu_counter_add is less SMP-safe than
percpu_counter_add. They are both identical irrespective of CONFIG_SNMP value.
The only difference is that the __ version takes a batch parameter.

Make this a bit more explicit by just renaming __percpu_counter_add to
percpu_counter_add_batch.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c             | 4 ++--
 fs/btrfs/extent_io.c           | 2 +-
 fs/btrfs/inode.c               | 4 ++--
 fs/xfs/xfs_mount.c             | 4 ++--
 include/linux/backing-dev.h    | 2 +-
 include/linux/blk-cgroup.h     | 6 +++---
 include/linux/mman.h           | 2 +-
 include/linux/percpu_counter.h | 7 ++++---
 include/net/inet_frag.h        | 4 ++--
 lib/flex_proportions.c         | 6 +++---
 lib/percpu_counter.c           | 4 ++--
 11 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 2eaa1b1db08d..9fc37d6641cd 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1255,7 +1255,7 @@ void clean_tree_block(struct btrfs_fs_info *fs_info,
 		btrfs_assert_tree_locked(buf);
 
 		if (test_and_clear_bit(EXTENT_BUFFER_DIRTY, &buf->bflags)) {
-			__percpu_counter_add(&fs_info->dirty_metadata_bytes,
+			percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
 					     -buf->len,
 					     fs_info->dirty_metadata_batch);
 			/* ugh, clear_extent_buffer_dirty needs to lock the page */
@@ -4048,7 +4048,7 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 			buf->start, transid, fs_info->generation);
 	was_dirty = set_extent_buffer_dirty(buf);
 	if (!was_dirty)
-		__percpu_counter_add(&fs_info->dirty_metadata_bytes,
+		percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
 				     buf->len,
 				     fs_info->dirty_metadata_batch);
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d3619e010005..a1c303f27699 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3597,7 +3597,7 @@ lock_extent_buffer_for_io(struct extent_buffer *eb,
 		set_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
 		spin_unlock(&eb->refs_lock);
 		btrfs_set_header_flag(eb, BTRFS_HEADER_FLAG_WRITTEN);
-		__percpu_counter_add(&fs_info->dirty_metadata_bytes,
+		percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
 				     -eb->len,
 				     fs_info->dirty_metadata_batch);
 		ret = 1;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ef3c98c527c1..fa138217219e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1766,7 +1766,7 @@ static void btrfs_set_bit_hook(struct inode *inode,
 		if (btrfs_is_testing(fs_info))
 			return;
 
-		__percpu_counter_add(&fs_info->delalloc_bytes, len,
+		percpu_counter_add_batch(&fs_info->delalloc_bytes, len,
 				     fs_info->delalloc_batch);
 		spin_lock(&BTRFS_I(inode)->lock);
 		BTRFS_I(inode)->delalloc_bytes += len;
@@ -1840,7 +1840,7 @@ static void btrfs_clear_bit_hook(struct btrfs_inode *inode,
 					&inode->vfs_inode,
 					state->start, len);
 
-		__percpu_counter_add(&fs_info->delalloc_bytes, -len,
+		percpu_counter_add_batch(&fs_info->delalloc_bytes, -len,
 				     fs_info->delalloc_batch);
 		spin_lock(&inode->lock);
 		inode->delalloc_bytes -= len;
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 2eaf81859166..7147d4a8d207 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1209,7 +1209,7 @@ xfs_mod_icount(
 	struct xfs_mount	*mp,
 	int64_t			delta)
 {
-	__percpu_counter_add(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
+	percpu_counter_add_batch(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
 	if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) {
 		ASSERT(0);
 		percpu_counter_add(&mp->m_icount, -delta);
@@ -1288,7 +1288,7 @@ xfs_mod_fdblocks(
 	else
 		batch = XFS_FDBLOCKS_BATCH;
 
-	__percpu_counter_add(&mp->m_fdblocks, delta, batch);
+	percpu_counter_add_batch(&mp->m_fdblocks, delta, batch);
 	if (__percpu_counter_compare(&mp->m_fdblocks, mp->m_alloc_set_aside,
 				     XFS_FDBLOCKS_BATCH) >= 0) {
 		/* we had space! */
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 557d84063934..ace73f96eb1e 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -66,7 +66,7 @@ static inline bool bdi_has_dirty_io(struct backing_dev_info *bdi)
 static inline void __add_wb_stat(struct bdi_writeback *wb,
 				 enum wb_stat_item item, s64 amount)
 {
-	__percpu_counter_add(&wb->stat[item], amount, WB_STAT_BATCH);
+	percpu_counter_add_batch(&wb->stat[item], amount, WB_STAT_BATCH);
 }
 
 static inline void __inc_wb_stat(struct bdi_writeback *wb,
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 01b62e7bac74..7104bea8dab1 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -518,7 +518,7 @@ static inline void blkg_stat_exit(struct blkg_stat *stat)
  */
 static inline void blkg_stat_add(struct blkg_stat *stat, uint64_t val)
 {
-	__percpu_counter_add(&stat->cpu_cnt, val, BLKG_STAT_CPU_BATCH);
+	percpu_counter_add_batch(&stat->cpu_cnt, val, BLKG_STAT_CPU_BATCH);
 }
 
 /**
@@ -597,14 +597,14 @@ static inline void blkg_rwstat_add(struct blkg_rwstat *rwstat,
 	else
 		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_READ];
 
-	__percpu_counter_add(cnt, val, BLKG_STAT_CPU_BATCH);
+	percpu_counter_add_batch(cnt, val, BLKG_STAT_CPU_BATCH);
 
 	if (op_is_sync(op))
 		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_SYNC];
 	else
 		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_ASYNC];
 
-	__percpu_counter_add(cnt, val, BLKG_STAT_CPU_BATCH);
+	percpu_counter_add_batch(cnt, val, BLKG_STAT_CPU_BATCH);
 }
 
 /**
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 634c4c51fe3a..c8367041fafd 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -22,7 +22,7 @@ unsigned long vm_memory_committed(void);
 
 static inline void vm_acct_memory(long pages)
 {
-	__percpu_counter_add(&vm_committed_as, pages, vm_committed_as_batch);
+	percpu_counter_add_batch(&vm_committed_as, pages, vm_committed_as_batch);
 }
 
 static inline void vm_unacct_memory(long pages)
diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
index 84a109449610..ec065387f443 100644
--- a/include/linux/percpu_counter.h
+++ b/include/linux/percpu_counter.h
@@ -39,7 +39,8 @@ int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, gfp_t gfp,
 
 void percpu_counter_destroy(struct percpu_counter *fbc);
 void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
-void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch);
+void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount,
+			      s32 batch);
 s64 __percpu_counter_sum(struct percpu_counter *fbc);
 int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch);
 
@@ -50,7 +51,7 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs)
 
 static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount)
 {
-	__percpu_counter_add(fbc, amount, percpu_counter_batch);
+	percpu_counter_add_batch(fbc, amount, percpu_counter_batch);
 }
 
 static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)
@@ -136,7 +137,7 @@ percpu_counter_add(struct percpu_counter *fbc, s64 amount)
 }
 
 static inline void
-__percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
+percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
 {
 	percpu_counter_add(fbc, amount);
 }
diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 5894730ec82a..5932e6de8fc0 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -154,12 +154,12 @@ static inline int frag_mem_limit(struct netns_frags *nf)
 
 static inline void sub_frag_mem_limit(struct netns_frags *nf, int i)
 {
-	__percpu_counter_add(&nf->mem, -i, frag_percpu_counter_batch);
+	percpu_counter_add_batch(&nf->mem, -i, frag_percpu_counter_batch);
 }
 
 static inline void add_frag_mem_limit(struct netns_frags *nf, int i)
 {
-	__percpu_counter_add(&nf->mem, i, frag_percpu_counter_batch);
+	percpu_counter_add_batch(&nf->mem, i, frag_percpu_counter_batch);
 }
 
 static inline unsigned int sum_frag_mem_limit(struct netns_frags *nf)
diff --git a/lib/flex_proportions.c b/lib/flex_proportions.c
index a71cf1bdd4c9..2cc1f94e03a1 100644
--- a/lib/flex_proportions.c
+++ b/lib/flex_proportions.c
@@ -207,7 +207,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
 		if (val < (nr_cpu_ids * PROP_BATCH))
 			val = percpu_counter_sum(&pl->events);
 
-		__percpu_counter_add(&pl->events,
+		percpu_counter_add_batch(&pl->events,
 			-val + (val >> (period-pl->period)), PROP_BATCH);
 	} else
 		percpu_counter_set(&pl->events, 0);
@@ -219,7 +219,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
 void __fprop_inc_percpu(struct fprop_global *p, struct fprop_local_percpu *pl)
 {
 	fprop_reflect_period_percpu(p, pl);
-	__percpu_counter_add(&pl->events, 1, PROP_BATCH);
+	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
 	percpu_counter_add(&p->events, 1);
 }
 
@@ -267,6 +267,6 @@ void __fprop_inc_percpu_max(struct fprop_global *p,
 			return;
 	} else
 		fprop_reflect_period_percpu(p, pl);
-	__percpu_counter_add(&pl->events, 1, PROP_BATCH);
+	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
 	percpu_counter_add(&p->events, 1);
 }
diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index 9c21000df0b5..8ee7e5ec21be 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -72,7 +72,7 @@ void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
 }
 EXPORT_SYMBOL(percpu_counter_set);
 
-void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
+void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
 {
 	s64 count;
 
@@ -89,7 +89,7 @@ void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
 	}
 	preempt_enable();
 }
-EXPORT_SYMBOL(__percpu_counter_add);
+EXPORT_SYMBOL(percpu_counter_add_batch);
 
 /*
  * Add up all the per-cpu counts, return the result.  This is a more accurate
-- 
2.7.4

[PATCH 2/2] writeback: Rework wb_[dec|inc]_stat family of functions

[Nikolay Borisov]

Currently the writeback statistics code uses a percpu counters to hold
various statistics. As such we have 2 families of functions - those which
disable local irq and those which doesn't and whose names are begin with
double underscore. However, they both end up calling __add_wb_stats which in
turn end up calling percpu_counter_add_batch which is already SMP-safe.

Exploiting this fact allows to eliminated the __wb_* functions since they do
in fact cal SMP-safe primitive. Furthermore, refactor the wb_* function
to call __add_wb_stat directly without the irq-disabling dance. This will
likely result in better runtime of code which deals with modifying the stat
counters.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
Hello Tejun, 

This patch resulted from me reading your feedback on Josef's memory 
throttling prep patch https://patchwork.kernel.org/patch/9395219/ . If these
changes are merged then his series can eliminated irq clustering and use 
straight __add_wb_stat call. I'd like to see his series merged sooner rather
than later hence why sending this cleanup. 

 include/linux/backing-dev.h | 24 ++----------------------
 mm/page-writeback.c         | 10 +++++-----
 2 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index ace73f96eb1e..e9c967b86054 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -69,34 +69,14 @@ static inline void __add_wb_stat(struct bdi_writeback *wb,
 	percpu_counter_add_batch(&wb->stat[item], amount, WB_STAT_BATCH);
 }
 
-static inline void __inc_wb_stat(struct bdi_writeback *wb,
-				 enum wb_stat_item item)
-{
-	__add_wb_stat(wb, item, 1);
-}
-
 static inline void inc_wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__inc_wb_stat(wb, item);
-	local_irq_restore(flags);
-}
-
-static inline void __dec_wb_stat(struct bdi_writeback *wb,
-				 enum wb_stat_item item)
-{
-	__add_wb_stat(wb, item, -1);
+	__add_wb_stat(wb, item, 1);
 }
 
 static inline void dec_wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__dec_wb_stat(wb, item);
-	local_irq_restore(flags);
+	__add_wb_stat(wb, item, -1);
 }
 
 static inline s64 wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 143c1c25d680..b7451891959a 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -601,7 +601,7 @@ static inline void __wb_writeout_inc(struct bdi_writeback *wb)
 {
 	struct wb_domain *cgdom;
 
-	__inc_wb_stat(wb, WB_WRITTEN);
+	inc_wb_stat(wb, WB_WRITTEN);
 	wb_domain_writeout_inc(&global_wb_domain, &wb->completions,
 			       wb->bdi->max_prop_frac);
 
@@ -2437,8 +2437,8 @@ void account_page_dirtied(struct page *page, struct address_space *mapping)
 		__inc_node_page_state(page, NR_FILE_DIRTY);
 		__inc_zone_page_state(page, NR_ZONE_WRITE_PENDING);
 		__inc_node_page_state(page, NR_DIRTIED);
-		__inc_wb_stat(wb, WB_RECLAIMABLE);
-		__inc_wb_stat(wb, WB_DIRTIED);
+		inc_wb_stat(wb, WB_RECLAIMABLE);
+		inc_wb_stat(wb, WB_DIRTIED);
 		task_io_account_write(PAGE_SIZE);
 		current->nr_dirtied++;
 		this_cpu_inc(bdp_ratelimits);
@@ -2745,7 +2745,7 @@ int test_clear_page_writeback(struct page *page)
 			if (bdi_cap_account_writeback(bdi)) {
 				struct bdi_writeback *wb = inode_to_wb(inode);
 
-				__dec_wb_stat(wb, WB_WRITEBACK);
+				dec_wb_stat(wb, WB_WRITEBACK);
 				__wb_writeout_inc(wb);
 			}
 		}
@@ -2791,7 +2791,7 @@ int __test_set_page_writeback(struct page *page, bool keep_write)
 						page_index(page),
 						PAGECACHE_TAG_WRITEBACK);
 			if (bdi_cap_account_writeback(bdi))
-				__inc_wb_stat(inode_to_wb(inode), WB_WRITEBACK);
+				inc_wb_stat(inode_to_wb(inode), WB_WRITEBACK);
 
 			/*
 			 * We can come through here when swapping anonymous
-- 
2.7.4

Re: [PATCH 1/2] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[Tejun Heo]

Hello, Nikolay.

On Tue, Jun 20, 2017 at 02:36:29PM +0300, Nikolay Borisov wrote:
> 252e0ba6b77d ("lib: percpu_counter variable batch") added a batched version
> of percpu_counter_add. However, one problem with this patch is the fact that it
> overloads the meaning of double underscore, which in kernel-land are taken
> to implicitly mean there is no preempt protection for the API. Currently, in

I don't think the above holds.  We use __ for quite a few different
things.  Sometimes it denotes internal functions which shouldn't be
used outside a subsystem, sometimes just more explicit / verbose
versions of certain operations, at other times less protection against
preemption / irq / whatever.

> both !SMP and SMP configs percpu_counter_add calls __percpu_counter_add which
> is preempt safe due to explicit calls to preempt_disable. This state of play
> creates the false sense that __percpu_counter_add is less SMP-safe than
> percpu_counter_add. They are both identical irrespective of CONFIG_SNMP value.
> The only difference is that the __ version takes a batch parameter.
> 
> Make this a bit more explicit by just renaming __percpu_counter_add to
> percpu_counter_add_batch.

I'm all for making the function name more explicit, but can you please
drop the first part of the commit description?

Thanks.

-- 
tejun

Re: [PATCH 2/2] writeback: Rework wb_[dec|inc]_stat family of functions

[Tejun Heo]

Hello,

On Tue, Jun 20, 2017 at 02:36:30PM +0300, Nikolay Borisov wrote:
> Currently the writeback statistics code uses a percpu counters to hold
> various statistics. As such we have 2 families of functions - those which
> disable local irq and those which doesn't and whose names are begin with
> double underscore. However, they both end up calling __add_wb_stats which in
> turn end up calling percpu_counter_add_batch which is already SMP-safe.

There's a difference between being SMP-safe and being preemption / irq
save.  Even on UP machine, rmw cycles can go wrong due to different
contexts operating on the same memory area, but you're right, all
percpu counter ops are irq safe, so there's no reason for wb stat
operations to have different variants.

> Exploiting this fact allows to eliminated the __wb_* functions since they do
> in fact cal SMP-safe primitive. Furthermore, refactor the wb_* function
> to call __add_wb_stat directly without the irq-disabling dance. This will
> likely result in better runtime of code which deals with modifying the stat
> counters.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> Hello Tejun, 
> 
> This patch resulted from me reading your feedback on Josef's memory 
> throttling prep patch https://patchwork.kernel.org/patch/9395219/ . If these
> changes are merged then his series can eliminated irq clustering and use 
> straight __add_wb_stat call. I'd like to see his series merged sooner rather
> than later hence why sending this cleanup. 

Sure, but can you please update the patch description?

Thanks.

-- 
tejun

[PATCH v2 1/2] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[Nikolay Borisov]

Currently, in both !SMP and SMP configs percpu_counter_add calls
__percpu_counter_add which is preempt safe due to explicit calls to
preempt_disable. This state of play creates the false sense that
__percpu_counter_add is less SMP-safe than percpu_counter_add. They are both
identical irrespective of CONFIG_SMP. The only difference is that the
__ version takes a batch parameter.

Make this a bit more explicit by just renaming __percpu_counter_add to
percpu_counter_add_batch.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c             | 4 ++--
 fs/btrfs/extent_io.c           | 2 +-
 fs/btrfs/inode.c               | 4 ++--
 fs/xfs/xfs_mount.c             | 4 ++--
 include/linux/backing-dev.h    | 2 +-
 include/linux/blk-cgroup.h     | 6 +++---
 include/linux/mman.h           | 2 +-
 include/linux/percpu_counter.h | 7 ++++---
 include/net/inet_frag.h        | 4 ++--
 lib/flex_proportions.c         | 6 +++---
 lib/percpu_counter.c           | 4 ++--
 11 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 5f678dcb20e6..0ebd44135f1f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1255,7 +1255,7 @@ void clean_tree_block(struct btrfs_fs_info *fs_info,
 		btrfs_assert_tree_locked(buf);
 
 		if (test_and_clear_bit(EXTENT_BUFFER_DIRTY, &buf->bflags)) {
-			__percpu_counter_add(&fs_info->dirty_metadata_bytes,
+			percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
 					     -buf->len,
 					     fs_info->dirty_metadata_batch);
 			/* ugh, clear_extent_buffer_dirty needs to lock the page */
@@ -4049,7 +4049,7 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 			buf->start, transid, fs_info->generation);
 	was_dirty = set_extent_buffer_dirty(buf);
 	if (!was_dirty)
-		__percpu_counter_add(&fs_info->dirty_metadata_bytes,
+		percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
 				     buf->len,
 				     fs_info->dirty_metadata_batch);
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d3619e010005..a1c303f27699 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3597,7 +3597,7 @@ lock_extent_buffer_for_io(struct extent_buffer *eb,
 		set_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
 		spin_unlock(&eb->refs_lock);
 		btrfs_set_header_flag(eb, BTRFS_HEADER_FLAG_WRITTEN);
-		__percpu_counter_add(&fs_info->dirty_metadata_bytes,
+		percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
 				     -eb->len,
 				     fs_info->dirty_metadata_batch);
 		ret = 1;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ef3c98c527c1..fa138217219e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1766,7 +1766,7 @@ static void btrfs_set_bit_hook(struct inode *inode,
 		if (btrfs_is_testing(fs_info))
 			return;
 
-		__percpu_counter_add(&fs_info->delalloc_bytes, len,
+		percpu_counter_add_batch(&fs_info->delalloc_bytes, len,
 				     fs_info->delalloc_batch);
 		spin_lock(&BTRFS_I(inode)->lock);
 		BTRFS_I(inode)->delalloc_bytes += len;
@@ -1840,7 +1840,7 @@ static void btrfs_clear_bit_hook(struct btrfs_inode *inode,
 					&inode->vfs_inode,
 					state->start, len);
 
-		__percpu_counter_add(&fs_info->delalloc_bytes, -len,
+		percpu_counter_add_batch(&fs_info->delalloc_bytes, -len,
 				     fs_info->delalloc_batch);
 		spin_lock(&inode->lock);
 		inode->delalloc_bytes -= len;
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 2eaf81859166..7147d4a8d207 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1209,7 +1209,7 @@ xfs_mod_icount(
 	struct xfs_mount	*mp,
 	int64_t			delta)
 {
-	__percpu_counter_add(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
+	percpu_counter_add_batch(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
 	if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) {
 		ASSERT(0);
 		percpu_counter_add(&mp->m_icount, -delta);
@@ -1288,7 +1288,7 @@ xfs_mod_fdblocks(
 	else
 		batch = XFS_FDBLOCKS_BATCH;
 
-	__percpu_counter_add(&mp->m_fdblocks, delta, batch);
+	percpu_counter_add_batch(&mp->m_fdblocks, delta, batch);
 	if (__percpu_counter_compare(&mp->m_fdblocks, mp->m_alloc_set_aside,
 				     XFS_FDBLOCKS_BATCH) >= 0) {
 		/* we had space! */
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 557d84063934..ace73f96eb1e 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -66,7 +66,7 @@ static inline bool bdi_has_dirty_io(struct backing_dev_info *bdi)
 static inline void __add_wb_stat(struct bdi_writeback *wb,
 				 enum wb_stat_item item, s64 amount)
 {
-	__percpu_counter_add(&wb->stat[item], amount, WB_STAT_BATCH);
+	percpu_counter_add_batch(&wb->stat[item], amount, WB_STAT_BATCH);
 }
 
 static inline void __inc_wb_stat(struct bdi_writeback *wb,
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 01b62e7bac74..7104bea8dab1 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -518,7 +518,7 @@ static inline void blkg_stat_exit(struct blkg_stat *stat)
  */
 static inline void blkg_stat_add(struct blkg_stat *stat, uint64_t val)
 {
-	__percpu_counter_add(&stat->cpu_cnt, val, BLKG_STAT_CPU_BATCH);
+	percpu_counter_add_batch(&stat->cpu_cnt, val, BLKG_STAT_CPU_BATCH);
 }
 
 /**
@@ -597,14 +597,14 @@ static inline void blkg_rwstat_add(struct blkg_rwstat *rwstat,
 	else
 		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_READ];
 
-	__percpu_counter_add(cnt, val, BLKG_STAT_CPU_BATCH);
+	percpu_counter_add_batch(cnt, val, BLKG_STAT_CPU_BATCH);
 
 	if (op_is_sync(op))
 		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_SYNC];
 	else
 		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_ASYNC];
 
-	__percpu_counter_add(cnt, val, BLKG_STAT_CPU_BATCH);
+	percpu_counter_add_batch(cnt, val, BLKG_STAT_CPU_BATCH);
 }
 
 /**
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 634c4c51fe3a..c8367041fafd 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -22,7 +22,7 @@ unsigned long vm_memory_committed(void);
 
 static inline void vm_acct_memory(long pages)
 {
-	__percpu_counter_add(&vm_committed_as, pages, vm_committed_as_batch);
+	percpu_counter_add_batch(&vm_committed_as, pages, vm_committed_as_batch);
 }
 
 static inline void vm_unacct_memory(long pages)
diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
index 84a109449610..ec065387f443 100644
--- a/include/linux/percpu_counter.h
+++ b/include/linux/percpu_counter.h
@@ -39,7 +39,8 @@ int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, gfp_t gfp,
 
 void percpu_counter_destroy(struct percpu_counter *fbc);
 void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
-void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch);
+void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount,
+			      s32 batch);
 s64 __percpu_counter_sum(struct percpu_counter *fbc);
 int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch);
 
@@ -50,7 +51,7 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs)
 
 static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount)
 {
-	__percpu_counter_add(fbc, amount, percpu_counter_batch);
+	percpu_counter_add_batch(fbc, amount, percpu_counter_batch);
 }
 
 static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)
@@ -136,7 +137,7 @@ percpu_counter_add(struct percpu_counter *fbc, s64 amount)
 }
 
 static inline void
-__percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
+percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
 {
 	percpu_counter_add(fbc, amount);
 }
diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 5894730ec82a..5932e6de8fc0 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -154,12 +154,12 @@ static inline int frag_mem_limit(struct netns_frags *nf)
 
 static inline void sub_frag_mem_limit(struct netns_frags *nf, int i)
 {
-	__percpu_counter_add(&nf->mem, -i, frag_percpu_counter_batch);
+	percpu_counter_add_batch(&nf->mem, -i, frag_percpu_counter_batch);
 }
 
 static inline void add_frag_mem_limit(struct netns_frags *nf, int i)
 {
-	__percpu_counter_add(&nf->mem, i, frag_percpu_counter_batch);
+	percpu_counter_add_batch(&nf->mem, i, frag_percpu_counter_batch);
 }
 
 static inline unsigned int sum_frag_mem_limit(struct netns_frags *nf)
diff --git a/lib/flex_proportions.c b/lib/flex_proportions.c
index a71cf1bdd4c9..2cc1f94e03a1 100644
--- a/lib/flex_proportions.c
+++ b/lib/flex_proportions.c
@@ -207,7 +207,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
 		if (val < (nr_cpu_ids * PROP_BATCH))
 			val = percpu_counter_sum(&pl->events);
 
-		__percpu_counter_add(&pl->events,
+		percpu_counter_add_batch(&pl->events,
 			-val + (val >> (period-pl->period)), PROP_BATCH);
 	} else
 		percpu_counter_set(&pl->events, 0);
@@ -219,7 +219,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
 void __fprop_inc_percpu(struct fprop_global *p, struct fprop_local_percpu *pl)
 {
 	fprop_reflect_period_percpu(p, pl);
-	__percpu_counter_add(&pl->events, 1, PROP_BATCH);
+	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
 	percpu_counter_add(&p->events, 1);
 }
 
@@ -267,6 +267,6 @@ void __fprop_inc_percpu_max(struct fprop_global *p,
 			return;
 	} else
 		fprop_reflect_period_percpu(p, pl);
-	__percpu_counter_add(&pl->events, 1, PROP_BATCH);
+	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
 	percpu_counter_add(&p->events, 1);
 }
diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index 9c21000df0b5..8ee7e5ec21be 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -72,7 +72,7 @@ void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
 }
 EXPORT_SYMBOL(percpu_counter_set);
 
-void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
+void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
 {
 	s64 count;
 
@@ -89,7 +89,7 @@ void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
 	}
 	preempt_enable();
 }
-EXPORT_SYMBOL(__percpu_counter_add);
+EXPORT_SYMBOL(percpu_counter_add_batch);
 
 /*
  * Add up all the per-cpu counts, return the result.  This is a more accurate
-- 
2.7.4

[PATCH v2 2/2] writeback: Rework wb_[dec|inc]_stat family of functions

[Nikolay Borisov]

Currently the writeback statistics code uses a percpu counters to hold
various statistics. Furthermore we have 2 families of functions - those which
disable local irq and those which doesn't and whose names begin with
double underscore. However, they both end up calling __add_wb_stats which in
turn calls percpu_counter_add_batch which is already irq-safe.

Exploiting this fact allows to eliminated the __wb_* functions since they don't
add any further protection than we already have. Furthermore, refactor
the wb_* function to call __add_wb_stat directly without the irq-disabling
dance. This will likely result in better runtime of code which deals with
modifying the stat counters.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 include/linux/backing-dev.h | 24 ++----------------------
 mm/page-writeback.c         | 10 +++++-----
 2 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index ace73f96eb1e..e9c967b86054 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -69,34 +69,14 @@ static inline void __add_wb_stat(struct bdi_writeback *wb,
 	percpu_counter_add_batch(&wb->stat[item], amount, WB_STAT_BATCH);
 }
 
-static inline void __inc_wb_stat(struct bdi_writeback *wb,
-				 enum wb_stat_item item)
-{
-	__add_wb_stat(wb, item, 1);
-}
-
 static inline void inc_wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__inc_wb_stat(wb, item);
-	local_irq_restore(flags);
-}
-
-static inline void __dec_wb_stat(struct bdi_writeback *wb,
-				 enum wb_stat_item item)
-{
-	__add_wb_stat(wb, item, -1);
+	__add_wb_stat(wb, item, 1);
 }
 
 static inline void dec_wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__dec_wb_stat(wb, item);
-	local_irq_restore(flags);
+	__add_wb_stat(wb, item, -1);
 }
 
 static inline s64 wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 143c1c25d680..b7451891959a 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -601,7 +601,7 @@ static inline void __wb_writeout_inc(struct bdi_writeback *wb)
 {
 	struct wb_domain *cgdom;
 
-	__inc_wb_stat(wb, WB_WRITTEN);
+	inc_wb_stat(wb, WB_WRITTEN);
 	wb_domain_writeout_inc(&global_wb_domain, &wb->completions,
 			       wb->bdi->max_prop_frac);
 
@@ -2437,8 +2437,8 @@ void account_page_dirtied(struct page *page, struct address_space *mapping)
 		__inc_node_page_state(page, NR_FILE_DIRTY);
 		__inc_zone_page_state(page, NR_ZONE_WRITE_PENDING);
 		__inc_node_page_state(page, NR_DIRTIED);
-		__inc_wb_stat(wb, WB_RECLAIMABLE);
-		__inc_wb_stat(wb, WB_DIRTIED);
+		inc_wb_stat(wb, WB_RECLAIMABLE);
+		inc_wb_stat(wb, WB_DIRTIED);
 		task_io_account_write(PAGE_SIZE);
 		current->nr_dirtied++;
 		this_cpu_inc(bdp_ratelimits);
@@ -2745,7 +2745,7 @@ int test_clear_page_writeback(struct page *page)
 			if (bdi_cap_account_writeback(bdi)) {
 				struct bdi_writeback *wb = inode_to_wb(inode);
 
-				__dec_wb_stat(wb, WB_WRITEBACK);
+				dec_wb_stat(wb, WB_WRITEBACK);
 				__wb_writeout_inc(wb);
 			}
 		}
@@ -2791,7 +2791,7 @@ int __test_set_page_writeback(struct page *page, bool keep_write)
 						page_index(page),
 						PAGECACHE_TAG_WRITEBACK);
 			if (bdi_cap_account_writeback(bdi))
-				__inc_wb_stat(inode_to_wb(inode), WB_WRITEBACK);
+				inc_wb_stat(inode_to_wb(inode), WB_WRITEBACK);
 
 			/*
 			 * We can come through here when swapping anonymous
-- 
2.7.4

Re: [PATCH v2 2/2] writeback: Rework wb_[dec|inc]_stat family of functions

[Tejun Heo]

Hello, Nikolay.

On Tue, Jun 20, 2017 at 09:02:00PM +0300, Nikolay Borisov wrote:
> Currently the writeback statistics code uses a percpu counters to hold
> various statistics. Furthermore we have 2 families of functions - those which
> disable local irq and those which doesn't and whose names begin with
> double underscore. However, they both end up calling __add_wb_stats which in
> turn calls percpu_counter_add_batch which is already irq-safe.

Heh, looks like I was confused.  __percpu_counter_add() is not
irq-safe.  It disables preemption and uses __this_cpu_read(), so
there's no protection against irq.  If writeback statistics want
irq-safe operations and it does, it would need these separate
operations.  Am I missing something?

Thanks.

-- 
tejun

[PATCH] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[Tejun Heo]

>From 104b4e5139fe384431ac11c3b8a6cf4a529edf4a Mon Sep 17 00:00:00 2001
From: Nikolay Borisov <nborisov@suse.com>
Date: Tue, 20 Jun 2017 21:01:20 +0300

Currently, percpu_counter_add is a wrapper around __percpu_counter_add
which is preempt safe due to explicit calls to preempt_disable.  Given
how __ prefix is used in percpu related interfaces, the naming
unfortunately creates the false sense that __percpu_counter_add is
less safe than percpu_counter_add.  In terms of context-safety,
they're equivalent.  The only difference is that the __ version takes
a batch parameter.

Make this a bit more explicit by just renaming __percpu_counter_add to
percpu_counter_add_batch.

This patch doesn't cause any functional changes.

tj: Minor updates to patch description for clarity.  Cosmetic
    indentation updates.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <jbacik@fb.com>
Cc: David Sterba <dsterba@suse.com>
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: Jan Kara <jack@suse.com>
Cc: Jens Axboe <axboe@fb.com>
Cc: linux-mm@kvack.org
Cc: "David S. Miller" <davem@davemloft.net>
---
Hello,

Applying this patch to percpu/for-4.13.  It's a pure rename patch.  If
there's any objection, please let me know.

Thanks.

 fs/btrfs/disk-io.c             | 12 ++++++------
 fs/btrfs/extent_io.c           |  6 +++---
 fs/btrfs/inode.c               |  8 ++++----
 fs/xfs/xfs_mount.c             |  4 ++--
 include/linux/backing-dev.h    |  2 +-
 include/linux/blk-cgroup.h     |  6 +++---
 include/linux/mman.h           |  2 +-
 include/linux/percpu_counter.h |  7 ++++---
 include/net/inet_frag.h        |  4 ++--
 lib/flex_proportions.c         |  6 +++---
 lib/percpu_counter.c           |  4 ++--
 11 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 061c1d1f774f..eb5c95569300 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1255,9 +1255,9 @@ void clean_tree_block(struct btrfs_fs_info *fs_info,
 		btrfs_assert_tree_locked(buf);
 
 		if (test_and_clear_bit(EXTENT_BUFFER_DIRTY, &buf->bflags)) {
-			__percpu_counter_add(&fs_info->dirty_metadata_bytes,
-					     -buf->len,
-					     fs_info->dirty_metadata_batch);
+			percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
+						 -buf->len,
+						 fs_info->dirty_metadata_batch);
 			/* ugh, clear_extent_buffer_dirty needs to lock the page */
 			btrfs_set_lock_blocking(buf);
 			clear_extent_buffer_dirty(buf);
@@ -4046,9 +4046,9 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 			buf->start, transid, fs_info->generation);
 	was_dirty = set_extent_buffer_dirty(buf);
 	if (!was_dirty)
-		__percpu_counter_add(&fs_info->dirty_metadata_bytes,
-				     buf->len,
-				     fs_info->dirty_metadata_batch);
+		percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
+					 buf->len,
+					 fs_info->dirty_metadata_batch);
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
 	if (btrfs_header_level(buf) == 0 && check_leaf(root, buf)) {
 		btrfs_print_leaf(fs_info, buf);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 27fdb250b446..a3455d216a1d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3584,9 +3584,9 @@ lock_extent_buffer_for_io(struct extent_buffer *eb,
 		set_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
 		spin_unlock(&eb->refs_lock);
 		btrfs_set_header_flag(eb, BTRFS_HEADER_FLAG_WRITTEN);
-		__percpu_counter_add(&fs_info->dirty_metadata_bytes,
-				     -eb->len,
-				     fs_info->dirty_metadata_batch);
+		percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
+					 -eb->len,
+					 fs_info->dirty_metadata_batch);
 		ret = 1;
 	} else {
 		spin_unlock(&eb->refs_lock);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 5e71f1ea3391..aabdcb9f234f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1682,8 +1682,8 @@ static void btrfs_set_bit_hook(struct inode *inode,
 		if (btrfs_is_testing(fs_info))
 			return;
 
-		__percpu_counter_add(&fs_info->delalloc_bytes, len,
-				     fs_info->delalloc_batch);
+		percpu_counter_add_batch(&fs_info->delalloc_bytes, len,
+					 fs_info->delalloc_batch);
 		spin_lock(&BTRFS_I(inode)->lock);
 		BTRFS_I(inode)->delalloc_bytes += len;
 		if (*bits & EXTENT_DEFRAG)
@@ -1749,8 +1749,8 @@ static void btrfs_clear_bit_hook(struct btrfs_inode *inode,
 					&inode->vfs_inode,
 					state->start, len);
 
-		__percpu_counter_add(&fs_info->delalloc_bytes, -len,
-				     fs_info->delalloc_batch);
+		percpu_counter_add_batch(&fs_info->delalloc_bytes, -len,
+					 fs_info->delalloc_batch);
 		spin_lock(&inode->lock);
 		inode->delalloc_bytes -= len;
 		if (do_list && inode->delalloc_bytes == 0 &&
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 2eaf81859166..7147d4a8d207 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1209,7 +1209,7 @@ xfs_mod_icount(
 	struct xfs_mount	*mp,
 	int64_t			delta)
 {
-	__percpu_counter_add(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
+	percpu_counter_add_batch(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
 	if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) {
 		ASSERT(0);
 		percpu_counter_add(&mp->m_icount, -delta);
@@ -1288,7 +1288,7 @@ xfs_mod_fdblocks(
 	else
 		batch = XFS_FDBLOCKS_BATCH;
 
-	__percpu_counter_add(&mp->m_fdblocks, delta, batch);
+	percpu_counter_add_batch(&mp->m_fdblocks, delta, batch);
 	if (__percpu_counter_compare(&mp->m_fdblocks, mp->m_alloc_set_aside,
 				     XFS_FDBLOCKS_BATCH) >= 0) {
 		/* we had space! */
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 557d84063934..ace73f96eb1e 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -66,7 +66,7 @@ static inline bool bdi_has_dirty_io(struct backing_dev_info *bdi)
 static inline void __add_wb_stat(struct bdi_writeback *wb,
 				 enum wb_stat_item item, s64 amount)
 {
-	__percpu_counter_add(&wb->stat[item], amount, WB_STAT_BATCH);
+	percpu_counter_add_batch(&wb->stat[item], amount, WB_STAT_BATCH);
 }
 
 static inline void __inc_wb_stat(struct bdi_writeback *wb,
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 01b62e7bac74..7104bea8dab1 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -518,7 +518,7 @@ static inline void blkg_stat_exit(struct blkg_stat *stat)
  */
 static inline void blkg_stat_add(struct blkg_stat *stat, uint64_t val)
 {
-	__percpu_counter_add(&stat->cpu_cnt, val, BLKG_STAT_CPU_BATCH);
+	percpu_counter_add_batch(&stat->cpu_cnt, val, BLKG_STAT_CPU_BATCH);
 }
 
 /**
@@ -597,14 +597,14 @@ static inline void blkg_rwstat_add(struct blkg_rwstat *rwstat,
 	else
 		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_READ];
 
-	__percpu_counter_add(cnt, val, BLKG_STAT_CPU_BATCH);
+	percpu_counter_add_batch(cnt, val, BLKG_STAT_CPU_BATCH);
 
 	if (op_is_sync(op))
 		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_SYNC];
 	else
 		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_ASYNC];
 
-	__percpu_counter_add(cnt, val, BLKG_STAT_CPU_BATCH);
+	percpu_counter_add_batch(cnt, val, BLKG_STAT_CPU_BATCH);
 }
 
 /**
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 634c4c51fe3a..c8367041fafd 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -22,7 +22,7 @@ unsigned long vm_memory_committed(void);
 
 static inline void vm_acct_memory(long pages)
 {
-	__percpu_counter_add(&vm_committed_as, pages, vm_committed_as_batch);
+	percpu_counter_add_batch(&vm_committed_as, pages, vm_committed_as_batch);
 }
 
 static inline void vm_unacct_memory(long pages)
diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
index 84a109449610..ec065387f443 100644
--- a/include/linux/percpu_counter.h
+++ b/include/linux/percpu_counter.h
@@ -39,7 +39,8 @@ int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, gfp_t gfp,
 
 void percpu_counter_destroy(struct percpu_counter *fbc);
 void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
-void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch);
+void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount,
+			      s32 batch);
 s64 __percpu_counter_sum(struct percpu_counter *fbc);
 int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch);
 
@@ -50,7 +51,7 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs)
 
 static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount)
 {
-	__percpu_counter_add(fbc, amount, percpu_counter_batch);
+	percpu_counter_add_batch(fbc, amount, percpu_counter_batch);
 }
 
 static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)
@@ -136,7 +137,7 @@ percpu_counter_add(struct percpu_counter *fbc, s64 amount)
 }
 
 static inline void
-__percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
+percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
 {
 	percpu_counter_add(fbc, amount);
 }
diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 5894730ec82a..5932e6de8fc0 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -154,12 +154,12 @@ static inline int frag_mem_limit(struct netns_frags *nf)
 
 static inline void sub_frag_mem_limit(struct netns_frags *nf, int i)
 {
-	__percpu_counter_add(&nf->mem, -i, frag_percpu_counter_batch);
+	percpu_counter_add_batch(&nf->mem, -i, frag_percpu_counter_batch);
 }
 
 static inline void add_frag_mem_limit(struct netns_frags *nf, int i)
 {
-	__percpu_counter_add(&nf->mem, i, frag_percpu_counter_batch);
+	percpu_counter_add_batch(&nf->mem, i, frag_percpu_counter_batch);
 }
 
 static inline unsigned int sum_frag_mem_limit(struct netns_frags *nf)
diff --git a/lib/flex_proportions.c b/lib/flex_proportions.c
index a71cf1bdd4c9..2cc1f94e03a1 100644
--- a/lib/flex_proportions.c
+++ b/lib/flex_proportions.c
@@ -207,7 +207,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
 		if (val < (nr_cpu_ids * PROP_BATCH))
 			val = percpu_counter_sum(&pl->events);
 
-		__percpu_counter_add(&pl->events,
+		percpu_counter_add_batch(&pl->events,
 			-val + (val >> (period-pl->period)), PROP_BATCH);
 	} else
 		percpu_counter_set(&pl->events, 0);
@@ -219,7 +219,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
 void __fprop_inc_percpu(struct fprop_global *p, struct fprop_local_percpu *pl)
 {
 	fprop_reflect_period_percpu(p, pl);
-	__percpu_counter_add(&pl->events, 1, PROP_BATCH);
+	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
 	percpu_counter_add(&p->events, 1);
 }
 
@@ -267,6 +267,6 @@ void __fprop_inc_percpu_max(struct fprop_global *p,
 			return;
 	} else
 		fprop_reflect_period_percpu(p, pl);
-	__percpu_counter_add(&pl->events, 1, PROP_BATCH);
+	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
 	percpu_counter_add(&p->events, 1);
 }
diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index 9c21000df0b5..8ee7e5ec21be 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -72,7 +72,7 @@ void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
 }
 EXPORT_SYMBOL(percpu_counter_set);
 
-void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
+void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
 {
 	s64 count;
 
@@ -89,7 +89,7 @@ void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
 	}
 	preempt_enable();
 }
-EXPORT_SYMBOL(__percpu_counter_add);
+EXPORT_SYMBOL(percpu_counter_add_batch);
 
 /*
  * Add up all the per-cpu counts, return the result.  This is a more accurate
-- 
2.13.0

[PATCH] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[Tejun Heo]

Re: [PATCH] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[David Miller]

From: Tejun Heo <tj@kernel.org>
Date: Tue, 20 Jun 2017 15:47:59 -0400

> From 104b4e5139fe384431ac11c3b8a6cf4a529edf4a Mon Sep 17 00:00:00 2001
> From: Nikolay Borisov <nborisov@suse.com>
> Date: Tue, 20 Jun 2017 21:01:20 +0300
> 
> Currently, percpu_counter_add is a wrapper around __percpu_counter_add
> which is preempt safe due to explicit calls to preempt_disable.  Given
> how __ prefix is used in percpu related interfaces, the naming
> unfortunately creates the false sense that __percpu_counter_add is
> less safe than percpu_counter_add.  In terms of context-safety,
> they're equivalent.  The only difference is that the __ version takes
> a batch parameter.
> 
> Make this a bit more explicit by just renaming __percpu_counter_add to
> percpu_counter_add_batch.
> 
> This patch doesn't cause any functional changes.
> 
> tj: Minor updates to patch description for clarity.  Cosmetic
>     indentation updates.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[David Miller]

From: Tejun Heo <tj@kernel.org>
Date: Tue, 20 Jun 2017 15:47:59 -0400

> From 104b4e5139fe384431ac11c3b8a6cf4a529edf4a Mon Sep 17 00:00:00 2001
> From: Nikolay Borisov <nborisov@suse.com>
> Date: Tue, 20 Jun 2017 21:01:20 +0300
> 
> Currently, percpu_counter_add is a wrapper around __percpu_counter_add
> which is preempt safe due to explicit calls to preempt_disable.  Given
> how __ prefix is used in percpu related interfaces, the naming
> unfortunately creates the false sense that __percpu_counter_add is
> less safe than percpu_counter_add.  In terms of context-safety,
> they're equivalent.  The only difference is that the __ version takes
> a batch parameter.
> 
> Make this a bit more explicit by just renaming __percpu_counter_add to
> percpu_counter_add_batch.
> 
> This patch doesn't cause any functional changes.
> 
> tj: Minor updates to patch description for clarity.  Cosmetic
>     indentation updates.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>

Acked-by: David S. Miller <davem@davemloft.net>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] writeback: Rework wb_[dec|inc]_stat family of functions

[Nikolay Borisov]

On 20.06.2017 22:37, Tejun Heo wrote:
> Hello, Nikolay.
> 
> On Tue, Jun 20, 2017 at 09:02:00PM +0300, Nikolay Borisov wrote:
>> Currently the writeback statistics code uses a percpu counters to hold
>> various statistics. Furthermore we have 2 families of functions - those which
>> disable local irq and those which doesn't and whose names begin with
>> double underscore. However, they both end up calling __add_wb_stats which in
>> turn calls percpu_counter_add_batch which is already irq-safe.
> 
> Heh, looks like I was confused.  __percpu_counter_add() is not
> irq-safe.  It disables preemption and uses __this_cpu_read(), so
> there's no protection against irq.  If writeback statistics want
> irq-safe operations and it does, it would need these separate
> operations.  Am I missing something?

So looking at the history of the commit initially there was
preempt_disable + this_cpu_ptr which was later changed in:

819a72af8d66 ("percpucounter: Optimize __percpu_counter_add a bit
through the use of this_cpu() options.")


I believe that having __this_cpu_read ensures that we get an atomic
snapshot of the variable but when we are doing the actual write e.g. the
else {} branch we actually use this_cpu_add which ought to be preempt +
irq safe, meaning we won't get torn write. In essence we have atomic
reads by merit of __this_cpu_read + atomic writes by merit of using
raw_spin_lock_irqsave in the if() branch and this_cpu_add in the else {}
branch.

> 
> Thanks.
> 

Re: [PATCH v2 2/2] writeback: Rework wb_[dec|inc]_stat family of functions

[Tejun Heo]

Hello,

On Tue, Jun 20, 2017 at 11:28:30PM +0300, Nikolay Borisov wrote:
> > Heh, looks like I was confused.  __percpu_counter_add() is not
> > irq-safe.  It disables preemption and uses __this_cpu_read(), so
> > there's no protection against irq.  If writeback statistics want
> > irq-safe operations and it does, it would need these separate
> > operations.  Am I missing something?
> 
> So looking at the history of the commit initially there was
> preempt_disable + this_cpu_ptr which was later changed in:
> 
> 819a72af8d66 ("percpucounter: Optimize __percpu_counter_add a bit
> through the use of this_cpu() options.")
> 
> I believe that having __this_cpu_read ensures that we get an atomic
> snapshot of the variable but when we are doing the actual write e.g. the
> else {} branch we actually use this_cpu_add which ought to be preempt +
> irq safe, meaning we won't get torn write. In essence we have atomic
> reads by merit of __this_cpu_read + atomic writes by merit of using
> raw_spin_lock_irqsave in the if() branch and this_cpu_add in the else {}
> branch.

Ah, you're right.  The initial read is speculative.  The slow path is
protected with irq spinlock.  The fast path is this_cpu_add() which is
irq-safe.  We really need to document these functions.

Can I bother you with adding documentation to them while you're at it?

Thanks.

-- 
tejun

Re: [PATCH v2 2/2] writeback: Rework wb_[dec|inc]_stat family of functions

[Nikolay Borisov]

On 20.06.2017 23:30, Tejun Heo wrote:
> Hello,
> 
> On Tue, Jun 20, 2017 at 11:28:30PM +0300, Nikolay Borisov wrote:
>>> Heh, looks like I was confused.  __percpu_counter_add() is not
>>> irq-safe.  It disables preemption and uses __this_cpu_read(), so
>>> there's no protection against irq.  If writeback statistics want
>>> irq-safe operations and it does, it would need these separate
>>> operations.  Am I missing something?
>>
>> So looking at the history of the commit initially there was
>> preempt_disable + this_cpu_ptr which was later changed in:
>>
>> 819a72af8d66 ("percpucounter: Optimize __percpu_counter_add a bit
>> through the use of this_cpu() options.")
>>
>> I believe that having __this_cpu_read ensures that we get an atomic
>> snapshot of the variable but when we are doing the actual write e.g. the
>> else {} branch we actually use this_cpu_add which ought to be preempt +
>> irq safe, meaning we won't get torn write. In essence we have atomic
>> reads by merit of __this_cpu_read + atomic writes by merit of using
>> raw_spin_lock_irqsave in the if() branch and this_cpu_add in the else {}
>> branch.
> 
> Ah, you're right.  The initial read is speculative.  The slow path is
> protected with irq spinlock.  The fast path is this_cpu_add() which is
> irq-safe.  We really need to document these functions.
> 
> Can I bother you with adding documentation to them while you're at it?

Sure, I will likely resend with a fresh head on my shoulders.

> 
> Thanks.
> 

Re: [PATCH 2/2] writeback: Rework wb_[dec|inc]_stat family of functions

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

Hi Nikolay,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.12-rc6 next-20170620]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikolay-Borisov/percpu_counter-Rename-__percpu_counter_add-to-percpu_counter_add_batch/20170621-045738
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

All errors (new ones prefixed by >>):

   fs/fs-writeback.c: In function 'inode_switch_wbs_work_fn':
>> fs/fs-writeback.c:383:4: error: implicit declaration of function '__dec_wb_stat' [-Werror=implicit-function-declaration]
       __dec_wb_stat(old_wb, WB_RECLAIMABLE);
       ^~~~~~~~~~~~~
>> fs/fs-writeback.c:384:4: error: implicit declaration of function '__inc_wb_stat' [-Werror=implicit-function-declaration]
       __inc_wb_stat(new_wb, WB_RECLAIMABLE);
       ^~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/__dec_wb_stat +383 fs/fs-writeback.c

d10c8095 Tejun Heo 2015-05-28  377  	 */
d10c8095 Tejun Heo 2015-05-28  378  	radix_tree_for_each_tagged(slot, &mapping->page_tree, &iter, 0,
d10c8095 Tejun Heo 2015-05-28  379  				   PAGECACHE_TAG_DIRTY) {
d10c8095 Tejun Heo 2015-05-28  380  		struct page *page = radix_tree_deref_slot_protected(slot,
d10c8095 Tejun Heo 2015-05-28  381  							&mapping->tree_lock);
d10c8095 Tejun Heo 2015-05-28  382  		if (likely(page) && PageDirty(page)) {
d10c8095 Tejun Heo 2015-05-28 @383  			__dec_wb_stat(old_wb, WB_RECLAIMABLE);
d10c8095 Tejun Heo 2015-05-28 @384  			__inc_wb_stat(new_wb, WB_RECLAIMABLE);
d10c8095 Tejun Heo 2015-05-28  385  		}
d10c8095 Tejun Heo 2015-05-28  386  	}
d10c8095 Tejun Heo 2015-05-28  387  

:::::: The code at line 383 was first introduced by commit
:::::: d10c809552659d8a0089062b9d73da6d47e84cbf writeback: implement foreign cgroup inode bdi_writeback switching

:::::: TO: Tejun Heo <tj@kernel.org>
:::::: CC: Jens Axboe <axboe@fb.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50323 bytes --]

Re: [PATCH] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[Darrick J. Wong]

On Tue, Jun 20, 2017 at 03:47:59PM -0400, Tejun Heo wrote:
> From 104b4e5139fe384431ac11c3b8a6cf4a529edf4a Mon Sep 17 00:00:00 2001
> From: Nikolay Borisov <nborisov@suse.com>
> Date: Tue, 20 Jun 2017 21:01:20 +0300
> 
> Currently, percpu_counter_add is a wrapper around __percpu_counter_add
> which is preempt safe due to explicit calls to preempt_disable.  Given
> how __ prefix is used in percpu related interfaces, the naming
> unfortunately creates the false sense that __percpu_counter_add is
> less safe than percpu_counter_add.  In terms of context-safety,
> they're equivalent.  The only difference is that the __ version takes
> a batch parameter.
> 
> Make this a bit more explicit by just renaming __percpu_counter_add to
> percpu_counter_add_batch.
> 
> This patch doesn't cause any functional changes.
> 
> tj: Minor updates to patch description for clarity.  Cosmetic
>     indentation updates.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Chris Mason <clm@fb.com>
> Cc: Josef Bacik <jbacik@fb.com>
> Cc: David Sterba <dsterba@suse.com>
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: Jan Kara <jack@suse.com>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: linux-mm@kvack.org
> Cc: "David S. Miller" <davem@davemloft.net>
> ---
> Hello,
> 
> Applying this patch to percpu/for-4.13.  It's a pure rename patch.  If
> there's any objection, please let me know.
> 
> Thanks.
> 
>  fs/btrfs/disk-io.c             | 12 ++++++------
>  fs/btrfs/extent_io.c           |  6 +++---
>  fs/btrfs/inode.c               |  8 ++++----
>  fs/xfs/xfs_mount.c             |  4 ++--
>  include/linux/backing-dev.h    |  2 +-
>  include/linux/blk-cgroup.h     |  6 +++---
>  include/linux/mman.h           |  2 +-
>  include/linux/percpu_counter.h |  7 ++++---
>  include/net/inet_frag.h        |  4 ++--
>  lib/flex_proportions.c         |  6 +++---
>  lib/percpu_counter.c           |  4 ++--
>  11 files changed, 31 insertions(+), 30 deletions(-)
> 
<snip>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 2eaf81859166..7147d4a8d207 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1209,7 +1209,7 @@ xfs_mod_icount(
>  	struct xfs_mount	*mp,
>  	int64_t			delta)
>  {
> -	__percpu_counter_add(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
> +	percpu_counter_add_batch(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
>  	if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) {
>  		ASSERT(0);
>  		percpu_counter_add(&mp->m_icount, -delta);
> @@ -1288,7 +1288,7 @@ xfs_mod_fdblocks(
>  	else
>  		batch = XFS_FDBLOCKS_BATCH;
>  
> -	__percpu_counter_add(&mp->m_fdblocks, delta, batch);
> +	percpu_counter_add_batch(&mp->m_fdblocks, delta, batch);
>  	if (__percpu_counter_compare(&mp->m_fdblocks, mp->m_alloc_set_aside,
>  				     XFS_FDBLOCKS_BATCH) >= 0) {
>  		/* we had space! */

Straight rename looks ok to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 557d84063934..ace73f96eb1e 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -66,7 +66,7 @@ static inline bool bdi_has_dirty_io(struct backing_dev_info *bdi)
>  static inline void __add_wb_stat(struct bdi_writeback *wb,
>  				 enum wb_stat_item item, s64 amount)
>  {
> -	__percpu_counter_add(&wb->stat[item], amount, WB_STAT_BATCH);
> +	percpu_counter_add_batch(&wb->stat[item], amount, WB_STAT_BATCH);
>  }
>  
>  static inline void __inc_wb_stat(struct bdi_writeback *wb,
> diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
> index 01b62e7bac74..7104bea8dab1 100644
> --- a/include/linux/blk-cgroup.h
> +++ b/include/linux/blk-cgroup.h
> @@ -518,7 +518,7 @@ static inline void blkg_stat_exit(struct blkg_stat *stat)
>   */
>  static inline void blkg_stat_add(struct blkg_stat *stat, uint64_t val)
>  {
> -	__percpu_counter_add(&stat->cpu_cnt, val, BLKG_STAT_CPU_BATCH);
> +	percpu_counter_add_batch(&stat->cpu_cnt, val, BLKG_STAT_CPU_BATCH);
>  }
>  
>  /**
> @@ -597,14 +597,14 @@ static inline void blkg_rwstat_add(struct blkg_rwstat *rwstat,
>  	else
>  		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_READ];
>  
> -	__percpu_counter_add(cnt, val, BLKG_STAT_CPU_BATCH);
> +	percpu_counter_add_batch(cnt, val, BLKG_STAT_CPU_BATCH);
>  
>  	if (op_is_sync(op))
>  		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_SYNC];
>  	else
>  		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_ASYNC];
>  
> -	__percpu_counter_add(cnt, val, BLKG_STAT_CPU_BATCH);
> +	percpu_counter_add_batch(cnt, val, BLKG_STAT_CPU_BATCH);
>  }
>  
>  /**
> diff --git a/include/linux/mman.h b/include/linux/mman.h
> index 634c4c51fe3a..c8367041fafd 100644
> --- a/include/linux/mman.h
> +++ b/include/linux/mman.h
> @@ -22,7 +22,7 @@ unsigned long vm_memory_committed(void);
>  
>  static inline void vm_acct_memory(long pages)
>  {
> -	__percpu_counter_add(&vm_committed_as, pages, vm_committed_as_batch);
> +	percpu_counter_add_batch(&vm_committed_as, pages, vm_committed_as_batch);
>  }
>  
>  static inline void vm_unacct_memory(long pages)
> diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
> index 84a109449610..ec065387f443 100644
> --- a/include/linux/percpu_counter.h
> +++ b/include/linux/percpu_counter.h
> @@ -39,7 +39,8 @@ int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, gfp_t gfp,
>  
>  void percpu_counter_destroy(struct percpu_counter *fbc);
>  void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
> -void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch);
> +void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount,
> +			      s32 batch);
>  s64 __percpu_counter_sum(struct percpu_counter *fbc);
>  int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch);
>  
> @@ -50,7 +51,7 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs)
>  
>  static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount)
>  {
> -	__percpu_counter_add(fbc, amount, percpu_counter_batch);
> +	percpu_counter_add_batch(fbc, amount, percpu_counter_batch);
>  }
>  
>  static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)
> @@ -136,7 +137,7 @@ percpu_counter_add(struct percpu_counter *fbc, s64 amount)
>  }
>  
>  static inline void
> -__percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
> +percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
>  {
>  	percpu_counter_add(fbc, amount);
>  }
> diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
> index 5894730ec82a..5932e6de8fc0 100644
> --- a/include/net/inet_frag.h
> +++ b/include/net/inet_frag.h
> @@ -154,12 +154,12 @@ static inline int frag_mem_limit(struct netns_frags *nf)
>  
>  static inline void sub_frag_mem_limit(struct netns_frags *nf, int i)
>  {
> -	__percpu_counter_add(&nf->mem, -i, frag_percpu_counter_batch);
> +	percpu_counter_add_batch(&nf->mem, -i, frag_percpu_counter_batch);
>  }
>  
>  static inline void add_frag_mem_limit(struct netns_frags *nf, int i)
>  {
> -	__percpu_counter_add(&nf->mem, i, frag_percpu_counter_batch);
> +	percpu_counter_add_batch(&nf->mem, i, frag_percpu_counter_batch);
>  }
>  
>  static inline unsigned int sum_frag_mem_limit(struct netns_frags *nf)
> diff --git a/lib/flex_proportions.c b/lib/flex_proportions.c
> index a71cf1bdd4c9..2cc1f94e03a1 100644
> --- a/lib/flex_proportions.c
> +++ b/lib/flex_proportions.c
> @@ -207,7 +207,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
>  		if (val < (nr_cpu_ids * PROP_BATCH))
>  			val = percpu_counter_sum(&pl->events);
>  
> -		__percpu_counter_add(&pl->events,
> +		percpu_counter_add_batch(&pl->events,
>  			-val + (val >> (period-pl->period)), PROP_BATCH);
>  	} else
>  		percpu_counter_set(&pl->events, 0);
> @@ -219,7 +219,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
>  void __fprop_inc_percpu(struct fprop_global *p, struct fprop_local_percpu *pl)
>  {
>  	fprop_reflect_period_percpu(p, pl);
> -	__percpu_counter_add(&pl->events, 1, PROP_BATCH);
> +	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
>  	percpu_counter_add(&p->events, 1);
>  }
>  
> @@ -267,6 +267,6 @@ void __fprop_inc_percpu_max(struct fprop_global *p,
>  			return;
>  	} else
>  		fprop_reflect_period_percpu(p, pl);
> -	__percpu_counter_add(&pl->events, 1, PROP_BATCH);
> +	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
>  	percpu_counter_add(&p->events, 1);
>  }
> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> index 9c21000df0b5..8ee7e5ec21be 100644
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -72,7 +72,7 @@ void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
>  }
>  EXPORT_SYMBOL(percpu_counter_set);
>  
> -void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
> +void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
>  {
>  	s64 count;
>  
> @@ -89,7 +89,7 @@ void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
>  	}
>  	preempt_enable();
>  }
> -EXPORT_SYMBOL(__percpu_counter_add);
> +EXPORT_SYMBOL(percpu_counter_add_batch);
>  
>  /*
>   * Add up all the per-cpu counts, return the result.  This is a more accurate
> -- 
> 2.13.0
> 

Re: [PATCH] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[Darrick J. Wong]

On Tue, Jun 20, 2017 at 03:47:59PM -0400, Tejun Heo wrote:
> From 104b4e5139fe384431ac11c3b8a6cf4a529edf4a Mon Sep 17 00:00:00 2001
> From: Nikolay Borisov <nborisov@suse.com>
> Date: Tue, 20 Jun 2017 21:01:20 +0300
> 
> Currently, percpu_counter_add is a wrapper around __percpu_counter_add
> which is preempt safe due to explicit calls to preempt_disable.  Given
> how __ prefix is used in percpu related interfaces, the naming
> unfortunately creates the false sense that __percpu_counter_add is
> less safe than percpu_counter_add.  In terms of context-safety,
> they're equivalent.  The only difference is that the __ version takes
> a batch parameter.
> 
> Make this a bit more explicit by just renaming __percpu_counter_add to
> percpu_counter_add_batch.
> 
> This patch doesn't cause any functional changes.
> 
> tj: Minor updates to patch description for clarity.  Cosmetic
>     indentation updates.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Chris Mason <clm@fb.com>
> Cc: Josef Bacik <jbacik@fb.com>
> Cc: David Sterba <dsterba@suse.com>
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: Jan Kara <jack@suse.com>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: linux-mm@kvack.org
> Cc: "David S. Miller" <davem@davemloft.net>
> ---
> Hello,
> 
> Applying this patch to percpu/for-4.13.  It's a pure rename patch.  If
> there's any objection, please let me know.
> 
> Thanks.
> 
>  fs/btrfs/disk-io.c             | 12 ++++++------
>  fs/btrfs/extent_io.c           |  6 +++---
>  fs/btrfs/inode.c               |  8 ++++----
>  fs/xfs/xfs_mount.c             |  4 ++--
>  include/linux/backing-dev.h    |  2 +-
>  include/linux/blk-cgroup.h     |  6 +++---
>  include/linux/mman.h           |  2 +-
>  include/linux/percpu_counter.h |  7 ++++---
>  include/net/inet_frag.h        |  4 ++--
>  lib/flex_proportions.c         |  6 +++---
>  lib/percpu_counter.c           |  4 ++--
>  11 files changed, 31 insertions(+), 30 deletions(-)
> 
<snip>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 2eaf81859166..7147d4a8d207 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1209,7 +1209,7 @@ xfs_mod_icount(
>  	struct xfs_mount	*mp,
>  	int64_t			delta)
>  {
> -	__percpu_counter_add(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
> +	percpu_counter_add_batch(&mp->m_icount, delta, XFS_ICOUNT_BATCH);
>  	if (__percpu_counter_compare(&mp->m_icount, 0, XFS_ICOUNT_BATCH) < 0) {
>  		ASSERT(0);
>  		percpu_counter_add(&mp->m_icount, -delta);
> @@ -1288,7 +1288,7 @@ xfs_mod_fdblocks(
>  	else
>  		batch = XFS_FDBLOCKS_BATCH;
>  
> -	__percpu_counter_add(&mp->m_fdblocks, delta, batch);
> +	percpu_counter_add_batch(&mp->m_fdblocks, delta, batch);
>  	if (__percpu_counter_compare(&mp->m_fdblocks, mp->m_alloc_set_aside,
>  				     XFS_FDBLOCKS_BATCH) >= 0) {
>  		/* we had space! */

Straight rename looks ok to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 557d84063934..ace73f96eb1e 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -66,7 +66,7 @@ static inline bool bdi_has_dirty_io(struct backing_dev_info *bdi)
>  static inline void __add_wb_stat(struct bdi_writeback *wb,
>  				 enum wb_stat_item item, s64 amount)
>  {
> -	__percpu_counter_add(&wb->stat[item], amount, WB_STAT_BATCH);
> +	percpu_counter_add_batch(&wb->stat[item], amount, WB_STAT_BATCH);
>  }
>  
>  static inline void __inc_wb_stat(struct bdi_writeback *wb,
> diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
> index 01b62e7bac74..7104bea8dab1 100644
> --- a/include/linux/blk-cgroup.h
> +++ b/include/linux/blk-cgroup.h
> @@ -518,7 +518,7 @@ static inline void blkg_stat_exit(struct blkg_stat *stat)
>   */
>  static inline void blkg_stat_add(struct blkg_stat *stat, uint64_t val)
>  {
> -	__percpu_counter_add(&stat->cpu_cnt, val, BLKG_STAT_CPU_BATCH);
> +	percpu_counter_add_batch(&stat->cpu_cnt, val, BLKG_STAT_CPU_BATCH);
>  }
>  
>  /**
> @@ -597,14 +597,14 @@ static inline void blkg_rwstat_add(struct blkg_rwstat *rwstat,
>  	else
>  		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_READ];
>  
> -	__percpu_counter_add(cnt, val, BLKG_STAT_CPU_BATCH);
> +	percpu_counter_add_batch(cnt, val, BLKG_STAT_CPU_BATCH);
>  
>  	if (op_is_sync(op))
>  		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_SYNC];
>  	else
>  		cnt = &rwstat->cpu_cnt[BLKG_RWSTAT_ASYNC];
>  
> -	__percpu_counter_add(cnt, val, BLKG_STAT_CPU_BATCH);
> +	percpu_counter_add_batch(cnt, val, BLKG_STAT_CPU_BATCH);
>  }
>  
>  /**
> diff --git a/include/linux/mman.h b/include/linux/mman.h
> index 634c4c51fe3a..c8367041fafd 100644
> --- a/include/linux/mman.h
> +++ b/include/linux/mman.h
> @@ -22,7 +22,7 @@ unsigned long vm_memory_committed(void);
>  
>  static inline void vm_acct_memory(long pages)
>  {
> -	__percpu_counter_add(&vm_committed_as, pages, vm_committed_as_batch);
> +	percpu_counter_add_batch(&vm_committed_as, pages, vm_committed_as_batch);
>  }
>  
>  static inline void vm_unacct_memory(long pages)
> diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
> index 84a109449610..ec065387f443 100644
> --- a/include/linux/percpu_counter.h
> +++ b/include/linux/percpu_counter.h
> @@ -39,7 +39,8 @@ int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, gfp_t gfp,
>  
>  void percpu_counter_destroy(struct percpu_counter *fbc);
>  void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
> -void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch);
> +void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount,
> +			      s32 batch);
>  s64 __percpu_counter_sum(struct percpu_counter *fbc);
>  int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch);
>  
> @@ -50,7 +51,7 @@ static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs)
>  
>  static inline void percpu_counter_add(struct percpu_counter *fbc, s64 amount)
>  {
> -	__percpu_counter_add(fbc, amount, percpu_counter_batch);
> +	percpu_counter_add_batch(fbc, amount, percpu_counter_batch);
>  }
>  
>  static inline s64 percpu_counter_sum_positive(struct percpu_counter *fbc)
> @@ -136,7 +137,7 @@ percpu_counter_add(struct percpu_counter *fbc, s64 amount)
>  }
>  
>  static inline void
> -__percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
> +percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
>  {
>  	percpu_counter_add(fbc, amount);
>  }
> diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
> index 5894730ec82a..5932e6de8fc0 100644
> --- a/include/net/inet_frag.h
> +++ b/include/net/inet_frag.h
> @@ -154,12 +154,12 @@ static inline int frag_mem_limit(struct netns_frags *nf)
>  
>  static inline void sub_frag_mem_limit(struct netns_frags *nf, int i)
>  {
> -	__percpu_counter_add(&nf->mem, -i, frag_percpu_counter_batch);
> +	percpu_counter_add_batch(&nf->mem, -i, frag_percpu_counter_batch);
>  }
>  
>  static inline void add_frag_mem_limit(struct netns_frags *nf, int i)
>  {
> -	__percpu_counter_add(&nf->mem, i, frag_percpu_counter_batch);
> +	percpu_counter_add_batch(&nf->mem, i, frag_percpu_counter_batch);
>  }
>  
>  static inline unsigned int sum_frag_mem_limit(struct netns_frags *nf)
> diff --git a/lib/flex_proportions.c b/lib/flex_proportions.c
> index a71cf1bdd4c9..2cc1f94e03a1 100644
> --- a/lib/flex_proportions.c
> +++ b/lib/flex_proportions.c
> @@ -207,7 +207,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
>  		if (val < (nr_cpu_ids * PROP_BATCH))
>  			val = percpu_counter_sum(&pl->events);
>  
> -		__percpu_counter_add(&pl->events,
> +		percpu_counter_add_batch(&pl->events,
>  			-val + (val >> (period-pl->period)), PROP_BATCH);
>  	} else
>  		percpu_counter_set(&pl->events, 0);
> @@ -219,7 +219,7 @@ static void fprop_reflect_period_percpu(struct fprop_global *p,
>  void __fprop_inc_percpu(struct fprop_global *p, struct fprop_local_percpu *pl)
>  {
>  	fprop_reflect_period_percpu(p, pl);
> -	__percpu_counter_add(&pl->events, 1, PROP_BATCH);
> +	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
>  	percpu_counter_add(&p->events, 1);
>  }
>  
> @@ -267,6 +267,6 @@ void __fprop_inc_percpu_max(struct fprop_global *p,
>  			return;
>  	} else
>  		fprop_reflect_period_percpu(p, pl);
> -	__percpu_counter_add(&pl->events, 1, PROP_BATCH);
> +	percpu_counter_add_batch(&pl->events, 1, PROP_BATCH);
>  	percpu_counter_add(&p->events, 1);
>  }
> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> index 9c21000df0b5..8ee7e5ec21be 100644
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -72,7 +72,7 @@ void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
>  }
>  EXPORT_SYMBOL(percpu_counter_set);
>  
> -void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
> +void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
>  {
>  	s64 count;
>  
> @@ -89,7 +89,7 @@ void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
>  	}
>  	preempt_enable();
>  }
> -EXPORT_SYMBOL(__percpu_counter_add);
> +EXPORT_SYMBOL(percpu_counter_add_batch);
>  
>  /*
>   * Add up all the per-cpu counts, return the result.  This is a more accurate
> -- 
> 2.13.0
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v3] writeback: Rework wb_[dec|inc]_stat family of functions

[Nikolay Borisov]

Currently the writeback statistics code uses a percpu counters to hold
various statistics. Furthermore we have 2 families of functions - those which
disable local irq and those which doesn't and whose names begin with
double underscore. However, they both end up calling __add_wb_stats which in
turn calls percpu_counter_add_batch which is already irq-safe.

Exploiting this fact allows to eliminated the __wb_* functions since they don't
add any further protection than we already have. Furthermore, refactor
the wb_* function to call __add_wb_stat directly without the irq-disabling
dance. This will likely result in better runtime of code which deals with
modifying the stat counters.

While at it also document why percpu_counter_add_batch is in fact preempt and
irq-safe since at least 3 people got confused.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

Changes since v2: 
    * Fixed build failure reported by kbuild test robot
    * Explicitly document that percpu_counter_add_batch is preempt/irq safe
 fs/fs-writeback.c           |  8 ++++----
 include/linux/backing-dev.h | 24 ++----------------------
 lib/percpu_counter.c        |  7 +++++++
 mm/page-writeback.c         | 10 +++++-----
 4 files changed, 18 insertions(+), 31 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 63ee2940775c..309364aab2a5 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -380,8 +380,8 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
 		struct page *page = radix_tree_deref_slot_protected(slot,
 							&mapping->tree_lock);
 		if (likely(page) && PageDirty(page)) {
-			__dec_wb_stat(old_wb, WB_RECLAIMABLE);
-			__inc_wb_stat(new_wb, WB_RECLAIMABLE);
+			dec_wb_stat(old_wb, WB_RECLAIMABLE);
+			inc_wb_stat(new_wb, WB_RECLAIMABLE);
 		}
 	}
 
@@ -391,8 +391,8 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
 							&mapping->tree_lock);
 		if (likely(page)) {
 			WARN_ON_ONCE(!PageWriteback(page));
-			__dec_wb_stat(old_wb, WB_WRITEBACK);
-			__inc_wb_stat(new_wb, WB_WRITEBACK);
+			dec_wb_stat(old_wb, WB_WRITEBACK);
+			inc_wb_stat(new_wb, WB_WRITEBACK);
 		}
 	}
 
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index ace73f96eb1e..e9c967b86054 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -69,34 +69,14 @@ static inline void __add_wb_stat(struct bdi_writeback *wb,
 	percpu_counter_add_batch(&wb->stat[item], amount, WB_STAT_BATCH);
 }
 
-static inline void __inc_wb_stat(struct bdi_writeback *wb,
-				 enum wb_stat_item item)
-{
-	__add_wb_stat(wb, item, 1);
-}
-
 static inline void inc_wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__inc_wb_stat(wb, item);
-	local_irq_restore(flags);
-}
-
-static inline void __dec_wb_stat(struct bdi_writeback *wb,
-				 enum wb_stat_item item)
-{
-	__add_wb_stat(wb, item, -1);
+	__add_wb_stat(wb, item, 1);
 }
 
 static inline void dec_wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__dec_wb_stat(wb, item);
-	local_irq_restore(flags);
+	__add_wb_stat(wb, item, -1);
 }
 
 static inline s64 wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index 8ee7e5ec21be..3bf4a9984f4c 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -72,6 +72,13 @@ void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
 }
 EXPORT_SYMBOL(percpu_counter_set);
 
+/**
+ * This function is both preempt and irq safe. The former is due to explicit
+ * preemption disable. The latter is guaranteed by the fact that the slow path
+ * is explicitly protected by an irq-safe spinlock whereas the fast patch uses
+ * this_cpu_add which is irq-safe by definition. Hence there is no need muck
+ * with irq state before calling this one
+ */
 void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
 {
 	s64 count;
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 143c1c25d680..b7451891959a 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -601,7 +601,7 @@ static inline void __wb_writeout_inc(struct bdi_writeback *wb)
 {
 	struct wb_domain *cgdom;
 
-	__inc_wb_stat(wb, WB_WRITTEN);
+	inc_wb_stat(wb, WB_WRITTEN);
 	wb_domain_writeout_inc(&global_wb_domain, &wb->completions,
 			       wb->bdi->max_prop_frac);
 
@@ -2437,8 +2437,8 @@ void account_page_dirtied(struct page *page, struct address_space *mapping)
 		__inc_node_page_state(page, NR_FILE_DIRTY);
 		__inc_zone_page_state(page, NR_ZONE_WRITE_PENDING);
 		__inc_node_page_state(page, NR_DIRTIED);
-		__inc_wb_stat(wb, WB_RECLAIMABLE);
-		__inc_wb_stat(wb, WB_DIRTIED);
+		inc_wb_stat(wb, WB_RECLAIMABLE);
+		inc_wb_stat(wb, WB_DIRTIED);
 		task_io_account_write(PAGE_SIZE);
 		current->nr_dirtied++;
 		this_cpu_inc(bdp_ratelimits);
@@ -2745,7 +2745,7 @@ int test_clear_page_writeback(struct page *page)
 			if (bdi_cap_account_writeback(bdi)) {
 				struct bdi_writeback *wb = inode_to_wb(inode);
 
-				__dec_wb_stat(wb, WB_WRITEBACK);
+				dec_wb_stat(wb, WB_WRITEBACK);
 				__wb_writeout_inc(wb);
 			}
 		}
@@ -2791,7 +2791,7 @@ int __test_set_page_writeback(struct page *page, bool keep_write)
 						page_index(page),
 						PAGECACHE_TAG_WRITEBACK);
 			if (bdi_cap_account_writeback(bdi))
-				__inc_wb_stat(inode_to_wb(inode), WB_WRITEBACK);
+				inc_wb_stat(inode_to_wb(inode), WB_WRITEBACK);
 
 			/*
 			 * We can come through here when swapping anonymous
-- 
2.7.4

Re: [PATCH] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[David Sterba]

On Tue, Jun 20, 2017 at 03:47:59PM -0400, Tejun Heo wrote:
> From 104b4e5139fe384431ac11c3b8a6cf4a529edf4a Mon Sep 17 00:00:00 2001
> From: Nikolay Borisov <nborisov@suse.com>
> Date: Tue, 20 Jun 2017 21:01:20 +0300
> 
> Currently, percpu_counter_add is a wrapper around __percpu_counter_add
> which is preempt safe due to explicit calls to preempt_disable.  Given
> how __ prefix is used in percpu related interfaces, the naming
> unfortunately creates the false sense that __percpu_counter_add is
> less safe than percpu_counter_add.  In terms of context-safety,
> they're equivalent.  The only difference is that the __ version takes
> a batch parameter.
> 
> Make this a bit more explicit by just renaming __percpu_counter_add to
> percpu_counter_add_batch.
> 
> This patch doesn't cause any functional changes.
> 
> tj: Minor updates to patch description for clarity.  Cosmetic
>     indentation updates.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Chris Mason <clm@fb.com>
> Cc: Josef Bacik <jbacik@fb.com>
> Cc: David Sterba <dsterba@suse.com>

Acked-by: David Sterba <dsterba@suse.com>

Re: [PATCH] percpu_counter: Rename __percpu_counter_add to percpu_counter_add_batch

[David Sterba]

On Tue, Jun 20, 2017 at 03:47:59PM -0400, Tejun Heo wrote:
> From 104b4e5139fe384431ac11c3b8a6cf4a529edf4a Mon Sep 17 00:00:00 2001
> From: Nikolay Borisov <nborisov@suse.com>
> Date: Tue, 20 Jun 2017 21:01:20 +0300
> 
> Currently, percpu_counter_add is a wrapper around __percpu_counter_add
> which is preempt safe due to explicit calls to preempt_disable.  Given
> how __ prefix is used in percpu related interfaces, the naming
> unfortunately creates the false sense that __percpu_counter_add is
> less safe than percpu_counter_add.  In terms of context-safety,
> they're equivalent.  The only difference is that the __ version takes
> a batch parameter.
> 
> Make this a bit more explicit by just renaming __percpu_counter_add to
> percpu_counter_add_batch.
> 
> This patch doesn't cause any functional changes.
> 
> tj: Minor updates to patch description for clarity.  Cosmetic
>     indentation updates.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Chris Mason <clm@fb.com>
> Cc: Josef Bacik <jbacik@fb.com>
> Cc: David Sterba <dsterba@suse.com>

Acked-by: David Sterba <dsterba@suse.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3] writeback: Rework wb_[dec|inc]_stat family of functions

[Tejun Heo]

Hello,

cc'ing Andrew and Jan and cc'ing whole body.  The original patch
posting can be found at

 https://marc.info/?l=linux-kernel&m=149802995611259&q=raw

On Wed, Jun 21, 2017 at 10:25:37AM +0300, Nikolay Borisov wrote:
> Currently the writeback statistics code uses a percpu counters to hold
> various statistics. Furthermore we have 2 families of functions - those which
> disable local irq and those which doesn't and whose names begin with
> double underscore. However, they both end up calling __add_wb_stats which in
> turn calls percpu_counter_add_batch which is already irq-safe.
> 
> Exploiting this fact allows to eliminated the __wb_* functions since they don't
> add any further protection than we already have. Furthermore, refactor
> the wb_* function to call __add_wb_stat directly without the irq-disabling
> dance. This will likely result in better runtime of code which deals with
> modifying the stat counters.
> 
> While at it also document why percpu_counter_add_batch is in fact preempt and
> irq-safe since at least 3 people got confused.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

  Acked-by: Tejun Heo <tj@kernel.org>

Andrew, this looks good to me.  If you're okay with it, can you take
it through -mm?  If not, I can take it through percpu although that'd
be a bit of stretch.

Thanks.

> ---
> 
> Changes since v2: 
>     * Fixed build failure reported by kbuild test robot
>     * Explicitly document that percpu_counter_add_batch is preempt/irq safe
>  fs/fs-writeback.c           |  8 ++++----
>  include/linux/backing-dev.h | 24 ++----------------------
>  lib/percpu_counter.c        |  7 +++++++
>  mm/page-writeback.c         | 10 +++++-----
>  4 files changed, 18 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 63ee2940775c..309364aab2a5 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -380,8 +380,8 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
>  		struct page *page = radix_tree_deref_slot_protected(slot,
>  							&mapping->tree_lock);
>  		if (likely(page) && PageDirty(page)) {
> -			__dec_wb_stat(old_wb, WB_RECLAIMABLE);
> -			__inc_wb_stat(new_wb, WB_RECLAIMABLE);
> +			dec_wb_stat(old_wb, WB_RECLAIMABLE);
> +			inc_wb_stat(new_wb, WB_RECLAIMABLE);
>  		}
>  	}
>  
> @@ -391,8 +391,8 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
>  							&mapping->tree_lock);
>  		if (likely(page)) {
>  			WARN_ON_ONCE(!PageWriteback(page));
> -			__dec_wb_stat(old_wb, WB_WRITEBACK);
> -			__inc_wb_stat(new_wb, WB_WRITEBACK);
> +			dec_wb_stat(old_wb, WB_WRITEBACK);
> +			inc_wb_stat(new_wb, WB_WRITEBACK);
>  		}
>  	}
>  
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index ace73f96eb1e..e9c967b86054 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -69,34 +69,14 @@ static inline void __add_wb_stat(struct bdi_writeback *wb,
>  	percpu_counter_add_batch(&wb->stat[item], amount, WB_STAT_BATCH);
>  }
>  
> -static inline void __inc_wb_stat(struct bdi_writeback *wb,
> -				 enum wb_stat_item item)
> -{
> -	__add_wb_stat(wb, item, 1);
> -}
> -
>  static inline void inc_wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
>  {
> -	unsigned long flags;
> -
> -	local_irq_save(flags);
> -	__inc_wb_stat(wb, item);
> -	local_irq_restore(flags);
> -}
> -
> -static inline void __dec_wb_stat(struct bdi_writeback *wb,
> -				 enum wb_stat_item item)
> -{
> -	__add_wb_stat(wb, item, -1);
> +	__add_wb_stat(wb, item, 1);
>  }
>  
>  static inline void dec_wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
>  {
> -	unsigned long flags;
> -
> -	local_irq_save(flags);
> -	__dec_wb_stat(wb, item);
> -	local_irq_restore(flags);
> +	__add_wb_stat(wb, item, -1);
>  }
>  
>  static inline s64 wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> index 8ee7e5ec21be..3bf4a9984f4c 100644
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -72,6 +72,13 @@ void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
>  }
>  EXPORT_SYMBOL(percpu_counter_set);
>  
> +/**
> + * This function is both preempt and irq safe. The former is due to explicit
> + * preemption disable. The latter is guaranteed by the fact that the slow path
> + * is explicitly protected by an irq-safe spinlock whereas the fast patch uses
> + * this_cpu_add which is irq-safe by definition. Hence there is no need muck
> + * with irq state before calling this one
> + */
>  void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
>  {
>  	s64 count;
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 143c1c25d680..b7451891959a 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -601,7 +601,7 @@ static inline void __wb_writeout_inc(struct bdi_writeback *wb)
>  {
>  	struct wb_domain *cgdom;
>  
> -	__inc_wb_stat(wb, WB_WRITTEN);
> +	inc_wb_stat(wb, WB_WRITTEN);
>  	wb_domain_writeout_inc(&global_wb_domain, &wb->completions,
>  			       wb->bdi->max_prop_frac);
>  
> @@ -2437,8 +2437,8 @@ void account_page_dirtied(struct page *page, struct address_space *mapping)
>  		__inc_node_page_state(page, NR_FILE_DIRTY);
>  		__inc_zone_page_state(page, NR_ZONE_WRITE_PENDING);
>  		__inc_node_page_state(page, NR_DIRTIED);
> -		__inc_wb_stat(wb, WB_RECLAIMABLE);
> -		__inc_wb_stat(wb, WB_DIRTIED);
> +		inc_wb_stat(wb, WB_RECLAIMABLE);
> +		inc_wb_stat(wb, WB_DIRTIED);
>  		task_io_account_write(PAGE_SIZE);
>  		current->nr_dirtied++;
>  		this_cpu_inc(bdp_ratelimits);
> @@ -2745,7 +2745,7 @@ int test_clear_page_writeback(struct page *page)
>  			if (bdi_cap_account_writeback(bdi)) {
>  				struct bdi_writeback *wb = inode_to_wb(inode);
>  
> -				__dec_wb_stat(wb, WB_WRITEBACK);
> +				dec_wb_stat(wb, WB_WRITEBACK);
>  				__wb_writeout_inc(wb);
>  			}
>  		}
> @@ -2791,7 +2791,7 @@ int __test_set_page_writeback(struct page *page, bool keep_write)
>  						page_index(page),
>  						PAGECACHE_TAG_WRITEBACK);
>  			if (bdi_cap_account_writeback(bdi))
> -				__inc_wb_stat(inode_to_wb(inode), WB_WRITEBACK);
> +				inc_wb_stat(inode_to_wb(inode), WB_WRITEBACK);
>  
>  			/*
>  			 * We can come through here when swapping anonymous
> -- 
> 2.7.4
> 

-- 
tejun

Re: [PATCH v3] writeback: Rework wb_[dec|inc]_stat family of functions

[Jan Kara]

On Wed 21-06-17 11:59:41, Tejun Heo wrote:
> Hello,
> 
> cc'ing Andrew and Jan and cc'ing whole body.  The original patch
> posting can be found at
> 
>  https://marc.info/?l=linux-kernel&m=149802995611259&q=raw
> 
> On Wed, Jun 21, 2017 at 10:25:37AM +0300, Nikolay Borisov wrote:
> > Currently the writeback statistics code uses a percpu counters to hold
> > various statistics. Furthermore we have 2 families of functions - those which
> > disable local irq and those which doesn't and whose names begin with
> > double underscore. However, they both end up calling __add_wb_stats which in
> > turn calls percpu_counter_add_batch which is already irq-safe.
> > 
> > Exploiting this fact allows to eliminated the __wb_* functions since they don't
> > add any further protection than we already have. Furthermore, refactor
> > the wb_* function to call __add_wb_stat directly without the irq-disabling
> > dance. This will likely result in better runtime of code which deals with
> > modifying the stat counters.
> > 
> > While at it also document why percpu_counter_add_batch is in fact preempt and
> > irq-safe since at least 3 people got confused.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
>   Acked-by: Tejun Heo <tj@kernel.org>
> 
> Andrew, this looks good to me.  If you're okay with it, can you take
> it through -mm?  If not, I can take it through percpu although that'd
> be a bit of stretch.

The patch looks good to me as well. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> Thanks.
> 
> > ---
> > 
> > Changes since v2: 
> >     * Fixed build failure reported by kbuild test robot
> >     * Explicitly document that percpu_counter_add_batch is preempt/irq safe
> >  fs/fs-writeback.c           |  8 ++++----
> >  include/linux/backing-dev.h | 24 ++----------------------
> >  lib/percpu_counter.c        |  7 +++++++
> >  mm/page-writeback.c         | 10 +++++-----
> >  4 files changed, 18 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 63ee2940775c..309364aab2a5 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -380,8 +380,8 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
> >  		struct page *page = radix_tree_deref_slot_protected(slot,
> >  							&mapping->tree_lock);
> >  		if (likely(page) && PageDirty(page)) {
> > -			__dec_wb_stat(old_wb, WB_RECLAIMABLE);
> > -			__inc_wb_stat(new_wb, WB_RECLAIMABLE);
> > +			dec_wb_stat(old_wb, WB_RECLAIMABLE);
> > +			inc_wb_stat(new_wb, WB_RECLAIMABLE);
> >  		}
> >  	}
> >  
> > @@ -391,8 +391,8 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
> >  							&mapping->tree_lock);
> >  		if (likely(page)) {
> >  			WARN_ON_ONCE(!PageWriteback(page));
> > -			__dec_wb_stat(old_wb, WB_WRITEBACK);
> > -			__inc_wb_stat(new_wb, WB_WRITEBACK);
> > +			dec_wb_stat(old_wb, WB_WRITEBACK);
> > +			inc_wb_stat(new_wb, WB_WRITEBACK);
> >  		}
> >  	}
> >  
> > diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> > index ace73f96eb1e..e9c967b86054 100644
> > --- a/include/linux/backing-dev.h
> > +++ b/include/linux/backing-dev.h
> > @@ -69,34 +69,14 @@ static inline void __add_wb_stat(struct bdi_writeback *wb,
> >  	percpu_counter_add_batch(&wb->stat[item], amount, WB_STAT_BATCH);
> >  }
> >  
> > -static inline void __inc_wb_stat(struct bdi_writeback *wb,
> > -				 enum wb_stat_item item)
> > -{
> > -	__add_wb_stat(wb, item, 1);
> > -}
> > -
> >  static inline void inc_wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
> >  {
> > -	unsigned long flags;
> > -
> > -	local_irq_save(flags);
> > -	__inc_wb_stat(wb, item);
> > -	local_irq_restore(flags);
> > -}
> > -
> > -static inline void __dec_wb_stat(struct bdi_writeback *wb,
> > -				 enum wb_stat_item item)
> > -{
> > -	__add_wb_stat(wb, item, -1);
> > +	__add_wb_stat(wb, item, 1);
> >  }
> >  
> >  static inline void dec_wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
> >  {
> > -	unsigned long flags;
> > -
> > -	local_irq_save(flags);
> > -	__dec_wb_stat(wb, item);
> > -	local_irq_restore(flags);
> > +	__add_wb_stat(wb, item, -1);
> >  }
> >  
> >  static inline s64 wb_stat(struct bdi_writeback *wb, enum wb_stat_item item)
> > diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> > index 8ee7e5ec21be..3bf4a9984f4c 100644
> > --- a/lib/percpu_counter.c
> > +++ b/lib/percpu_counter.c
> > @@ -72,6 +72,13 @@ void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
> >  }
> >  EXPORT_SYMBOL(percpu_counter_set);
> >  
> > +/**
> > + * This function is both preempt and irq safe. The former is due to explicit
> > + * preemption disable. The latter is guaranteed by the fact that the slow path
> > + * is explicitly protected by an irq-safe spinlock whereas the fast patch uses
> > + * this_cpu_add which is irq-safe by definition. Hence there is no need muck
> > + * with irq state before calling this one
> > + */
> >  void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, s32 batch)
> >  {
> >  	s64 count;
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index 143c1c25d680..b7451891959a 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -601,7 +601,7 @@ static inline void __wb_writeout_inc(struct bdi_writeback *wb)
> >  {
> >  	struct wb_domain *cgdom;
> >  
> > -	__inc_wb_stat(wb, WB_WRITTEN);
> > +	inc_wb_stat(wb, WB_WRITTEN);
> >  	wb_domain_writeout_inc(&global_wb_domain, &wb->completions,
> >  			       wb->bdi->max_prop_frac);
> >  
> > @@ -2437,8 +2437,8 @@ void account_page_dirtied(struct page *page, struct address_space *mapping)
> >  		__inc_node_page_state(page, NR_FILE_DIRTY);
> >  		__inc_zone_page_state(page, NR_ZONE_WRITE_PENDING);
> >  		__inc_node_page_state(page, NR_DIRTIED);
> > -		__inc_wb_stat(wb, WB_RECLAIMABLE);
> > -		__inc_wb_stat(wb, WB_DIRTIED);
> > +		inc_wb_stat(wb, WB_RECLAIMABLE);
> > +		inc_wb_stat(wb, WB_DIRTIED);
> >  		task_io_account_write(PAGE_SIZE);
> >  		current->nr_dirtied++;
> >  		this_cpu_inc(bdp_ratelimits);
> > @@ -2745,7 +2745,7 @@ int test_clear_page_writeback(struct page *page)
> >  			if (bdi_cap_account_writeback(bdi)) {
> >  				struct bdi_writeback *wb = inode_to_wb(inode);
> >  
> > -				__dec_wb_stat(wb, WB_WRITEBACK);
> > +				dec_wb_stat(wb, WB_WRITEBACK);
> >  				__wb_writeout_inc(wb);
> >  			}
> >  		}
> > @@ -2791,7 +2791,7 @@ int __test_set_page_writeback(struct page *page, bool keep_write)
> >  						page_index(page),
> >  						PAGECACHE_TAG_WRITEBACK);
> >  			if (bdi_cap_account_writeback(bdi))
> > -				__inc_wb_stat(inode_to_wb(inode), WB_WRITEBACK);
> > +				inc_wb_stat(inode_to_wb(inode), WB_WRITEBACK);
> >  
> >  			/*
> >  			 * We can come through here when swapping anonymous
> > -- 
> > 2.7.4
> > 
> 
> -- 
> tejun
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR