[RFC PATCH 0/1] mm: provide one common K(x) macro

[RFC PATCH 0/1] mm: provide one common K(x) macro

[Oleksandr Natalenko]

Based on discussion with Miaohe Lin [1].

There are various places where the K(x) macro is defined, sometimes
under different names, sometimes it is just open-coded. In this RFC I
suggest providing a common K(x) definition that replaces existing ones.

For that, a new header, mm_tools.h, is created. I couldn't find a more
appropriate place for K(x). Probably, an existing header like mm_inline.h
would work as well. I tried placing this macro under mm.h, but then it
conflicts with uapi/linux/keyboard.h.

In case this approach is acceptable, replacing open-coded variants would
be done in a separate patch. For now, I'm using `git grep -nE
'<<\s?\(PAGE_SHIFT\s?\-\s?10\)'` to find such places.

Also note, here I do not touch files under arch/ like
arch/arc/include/asm/arcregs.h where PAGES_TO_KB() is defined, or
arch/powerpc/platforms/pseries/cmm.c with PAGES2KB() as well as
arch/s390/appldata/appldata_mem.c with P2K() as I'm not sure if it is
appropriate to include another header file there.

The patch is based on top of next-20210831 and is compile-tested using
allyesconfig.

Please let me know what you think.

Thanks.

[1] https://lore.kernel.org/linux-mm/9161665.bUqNH3lxUD@natalenko.name/

Oleksandr Natalenko (1):
  mm: provide one common K(x) macro

 drivers/base/node.c                 |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_base.c |  5 ++---
 drivers/xen/xen-balloon.c           | 11 +++++------
 include/linux/mm_tools.h            |  9 +++++++++
 include/trace/events/writeback.h    | 19 +++++++++----------
 kernel/debug/kdb/kdb_main.c         |  2 +-
 mm/backing-dev.c                    |  3 +--
 mm/memcontrol.c                     |  2 +-
 mm/oom_kill.c                       |  3 +--
 mm/page_alloc.c                     |  3 +--
 10 files changed, 31 insertions(+), 28 deletions(-)
 create mode 100644 include/linux/mm_tools.h

-- 
2.33.0

[RFC PATCH 1/1] mm: provide one common K(x) macro

[Oleksandr Natalenko]

There are various places where the K(x) macro is defined. This commit
gets rid of multiple definitions and provides a common one.

This doesn't solve open-coding this macro in various other places. This
should be addressed by another subsequent commit.

Signed-off-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
 drivers/base/node.c                 |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_base.c |  5 ++---
 drivers/xen/xen-balloon.c           | 11 +++++------
 include/linux/mm_tools.h            |  9 +++++++++
 include/trace/events/writeback.h    | 19 +++++++++----------
 kernel/debug/kdb/kdb_main.c         |  2 +-
 mm/backing-dev.c                    |  3 +--
 mm/memcontrol.c                     |  2 +-
 mm/oom_kill.c                       |  3 +--
 mm/page_alloc.c                     |  3 +--
 10 files changed, 31 insertions(+), 28 deletions(-)
 create mode 100644 include/linux/mm_tools.h

diff --git a/drivers/base/node.c b/drivers/base/node.c
index c56d34f8158f..0f6be7750e60 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -6,6 +6,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/mm.h>
+#include <linux/mm_tools.h>
 #include <linux/memory.h>
 #include <linux/vmstat.h>
 #include <linux/notifier.h>
@@ -365,7 +366,6 @@ static void node_init_caches(unsigned int nid) { }
 static void node_remove_caches(struct node *node) { }
 #endif
 
-#define K(x) ((x) << (PAGE_SHIFT - 10))
 static ssize_t node_read_meminfo(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 6c82435bc9cc..a822952d8b1a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -61,6 +61,7 @@
 #include <linux/kthread.h>
 #include <asm/page.h>        /* To get host page size per arch */
 #include <linux/aer.h>
+#include <linux/mm_tools.h>
 
 
 #include "mpt3sas_base.h"
@@ -2986,8 +2987,6 @@ _base_build_sg_ieee(struct MPT3SAS_ADAPTER *ioc, void *psge,
 	}
 }
 
-#define convert_to_kb(x) ((x) << (PAGE_SHIFT - 10))
-
 /**
  * _base_config_dma_addressing - set dma addressing
  * @ioc: per adapter object
@@ -3024,7 +3023,7 @@ _base_config_dma_addressing(struct MPT3SAS_ADAPTER *ioc, struct pci_dev *pdev)
 
 	si_meminfo(&s);
 	ioc_info(ioc, "%d BIT PCI BUS DMA ADDRESSING SUPPORTED, total mem (%ld kB)\n",
-		ioc->dma_mask, convert_to_kb(s.totalram));
+		ioc->dma_mask, K(s.totalram));
 
 	return 0;
 }
diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
index 8cd583db20b1..d295a6f132f6 100644
--- a/drivers/xen/xen-balloon.c
+++ b/drivers/xen/xen-balloon.c
@@ -34,6 +34,7 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/mm_tools.h>
 #include <linux/mm_types.h>
 #include <linux/init.h>
 #include <linux/capability.h>
@@ -47,8 +48,6 @@
 #include <xen/page.h>
 #include <xen/mem-reservation.h>
 
-#define PAGES2KB(_p) ((_p)<<(PAGE_SHIFT-10))
-
 #define BALLOON_CLASS_NAME "xen_memory"
 
 #ifdef CONFIG_MEMORY_HOTPLUG
@@ -142,9 +141,9 @@ EXPORT_SYMBOL_GPL(xen_balloon_init);
 	}								\
 	static DEVICE_ATTR_RO(name)
 
-BALLOON_SHOW(current_kb, "%lu\n", PAGES2KB(balloon_stats.current_pages));
-BALLOON_SHOW(low_kb, "%lu\n", PAGES2KB(balloon_stats.balloon_low));
-BALLOON_SHOW(high_kb, "%lu\n", PAGES2KB(balloon_stats.balloon_high));
+BALLOON_SHOW(current_kb, "%lu\n", K(balloon_stats.current_pages));
+BALLOON_SHOW(low_kb, "%lu\n", K(balloon_stats.balloon_low));
+BALLOON_SHOW(high_kb, "%lu\n", K(balloon_stats.balloon_high));
 
 static DEVICE_ULONG_ATTR(schedule_delay, 0444, balloon_stats.schedule_delay);
 static DEVICE_ULONG_ATTR(max_schedule_delay, 0644, balloon_stats.max_schedule_delay);
@@ -155,7 +154,7 @@ static DEVICE_BOOL_ATTR(scrub_pages, 0644, xen_scrub_pages);
 static ssize_t target_kb_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
-	return sprintf(buf, "%lu\n", PAGES2KB(balloon_stats.target_pages));
+	return sprintf(buf, "%lu\n", K(balloon_stats.target_pages));
 }
 
 static ssize_t target_kb_store(struct device *dev,
diff --git a/include/linux/mm_tools.h b/include/linux/mm_tools.h
new file mode 100644
index 000000000000..fcee439bc8db
--- /dev/null
+++ b/include/linux/mm_tools.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_MM_TOOLS_H
+#define _LINUX_MM_TOOLS_H
+
+#include <asm/page.h>
+
+#define K(x)	((x) << (PAGE_SHIFT - 10))
+
+#endif /* _LINUX_MM_TOOLS_H */
diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index 7dccb66474f7..ee3ccfacc3c1 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -8,6 +8,7 @@
 #include <linux/tracepoint.h>
 #include <linux/backing-dev.h>
 #include <linux/writeback.h>
+#include <linux/mm_tools.h>
 
 #define show_inode_state(state)					\
 	__print_flags(state, "|",				\
@@ -570,8 +571,6 @@ TRACE_EVENT(global_dirty_state,
 	)
 );
 
-#define KBps(x)			((x) << (PAGE_SHIFT - 10))
-
 TRACE_EVENT(bdi_dirty_ratelimit,
 
 	TP_PROTO(struct bdi_writeback *wb,
@@ -593,13 +592,13 @@ TRACE_EVENT(bdi_dirty_ratelimit,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->bdi, bdi_dev_name(wb->bdi), 32);
-		__entry->write_bw	= KBps(wb->write_bandwidth);
-		__entry->avg_write_bw	= KBps(wb->avg_write_bandwidth);
-		__entry->dirty_rate	= KBps(dirty_rate);
-		__entry->dirty_ratelimit = KBps(wb->dirty_ratelimit);
-		__entry->task_ratelimit	= KBps(task_ratelimit);
+		__entry->write_bw	= K(wb->write_bandwidth);
+		__entry->avg_write_bw	= K(wb->avg_write_bandwidth);
+		__entry->dirty_rate	= K(dirty_rate);
+		__entry->dirty_ratelimit = K(wb->dirty_ratelimit);
+		__entry->task_ratelimit	= K(task_ratelimit);
 		__entry->balanced_dirty_ratelimit =
-					KBps(wb->balanced_dirty_ratelimit);
+					K(wb->balanced_dirty_ratelimit);
 		__entry->cgroup_ino	= __trace_wb_assign_cgroup(wb);
 	),
 
@@ -666,8 +665,8 @@ TRACE_EVENT(balance_dirty_pages,
 		__entry->bdi_setpoint	= __entry->setpoint *
 						bdi_thresh / (thresh + 1);
 		__entry->bdi_dirty	= bdi_dirty;
-		__entry->dirty_ratelimit = KBps(dirty_ratelimit);
-		__entry->task_ratelimit	= KBps(task_ratelimit);
+		__entry->dirty_ratelimit = K(dirty_ratelimit);
+		__entry->task_ratelimit	= K(task_ratelimit);
 		__entry->dirtied	= dirtied;
 		__entry->dirtied_pause	= current->nr_dirtied_pause;
 		__entry->think		= current->dirty_paused_when == 0 ? 0 :
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index fa6deda894a1..10faf224c55a 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -29,6 +29,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/mm.h>
+#include <linux/mm_tools.h>
 #include <linux/init.h>
 #include <linux/kallsyms.h>
 #include <linux/kgdb.h>
@@ -2522,7 +2523,6 @@ static int kdb_summary(int argc, const char **argv)
 		LOAD_INT(val.loads[2]), LOAD_FRAC(val.loads[2]));
 
 	/* Display in kilobytes */
-#define K(x) ((x) << (PAGE_SHIFT - 10))
 	kdb_printf("\nMemTotal:       %8lu kB\nMemFree:        %8lu kB\n"
 		   "Buffers:        %8lu kB\n",
 		   K(val.totalram), K(val.freeram), K(val.bufferram));
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 4a9d4e27d0d9..0caf624ec99f 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -8,6 +8,7 @@
 #include <linux/fs.h>
 #include <linux/pagemap.h>
 #include <linux/mm.h>
+#include <linux/mm_tools.h>
 #include <linux/sched/mm.h>
 #include <linux/sched.h>
 #include <linux/module.h>
@@ -33,8 +34,6 @@ LIST_HEAD(bdi_list);
 /* bdi_wq serves all asynchronous writeback tasks */
 struct workqueue_struct *bdi_wq;
 
-#define K(x) ((x) << (PAGE_SHIFT - 10))
-
 #ifdef CONFIG_DEBUG_FS
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 999e626f4111..1164eb3e6d40 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -54,6 +54,7 @@
 #include <linux/seq_file.h>
 #include <linux/vmpressure.h>
 #include <linux/mm_inline.h>
+#include <linux/mm_tools.h>
 #include <linux/swap_cgroup.h>
 #include <linux/cpu.h>
 #include <linux/oom.h>
@@ -1471,7 +1472,6 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
 	return s.buffer;
 }
 
-#define K(x) ((x) << (PAGE_SHIFT-10))
 /**
  * mem_cgroup_print_oom_context: Print OOM information relevant to
  * memory controller.
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 431d38c3bba8..c1ba2051d3cf 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -20,6 +20,7 @@
 
 #include <linux/oom.h>
 #include <linux/mm.h>
+#include <linux/mm_tools.h>
 #include <linux/err.h>
 #include <linux/gfp.h>
 #include <linux/sched.h>
@@ -485,8 +486,6 @@ static DECLARE_WAIT_QUEUE_HEAD(oom_victims_wait);
 
 static bool oom_killer_disabled __read_mostly;
 
-#define K(x) ((x) << (PAGE_SHIFT-10))
-
 /*
  * task->mm can be NULL if the task is the exited group leader.  So to
  * determine whether the task is using a particular mm, we examine all the
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 91edb930b8ab..9cca67008ed8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -17,6 +17,7 @@
 
 #include <linux/stddef.h>
 #include <linux/mm.h>
+#include <linux/mm_tools.h>
 #include <linux/highmem.h>
 #include <linux/swap.h>
 #include <linux/interrupt.h>
@@ -5840,8 +5841,6 @@ static bool show_mem_node_skip(unsigned int flags, int nid, nodemask_t *nodemask
 	return !node_isset(nid, *nodemask);
 }
 
-#define K(x) ((x) << (PAGE_SHIFT-10))
-
 static void show_migration_types(unsigned char type)
 {
 	static const char types[MIGRATE_TYPES] = {
-- 
2.33.0

Re: [RFC PATCH 1/1] mm: provide one common K(x) macro

[Michal Hocko]

On Wed 01-09-21 11:21:49, Oleksandr Natalenko wrote:
> There are various places where the K(x) macro is defined. This commit
> gets rid of multiple definitions and provides a common one.
> 
> This doesn't solve open-coding this macro in various other places. This
> should be addressed by another subsequent commit.

Why is this an improvement? You are adding a header file for a single
macro which sounds like an overkill. The overall net outcome is added
lines of code. It is not like K() or any of its variant is adding a
maintenance burden due to code duplication. So why do we want to change
the existing state?
-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH 1/1] mm: provide one common K(x) macro

[Oleksandr Natalenko]

Hello.

On středa 1. září 2021 12:31:36 CEST Michal Hocko wrote:
> On Wed 01-09-21 11:21:49, Oleksandr Natalenko wrote:
> > There are various places where the K(x) macro is defined. This commit
> > gets rid of multiple definitions and provides a common one.
> > 
> > This doesn't solve open-coding this macro in various other places. This
> > should be addressed by another subsequent commit.
> 
> Why is this an improvement? You are adding a header file for a single
> macro which sounds like an overkill.

I agree a separate header file is an overkill for just one #define, hence
still looking for a suggestion on a better place for it.

> The overall net outcome is added
> lines of code.

Not always. There are some long statements like:

```
seq_printf(seq, ",size=%luk",
        sbinfo->max_blocks << (PAGE_SHIFT - 10));
