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

Groups > linux.kernel > #1646751 > unrolled thread

[PATCH] mm: Define KB, MB, GB, TB in core VM

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm: Define KB, MB, GB, TB in core VM Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-05-22 13:20 +0200
    Re: [PATCH] mm: Define KB, MB, GB, TB in core VM Andrew Morton <akpm@linux-foundation.org> - 2017-05-22 23:20 +0200
      Re: [PATCH] mm: Define KB, MB, GB, TB in core VM Christoph Hellwig <hch@infradead.org> - 2017-05-23 09:10 +0200
        Re: [PATCH] mm: Define KB, MB, GB, TB in core VM Vlastimil Babka <vbabka@suse.cz> - 2017-05-23 10:40 +0200
          Re: [PATCH] mm: Define KB, MB, GB, TB in core VM Christoph Hellwig <hch@infradead.org> - 2017-05-23 10:50 +0200
          Re: [PATCH] mm: Define KB, MB, GB, TB in core VM Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-05-23 13:30 +0200
            Re: [PATCH] mm: Define KB, MB, GB, TB in core VM Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-05-24 08:50 +0200
              Re: [PATCH] mm: Define KB, MB, GB, TB in core VM Michal Hocko <mhocko@kernel.org> - 2017-05-24 16:40 +0200
      Re: [PATCH] mm: Define KB, MB, GB, TB in core VM Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-05-23 13:20 +0200
    Re: [PATCH] mm: Define KB, MB, GB, TB in core VM kbuild test robot <lkp@intel.com> - 2017-05-23 08:50 +0200

#1646751 — [PATCH] mm: Define KB, MB, GB, TB in core VM

There are many places where we define size either left shifting integers
or multiplying 1024s without any generic definition to fall back on. But
there are couples of (powerpc and lz4) attempts to define these standard
memory sizes. Lets move these definitions to core VM to make sure that
all new usage come from these definitions eventually standardizing it
across all places.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/mm/hash_utils_64.c | 4 ----
 include/linux/mm.h              | 5 +++++
 lib/lz4/lz4defs.h               | 5 +----
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index f2095ce..ef64040 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -74,10 +74,6 @@
 #define DBG_LOW(fmt...)
 #endif
 
-#define KB (1024)
-#define MB (1024*KB)
-#define GB (1024L*MB)
-
 /*
  * Note:  pte   --> Linux PTE
  *        HPTE  --> PowerPC Hashed Page Table Entry
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7cb17c6..9f5779f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2549,5 +2549,10 @@ static inline bool page_is_guard(struct page *page)
 static inline void setup_nr_node_ids(void) {}
 #endif
 
+#define KB (1UL << 10)
+#define MB (1UL << 20)
+#define GB (1UL << 30)
+#define TB (1UL << 40)
+
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */
diff --git a/lib/lz4/lz4defs.h b/lib/lz4/lz4defs.h
index 00a0b58..67a0f6d 100644
--- a/lib/lz4/lz4defs.h
+++ b/lib/lz4/lz4defs.h
@@ -37,6 +37,7 @@
 
 #include <asm/unaligned.h>
 #include <linux/string.h>	 /* memset, memcpy */
+#include <linux/mm.h>
 
 #define FORCE_INLINE __always_inline
 
@@ -81,10 +82,6 @@
 
 #define HASH_UNIT sizeof(size_t)
 
-#define KB (1 << 10)
-#define MB (1 << 20)
-#define GB (1U << 30)
-
 #define MAXD_LOG 16
 #define MAX_DISTANCE ((1 << MAXD_LOG) - 1)
 #define STEPSIZE sizeof(size_t)
-- 
1.8.5.2

[toc] | [next] | [standalone]

#1647399

On Mon, 22 May 2017 16:47:42 +0530 Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:

> There are many places where we define size either left shifting integers
> or multiplying 1024s without any generic definition to fall back on. But
> there are couples of (powerpc and lz4) attempts to define these standard
> memory sizes. Lets move these definitions to core VM to make sure that
> all new usage come from these definitions eventually standardizing it
> across all places.

Grep further - there are many more definitions and some may now
generate warnings.

Newly including mm.h for these things seems a bit heavyweight.  I can't
immediately think of a more appropriate place.  Maybe printk.h or
kernel.h.

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

#1647699

On Mon, May 22, 2017 at 02:11:49PM -0700, Andrew Morton wrote:
> On Mon, 22 May 2017 16:47:42 +0530 Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
> 
> > There are many places where we define size either left shifting integers
> > or multiplying 1024s without any generic definition to fall back on. But
> > there are couples of (powerpc and lz4) attempts to define these standard
> > memory sizes. Lets move these definitions to core VM to make sure that
> > all new usage come from these definitions eventually standardizing it
> > across all places.
> 
> Grep further - there are many more definitions and some may now
> generate warnings.
> 
> Newly including mm.h for these things seems a bit heavyweight.  I can't
> immediately think of a more appropriate place.  Maybe printk.h or
> kernel.h.

