Re: [PATCH v2 3/7] mm/memory_hotplug: prepare passing flags to add_memory() and friends
On 09.09.20 13:37, David Hildenbrand wrote: > On 09.09.20 13:24, Michael Ellerman wrote: >> David Hildenbrand writes: >>> On 09.09.20 09:17, Greg Kroah-Hartman wrote: On Tue, Sep 08, 2020 at 10:10:08PM +0200, David Hildenbrand wrote: > We soon want to pass flags, e.g., to mark added System RAM resources. > mergeable. Prepare for that. What are these random "flags", and how do we know what should be passed to them? Why not make this an enumerated type so that we know it all works properly, like the GPF_* flags are? Passing around a random unsigned long feels very odd/broken... >>> >>> Agreed, an enum (mhp_flags) seems to give a better hint what can >>> actually be passed. Thanks! >> >> You probably know this but ... >> >> Just using a C enum doesn't get you any type safety. >> >> You can get some checking via sparse by using __bitwise, which is what >> gfp_t does. You don't actually have to use an enum for that, it works >> with #defines also. > > Yeah, we seem to be using different approaches. And there is always a > way to mess things up :) > > gfp_t is one (extreme) example, enum memblock_flags is another example. > I tend to prefer an enum in this particular case, because it's simple > and at least tells the user which values are expected. > Gave it another try, looks like mhp_t (like gfp_t) is actually nicer. -- Thanks, David / dhildenb ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/7] mm/memory_hotplug: prepare passing flags to add_memory() and friends
On 09.09.20 13:24, Michael Ellerman wrote: > David Hildenbrand writes: >> On 09.09.20 09:17, Greg Kroah-Hartman wrote: >>> On Tue, Sep 08, 2020 at 10:10:08PM +0200, David Hildenbrand wrote: We soon want to pass flags, e.g., to mark added System RAM resources. mergeable. Prepare for that. >>> >>> What are these random "flags", and how do we know what should be passed >>> to them? >>> >>> Why not make this an enumerated type so that we know it all works >>> properly, like the GPF_* flags are? Passing around a random unsigned >>> long feels very odd/broken... >> >> Agreed, an enum (mhp_flags) seems to give a better hint what can >> actually be passed. Thanks! > > You probably know this but ... > > Just using a C enum doesn't get you any type safety. > > You can get some checking via sparse by using __bitwise, which is what > gfp_t does. You don't actually have to use an enum for that, it works > with #defines also. Yeah, we seem to be using different approaches. And there is always a way to mess things up :) gfp_t is one (extreme) example, enum memblock_flags is another example. I tend to prefer an enum in this particular case, because it's simple and at least tells the user which values are expected. Thoughts? > > Or you can wrap the flag in a struct, the way atomic_t does, and then > the compiler will prevent passing plain integers in place of your custom > type. -- Thanks, David / dhildenb ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/7] mm/memory_hotplug: prepare passing flags to add_memory() and friends
David Hildenbrand writes: > On 09.09.20 09:17, Greg Kroah-Hartman wrote: >> On Tue, Sep 08, 2020 at 10:10:08PM +0200, David Hildenbrand wrote: >>> We soon want to pass flags, e.g., to mark added System RAM resources. >>> mergeable. Prepare for that. >> >> What are these random "flags", and how do we know what should be passed >> to them? >> >> Why not make this an enumerated type so that we know it all works >> properly, like the GPF_* flags are? Passing around a random unsigned >> long feels very odd/broken... > > Agreed, an enum (mhp_flags) seems to give a better hint what can > actually be passed. Thanks! You probably know this but ... Just using a C enum doesn't get you any type safety. You can get some checking via sparse by using __bitwise, which is what gfp_t does. You don't actually have to use an enum for that, it works with #defines also. Or you can wrap the flag in a struct, the way atomic_t does, and then the compiler will prevent passing plain integers in place of your custom type. cheers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/7] mm/memory_hotplug: prepare passing flags to add_memory() and friends
On 09.09.20 09:17, Greg Kroah-Hartman wrote: > On Tue, Sep 08, 2020 at 10:10:08PM +0200, David Hildenbrand wrote: >> We soon want to pass flags, e.g., to mark added System RAM resources. >> mergeable. Prepare for that. > > What are these random "flags", and how do we know what should be passed > to them? > > Why not make this an enumerated type so that we know it all works > properly, like the GPF_* flags are? Passing around a random unsigned > long feels very odd/broken... Agreed, an enum (mhp_flags) seems to give a better hint what can actually be passed. Thanks! -- Thanks, David / dhildenb ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/7] mm/memory_hotplug: prepare passing flags to add_memory() and friends
On Tue, Sep 08, 2020 at 10:10:08PM +0200, David Hildenbrand wrote: > We soon want to pass flags, e.g., to mark added System RAM resources. > mergeable. Prepare for that. What are these random "flags", and how do we know what should be passed to them? Why not make this an enumerated type so that we know it all works properly, like the GPF_* flags are? Passing around a random unsigned long feels very odd/broken... thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/7] mm/memory_hotplug: prepare passing flags to add_memory() and friends
On 08.09.20 22:10, David Hildenbrand wrote:
We soon want to pass flags, e.g., to mark added System RAM resources.
mergeable. Prepare for that.
This patch is based on a similar patch by Oscar Salvador:
https://lkml.kernel.org/r/20190625075227.15193-3-osalva...@suse.de
Acked-by: Wei Liu
Cc: Andrew Morton
Cc: Michal Hocko
Cc: Dan Williams
Cc: Jason Gunthorpe
Cc: Pankaj Gupta
Cc: Baoquan He
Cc: Wei Yang
Cc: Michael Ellerman
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: "Rafael J. Wysocki"
Cc: Len Brown
Cc: Greg Kroah-Hartman
Cc: Vishal Verma
Cc: Dave Jiang
Cc: "K. Y. Srinivasan"
Cc: Haiyang Zhang
Cc: Stephen Hemminger
Cc: Wei Liu
Cc: Heiko Carstens
Cc: Vasily Gorbik
Cc: Christian Borntraeger
Cc: David Hildenbrand
Cc: "Michael S. Tsirkin"
Cc: Jason Wang
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: "Oliver O'Halloran"
Cc: Pingfan Liu
Cc: Nathan Lynch
Cc: Libor Pechacek
Cc: Anton Blanchard
Cc: Leonardo Bras
Cc: linuxppc-...@lists.ozlabs.org
Cc: linux-a...@vger.kernel.org
Cc: linux-nvd...@lists.01.org
Cc: linux-hyp...@vger.kernel.org
Cc: linux-s...@vger.kernel.org
Cc: virtualization@lists.linux-foundation.org
Cc: xen-de...@lists.xenproject.org
Signed-off-by: David Hildenbrand
Reviewed-by: Juergen Gross (Xen related part)
Juergen
---
arch/powerpc/platforms/powernv/memtrace.c | 2 +-
arch/powerpc/platforms/pseries/hotplug-memory.c | 2 +-
drivers/acpi/acpi_memhotplug.c | 2 +-
drivers/base/memory.c | 2 +-
drivers/dax/kmem.c | 2 +-
drivers/hv/hv_balloon.c | 2 +-
drivers/s390/char/sclp_cmd.c| 2 +-
drivers/virtio/virtio_mem.c | 2 +-
drivers/xen/balloon.c | 2 +-
include/linux/memory_hotplug.h | 10 ++
mm/memory_hotplug.c | 15 ---
11 files changed, 23 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/memtrace.c
b/arch/powerpc/platforms/powernv/memtrace.c
index 13b369d2cc454..a7475d18c671c 100644
--- a/arch/powerpc/platforms/powernv/memtrace.c
+++ b/arch/powerpc/platforms/powernv/memtrace.c
@@ -224,7 +224,7 @@ static int memtrace_online(void)
ent->mem = 0;
}
- if (add_memory(ent->nid, ent->start, ent->size)) {
+ if (add_memory(ent->nid, ent->start, ent->size, 0)) {
pr_err("Failed to add trace memory to node %d\n",
ent->nid);
ret += 1;
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c
b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 5d545b78111f9..54a888ea7f751 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -606,7 +606,7 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
block_sz = memory_block_size_bytes();
/* Add the memory */
- rc = __add_memory(lmb->nid, lmb->base_addr, block_sz);
+ rc = __add_memory(lmb->nid, lmb->base_addr, block_sz, 0);
if (rc) {
invalidate_lmb_associativity_index(lmb);
return rc;
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index e294f44a78504..d91b3584d4b2b 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -207,7 +207,7 @@ static int acpi_memory_enable_device(struct
acpi_memory_device *mem_device)
if (node < 0)
node = memory_add_physaddr_to_nid(info->start_addr);
- result = __add_memory(node, info->start_addr, info->length);
+ result = __add_memory(node, info->start_addr, info->length, 0);
/*
* If the memory block has been used by the kernel, add_memory()
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 4db3c660de831..2287bcf86480e 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -432,7 +432,7 @@ static ssize_t probe_store(struct device *dev, struct
device_attribute *attr,
nid = memory_add_physaddr_to_nid(phys_addr);
ret = __add_memory(nid, phys_addr,
- MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+ MIN_MEMORY_BLOCK_SIZE * sections_per_block, 0);
if (ret)
goto out;
diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
index 7dcb2902e9b1b..8e66b28ef5bc6 100644
--- a/drivers/dax/kmem.c
+++ b/drivers/dax/kmem.c
@@ -95,7 +95,7 @@ int dev_dax_kmem_probe(struct dev_dax *dev_dax)
* this as RAM automatically.
*/
rc = add_memory_driver_managed(numa_node, range.start,
- range_len(), kmem_name);
+ range_len(), kmem_name, 0);
res->flags |= IORESOURCE_BUSY;
if (rc) {
[PATCH v2 3/7] mm/memory_hotplug: prepare passing flags to add_memory() and friends
We soon want to pass flags, e.g., to mark added System RAM resources.
mergeable. Prepare for that.
This patch is based on a similar patch by Oscar Salvador:
https://lkml.kernel.org/r/20190625075227.15193-3-osalva...@suse.de
Acked-by: Wei Liu
Cc: Andrew Morton
Cc: Michal Hocko
Cc: Dan Williams
Cc: Jason Gunthorpe
Cc: Pankaj Gupta
Cc: Baoquan He
Cc: Wei Yang
Cc: Michael Ellerman
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: "Rafael J. Wysocki"
Cc: Len Brown
Cc: Greg Kroah-Hartman
Cc: Vishal Verma
Cc: Dave Jiang
Cc: "K. Y. Srinivasan"
Cc: Haiyang Zhang
Cc: Stephen Hemminger
Cc: Wei Liu
Cc: Heiko Carstens
Cc: Vasily Gorbik
Cc: Christian Borntraeger
Cc: David Hildenbrand
Cc: "Michael S. Tsirkin"
Cc: Jason Wang
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: "Oliver O'Halloran"
Cc: Pingfan Liu
Cc: Nathan Lynch
Cc: Libor Pechacek
Cc: Anton Blanchard
Cc: Leonardo Bras
Cc: linuxppc-...@lists.ozlabs.org
Cc: linux-a...@vger.kernel.org
Cc: linux-nvd...@lists.01.org
Cc: linux-hyp...@vger.kernel.org
Cc: linux-s...@vger.kernel.org
Cc: virtualization@lists.linux-foundation.org
Cc: xen-de...@lists.xenproject.org
Signed-off-by: David Hildenbrand
---
arch/powerpc/platforms/powernv/memtrace.c | 2 +-
arch/powerpc/platforms/pseries/hotplug-memory.c | 2 +-
drivers/acpi/acpi_memhotplug.c | 2 +-
drivers/base/memory.c | 2 +-
drivers/dax/kmem.c | 2 +-
drivers/hv/hv_balloon.c | 2 +-
drivers/s390/char/sclp_cmd.c| 2 +-
drivers/virtio/virtio_mem.c | 2 +-
drivers/xen/balloon.c | 2 +-
include/linux/memory_hotplug.h | 10 ++
mm/memory_hotplug.c | 15 ---
11 files changed, 23 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/memtrace.c
b/arch/powerpc/platforms/powernv/memtrace.c
index 13b369d2cc454..a7475d18c671c 100644
--- a/arch/powerpc/platforms/powernv/memtrace.c
+++ b/arch/powerpc/platforms/powernv/memtrace.c
@@ -224,7 +224,7 @@ static int memtrace_online(void)
ent->mem = 0;
}
- if (add_memory(ent->nid, ent->start, ent->size)) {
+ if (add_memory(ent->nid, ent->start, ent->size, 0)) {
pr_err("Failed to add trace memory to node %d\n",
ent->nid);
ret += 1;
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c
b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 5d545b78111f9..54a888ea7f751 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -606,7 +606,7 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
block_sz = memory_block_size_bytes();
/* Add the memory */
- rc = __add_memory(lmb->nid, lmb->base_addr, block_sz);
+ rc = __add_memory(lmb->nid, lmb->base_addr, block_sz, 0);
if (rc) {
invalidate_lmb_associativity_index(lmb);
return rc;
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index e294f44a78504..d91b3584d4b2b 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -207,7 +207,7 @@ static int acpi_memory_enable_device(struct
acpi_memory_device *mem_device)
if (node < 0)
node = memory_add_physaddr_to_nid(info->start_addr);
- result = __add_memory(node, info->start_addr, info->length);
+ result = __add_memory(node, info->start_addr, info->length, 0);
/*
* If the memory block has been used by the kernel, add_memory()
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 4db3c660de831..2287bcf86480e 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -432,7 +432,7 @@ static ssize_t probe_store(struct device *dev, struct
device_attribute *attr,
nid = memory_add_physaddr_to_nid(phys_addr);
ret = __add_memory(nid, phys_addr,
- MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+ MIN_MEMORY_BLOCK_SIZE * sections_per_block, 0);
if (ret)
goto out;
diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
index 7dcb2902e9b1b..8e66b28ef5bc6 100644
--- a/drivers/dax/kmem.c
+++ b/drivers/dax/kmem.c
@@ -95,7 +95,7 @@ int dev_dax_kmem_probe(struct dev_dax *dev_dax)
* this as RAM automatically.
*/
rc = add_memory_driver_managed(numa_node, range.start,
- range_len(), kmem_name);
+ range_len(), kmem_name, 0);
res->flags |= IORESOURCE_BUSY;
if (rc) {
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