```

that are split into two lines. With the macro those take one line only:

```
seq_printf(seq, ",size=%luk", K(sbinfo->max_blocks));
```

As of now (counting unposted open-coding replacements) the grand total is:

```
31 files changed, 104 insertions(+), 90 deletions(-)
```

which is not that horrible.

> It is not like K() or any of its variant is adding a
> maintenance burden due to code duplication. So why do we want to change
> the existing state?

For me it's about readability. Compare, for instance:

```
seq_put_decimal_ull_width(m, str, (val) << (PAGE_SHIFT-10), 8)
```

and

```
seq_put_decimal_ull_width(m, str, K(val), 8)
```

It's a small yet visible difference.

Thanks.

-- 
Oleksandr Natalenko (post-factum)

Re: [RFC PATCH 1/1] mm: provide one common K(x) macro

[Michal Hocko]

On Wed 01-09-21 12:50:40, Oleksandr Natalenko wrote:
[...]
> ```
> 31 files changed, 104 insertions(+), 90 deletions(-)
> ```
> 
> which is not that horrible.

Still a lot of churn to my taste for something that is likely a matter
of personal preferences and taste. Consider additional costs as well.
E.g. go over additional git blame steps to learn why the code has been
introduced, review bandwith etc...

And just be clear, I am not really opposing this patch I just do not see
a justification and in general I am not super thrilled about cleanups
which are not really necessary for a bigger goal - exactly because of
the additional costs mentioned above.
-- 
Michal Hocko
SUSE Labs

RE: [RFC PATCH 1/1] mm: provide one common K(x) macro

[David Laight]

From: Michal Hocko
> Sent: 01 September 2021 12:11
> 
> On Wed 01-09-21 12:50:40, Oleksandr Natalenko wrote:
> [...]
> > ```
> > 31 files changed, 104 insertions(+), 90 deletions(-)
> > ```
> >
> > which is not that horrible.
> 
> Still a lot of churn to my taste for something that is likely a matter
> of personal preferences and taste. Consider additional costs as well.
> E.g. go over additional git blame steps to learn why the code has been
> introduced, review bandwith etc...

Not to mention the time taken by someone scan-reading the
code who has to go and find the definition of K() just
to see what it does.

A more descriptive name (eg PAGES_TO_KB) might save that.
but is it worth it?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)