IFF we do these kernel.h is the right place.  And please also add the
MiB & co variants for the binary versions right next to the decimal
ones.

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

#1647801

On 05/23/2017 09:02 AM, Christoph Hellwig wrote:
> On Mon, May 22, 2017 at 02:11:49PM -0700, Andrew Morton wrote:
>> On Mon, 22 May 2017 16:47:42 +0530 Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
>>
>>> There are many places where we define size either left shifting integers
>>> or multiplying 1024s without any generic definition to fall back on. But
>>> there are couples of (powerpc and lz4) attempts to define these standard
>>> memory sizes. Lets move these definitions to core VM to make sure that
>>> all new usage come from these definitions eventually standardizing it
>>> across all places.
>>
>> Grep further - there are many more definitions and some may now
>> generate warnings.
>>
>> Newly including mm.h for these things seems a bit heavyweight.  I can't
>> immediately think of a more appropriate place.  Maybe printk.h or
>> kernel.h.
> 
> IFF we do these kernel.h is the right place.  And please also add the
> MiB & co variants for the binary versions right next to the decimal
> ones.

Those defined in the patch are binary, not decimal. Do we even need
decimal ones?

> --
> 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>
> 

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

#1647807

On Tue, May 23, 2017 at 10:38:17AM +0200, Vlastimil Babka wrote:
> Those defined in the patch are binary, not decimal. Do we even need
> decimal ones?

Oh, good point.  In which case the names should change to avoid the
confusion.

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

#1647952

On 05/23/2017 02:08 PM, Vlastimil Babka wrote:
> On 05/23/2017 09:02 AM, Christoph Hellwig wrote:
>> On Mon, May 22, 2017 at 02:11:49PM -0700, Andrew Morton wrote:
>>> On Mon, 22 May 2017 16:47:42 +0530 Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
>>>
>>>> There are many places where we define size either left shifting integers
>>>> or multiplying 1024s without any generic definition to fall back on. But
>>>> there are couples of (powerpc and lz4) attempts to define these standard
>>>> memory sizes. Lets move these definitions to core VM to make sure that
>>>> all new usage come from these definitions eventually standardizing it
>>>> across all places.
>>> Grep further - there are many more definitions and some may now
>>> generate warnings.
>>>
>>> Newly including mm.h for these things seems a bit heavyweight.  I can't
>>> immediately think of a more appropriate place.  Maybe printk.h or
>>> kernel.h.
>> IFF we do these kernel.h is the right place.  And please also add the
>> MiB & co variants for the binary versions right next to the decimal
>> ones.
> Those defined in the patch are binary, not decimal. Do we even need
> decimal ones?
> 

I can define KiB, MiB, .... with the same values as binary.
Did not get about the decimal ones, we need different names
for them holding values which are multiple of 1024 ?

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

#1649156

On 05/23/2017 04:49 PM, Anshuman Khandual wrote:
> On 05/23/2017 02:08 PM, Vlastimil Babka wrote:
>> On 05/23/2017 09:02 AM, Christoph Hellwig wrote:
>>> On Mon, May 22, 2017 at 02:11:49PM -0700, Andrew Morton wrote:
>>>> On Mon, 22 May 2017 16:47:42 +0530 Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
>>>>
>>>>> There are many places where we define size either left shifting integers
>>>>> or multiplying 1024s without any generic definition to fall back on. But
>>>>> there are couples of (powerpc and lz4) attempts to define these standard
>>>>> memory sizes. Lets move these definitions to core VM to make sure that
>>>>> all new usage come from these definitions eventually standardizing it
>>>>> across all places.
>>>> Grep further - there are many more definitions and some may now
>>>> generate warnings.
>>>>
>>>> Newly including mm.h for these things seems a bit heavyweight.  I can't
>>>> immediately think of a more appropriate place.  Maybe printk.h or
>>>> kernel.h.
>>> IFF we do these kernel.h is the right place.  And please also add the
>>> MiB & co variants for the binary versions right next to the decimal
>>> ones.
>> Those defined in the patch are binary, not decimal. Do we even need
>> decimal ones?
>>
> 
> I can define KiB, MiB, .... with the same values as binary.
> Did not get about the decimal ones, we need different names
> for them holding values which are multiple of 1024 ?

Now it seems little bit complicated than I initially thought.
There are three different kind of definitions scattered across
the tree.

(1) Constant defines like these which can be unified across
    with little effort.

+#define KB (1UL << 10)
+#define MB (1UL << 20)
+#define GB (1UL << 30)
+#define TB (1UL << 40)

(2) Function type defines like these which need to be renamed
    first because of the static defines already added above.

#define KB(x) ((x) * 1024)
#define MB(x) (KB(x) * 1024)

    Does these sound good as a rename ?

+#define KBN(x) ((x) * KB)
+#define MBN(x) ((x) * MB)
+#define GBN(x) ((x) * GB)
+#define TBN(x) ((x) * TB)

    And these need to be replaced across the tree.

(3) Then there are many defines for MB, KB, GB which have nothing
    to do with memory size and they need to be changed as well to
    something else more appropriately to something they actually
    do.

#define MB CRB

* Defined inside arch/powerpc/xmon/ppc-opc.c

#define GB(p,n,s) gf2k_get_bits(data, p, n, s)

* Defined inside drivers/input/joystick/gf2k.c

#define GB(pos,num) sw_get_bits(buf, pos, num, sw->bits)

* Defined inside drivers/input/joystick/sidewinder.c 

So the question is are we willing to do all these changes across
the tree to achieve common definitions of KB, MB, GB, TB in the
kernel ? Is it worth ?

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

#1649674

On Wed 24-05-17 12:10:13, Anshuman Khandual wrote:
[...]
> So the question is are we willing to do all these changes across
> the tree to achieve common definitions of KB, MB, GB, TB in the
> kernel ? Is it worth ?

I do not think this is worth losing time. Any tree wide change should
have a considerable advantage in the end. These macro helpers do not
sound overly important to care. But that is just my 2c
-- 
Michal Hocko
SUSE Labs

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

#1647943

On 05/23/2017 02:41 AM, Andrew Morton wrote:
> On Mon, 22 May 2017 16:47:42 +0530 Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
> 
>> There are many places where we define size either left shifting integers
>> or multiplying 1024s without any generic definition to fall back on. But
>> there are couples of (powerpc and lz4) attempts to define these standard
>> memory sizes. Lets move these definitions to core VM to make sure that
>> all new usage come from these definitions eventually standardizing it
>> across all places.
> Grep further - there are many more definitions and some may now
> generate warnings.

Yeah, warning reports started coming in. Will try to change
all of those to follow the new definitions added.

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

#1647684

[Multipart message — attachments visible in raw view] — view raw

Hi Anshuman,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.12-rc2 next-20170522]
[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/Anshuman-Khandual/mm-Define-KB-MB-GB-TB-in-core-VM/20170523-141359
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

>> arch/x86/kernel/cpu/intel_cacheinfo.c:34:0: warning: "MB" redefined
    #define MB(x) ((x) * 1024)
    
   In file included from arch/x86/include/asm/pci.h:4:0,
                    from include/linux/pci.h:1618,
                    from arch/x86/kernel/cpu/intel_cacheinfo.c:16:
   include/linux/mm.h:2553:0: note: this is the location of the previous definition
    #define MB (1UL << 20)
    

vim +/MB +34 arch/x86/kernel/cpu/intel_cacheinfo.c

cd4d09ec arch/x86/kernel/cpu/intel_cacheinfo.c  Borislav Petkov  2016-01-26  18  #include <asm/cpufeature.h>
23ac4ae8 arch/x86/kernel/cpu/intel_cacheinfo.c  Andreas Herrmann 2010-09-17  19  #include <asm/amd_nb.h>
dcf39daf arch/x86/kernel/cpu/intel_cacheinfo.c  Borislav Petkov  2010-01-22  20  #include <asm/smp.h>
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  21  
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  22  #define LVL_1_INST	1
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  23  #define LVL_1_DATA	2
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  24  #define LVL_2		3
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  25  #define LVL_3		4
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  26  #define LVL_TRACE	5
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  27  
8bdbd962 arch/x86/kernel/cpu/intel_cacheinfo.c  Alan Cox         2009-07-04  28  struct _cache_table {
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  29  	unsigned char descriptor;
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  30  	char cache_type;
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  31  	short size;
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  32  };
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  33  
2ca49b2f arch/x86/kernel/cpu/intel_cacheinfo.c  Dave Jones       2010-01-04 @34  #define MB(x)	((x) * 1024)
2ca49b2f arch/x86/kernel/cpu/intel_cacheinfo.c  Dave Jones       2010-01-04  35  
8bdbd962 arch/x86/kernel/cpu/intel_cacheinfo.c  Alan Cox         2009-07-04  36  /* All the cache descriptor types we care about (no TLB or
8bdbd962 arch/x86/kernel/cpu/intel_cacheinfo.c  Alan Cox         2009-07-04  37     trace cache entries) */
8bdbd962 arch/x86/kernel/cpu/intel_cacheinfo.c  Alan Cox         2009-07-04  38  
148f9bb8 arch/x86/kernel/cpu/intel_cacheinfo.c  Paul Gortmaker   2013-06-18  39  static const struct _cache_table cache_table[] =
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  40  {
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  41  	{ 0x06, LVL_1_INST, 8 },	/* 4-way set assoc, 32 byte line size */
^1da177e arch/i386/kernel/cpu/intel_cacheinfo.c Linus Torvalds   2005-04-16  42  	{ 0x08, LVL_1_INST, 16 },	/* 4-way set assoc, 32 byte line size */

:::::: The code at line 34 was first introduced by commit
:::::: 2ca49b2fcf5813571663c3c4c894b78148c43690 x86: Macroise x86 cache descriptors

:::::: TO: Dave Jones <davej@redhat.com>
:::::: CC: Ingo Molnar <mingo@elte.hu>

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web