Re: [PATCH v7 1/7] mm, devm_memremap_pages: Mark devm_memremap_pages() EXPORT_SYMBOL_GPL
On Wed 17-10-18 09:30:58, Dan Williams wrote: > On Wed, Oct 17, 2018 at 1:18 AM Michal Hocko wrote: [...] > > > Again, devm_memremap_pagex() exposes and relies upon core kernel > > > internal assumptions and will continue to evolve along with 'struct > > > page', memory hotplug, and support for new memory types / topologies. > > > Only an in-kernel GPL-only driver is expected to keep up with this > > > ongoing evolution. This interface, and functionality derived from this > > > interface, is not suitable for kernel-external drivers. > > > > I do not follow this line of argumentation though. We generally do not > > care about out-of-tree modules and breaking them if the interface has to > > be updated. > > Exactly right. The EXPORT_SYMBOL_GPL is there to say that this api has > deep enough ties into the core kernel to lower the confidence that the > API will stay stable from one kernel revision to the next. It's also > an api that is attracting widening and varied reuse and the long term > health of the implementation depends on being able to peer deeply into > its users and refactor the interface and the core kernel as a result. I am sorry I do not follow. For in-tree modules you have to update users whether the export is GPL or not and we do not care _at all_ about out of tree because we do not guarantee _any_ kABI/API stability (Documentation/process/stable-api-nonsense.rst). Anyway, I do not really care much, but I find the way of the argumentation dubious. I can clearly understand a simple line "me as the author want this GPL - live with that". -- Michal Hocko SUSE Labs
Re: [PATCH v7 1/7] mm, devm_memremap_pages: Mark devm_memremap_pages() EXPORT_SYMBOL_GPL
On Wed, Oct 17, 2018 at 1:18 AM Michal Hocko wrote: > > On Fri 12-10-18 10:49:37, Dan Williams wrote: > > devm_memremap_pages() is a facility that can create struct page entries > > for any arbitrary range and give drivers the ability to subvert core > > aspects of page management. > > > > Specifically the facility is tightly integrated with the kernel's memory > > hotplug functionality. It injects an altmap argument deep into the > > architecture specific vmemmap implementation to allow allocating from > > specific reserved pages, and it has Linux specific assumptions about > > page structure reference counting relative to get_user_pages() and > > get_user_pages_fast(). It was an oversight and a mistake that this was > > not marked EXPORT_SYMBOL_GPL from the outset. > > One thing is still not clear to me. Both devm_memremap_* and > hmm_devmem_add essentially do the same thing AFAICS. They both allow to > hotplug a device memory. Both rely on the hotplug code (namely > add_pages) which itself is not exported to modules. One is GPL only > while the later is a general export. Is this mismatch desirable? That is resolved by the last patch in this series. > API exported by the core hotplug is ad-hoc to say the least. Symbols > that we actually export are GPL mostly (only try_offline_node is > EXPORT_SYMBOL without any explanation whatsoever). So I would call it a > general mess tweaked for specific usecases. > > I personally do not care about EXPORT_SYMBOL vs. EXPORT_SYMBOL_GPL > much to be honest. I understand an argument that we do not care about > out-of-tree modules a wee bit. I would just be worried those will find a > way around and my experience tells me that it would be much uglier than > what the core kernel can provide. But this seems more political than > technical discussion. I'm not worried about people finding a way around, I'm sure cringe-worthy workarounds of core kernel details happen on a regular basis in out-of-tree code. EXPORT_SYMBOL_GPL in this instance is not a political statement, it is a statement of the rate of evolution and depth of the api. > > > Again, devm_memremap_pagex() exposes and relies upon core kernel > > internal assumptions and will continue to evolve along with 'struct > > page', memory hotplug, and support for new memory types / topologies. > > Only an in-kernel GPL-only driver is expected to keep up with this > > ongoing evolution. This interface, and functionality derived from this > > interface, is not suitable for kernel-external drivers. > > I do not follow this line of argumentation though. We generally do not > care about out-of-tree modules and breaking them if the interface has to > be updated. Exactly right. The EXPORT_SYMBOL_GPL is there to say that this api has deep enough ties into the core kernel to lower the confidence that the API will stay stable from one kernel revision to the next. It's also an api that is attracting widening and varied reuse and the long term health of the implementation depends on being able to peer deeply into its users and refactor the interface and the core kernel as a result. > Also what about GPL out of tree modules? These are precisely the modules we want upstream. > That being said, I do not mind this patch. You and Christoph are the > authors and therefore it is you to decide. I just find the current > situation confusing to say the least. Hopefully I clarified, let me know if not.
Re: [PATCH v7 1/7] mm, devm_memremap_pages: Mark devm_memremap_pages() EXPORT_SYMBOL_GPL
On Fri 12-10-18 10:49:37, Dan Williams wrote:
> devm_memremap_pages() is a facility that can create struct page entries
> for any arbitrary range and give drivers the ability to subvert core
> aspects of page management.
>
> Specifically the facility is tightly integrated with the kernel's memory
> hotplug functionality. It injects an altmap argument deep into the
> architecture specific vmemmap implementation to allow allocating from
> specific reserved pages, and it has Linux specific assumptions about
> page structure reference counting relative to get_user_pages() and
> get_user_pages_fast(). It was an oversight and a mistake that this was
> not marked EXPORT_SYMBOL_GPL from the outset.
One thing is still not clear to me. Both devm_memremap_* and
hmm_devmem_add essentially do the same thing AFAICS. They both allow to
hotplug a device memory. Both rely on the hotplug code (namely
add_pages) which itself is not exported to modules. One is GPL only
while the later is a general export. Is this mismatch desirable?
API exported by the core hotplug is ad-hoc to say the least. Symbols
that we actually export are GPL mostly (only try_offline_node is
EXPORT_SYMBOL without any explanation whatsoever). So I would call it a
general mess tweaked for specific usecases.
I personally do not care about EXPORT_SYMBOL vs. EXPORT_SYMBOL_GPL
much to be honest. I understand an argument that we do not care about
out-of-tree modules a wee bit. I would just be worried those will find a
way around and my experience tells me that it would be much uglier than
what the core kernel can provide. But this seems more political than
technical discussion.
> Again, devm_memremap_pagex() exposes and relies upon core kernel
> internal assumptions and will continue to evolve along with 'struct
> page', memory hotplug, and support for new memory types / topologies.
> Only an in-kernel GPL-only driver is expected to keep up with this
> ongoing evolution. This interface, and functionality derived from this
> interface, is not suitable for kernel-external drivers.
I do not follow this line of argumentation though. We generally do not
care about out-of-tree modules and breaking them if the interface has to
be updated. Also what about GPL out of tree modules?
That being said, I do not mind this patch. You and Christoph are the
authors and therefore it is you to decide. I just find the current
situation confusing to say the least.
> Cc: Michal Hocko
> Cc: "Jérôme Glisse"
> Reviewed-by: Christoph Hellwig
> Signed-off-by: Dan Williams
> ---
> kernel/memremap.c |2 +-
> tools/testing/nvdimm/test/iomap.c |2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/memremap.c b/kernel/memremap.c
> index 6ec81e0d7a33..1bbb2e892941 100644
> --- a/kernel/memremap.c
> +++ b/kernel/memremap.c
> @@ -232,7 +232,7 @@ void *devm_memremap_pages(struct device *dev, struct
> dev_pagemap *pgmap)
> err_array:
> return ERR_PTR(error);
> }
> -EXPORT_SYMBOL(devm_memremap_pages);
> +EXPORT_SYMBOL_GPL(devm_memremap_pages);
>
> unsigned long vmem_altmap_offset(struct vmem_altmap *altmap)
> {
> diff --git a/tools/testing/nvdimm/test/iomap.c
> b/tools/testing/nvdimm/test/iomap.c
> index ff9d3a5825e1..ed18a0cbc0c8 100644
> --- a/tools/testing/nvdimm/test/iomap.c
> +++ b/tools/testing/nvdimm/test/iomap.c
> @@ -113,7 +113,7 @@ void *__wrap_devm_memremap_pages(struct device *dev,
> struct dev_pagemap *pgmap)
> return nfit_res->buf + offset - nfit_res->res.start;
> return devm_memremap_pages(dev, pgmap);
> }
> -EXPORT_SYMBOL(__wrap_devm_memremap_pages);
> +EXPORT_SYMBOL_GPL(__wrap_devm_memremap_pages);
>
> pfn_t __wrap_phys_to_pfn_t(phys_addr_t addr, unsigned long flags)
> {
>
--
Michal Hocko
SUSE Labs
[PATCH v7 1/7] mm, devm_memremap_pages: Mark devm_memremap_pages() EXPORT_SYMBOL_GPL
devm_memremap_pages() is a facility that can create struct page entries
for any arbitrary range and give drivers the ability to subvert core
aspects of page management.
Specifically the facility is tightly integrated with the kernel's memory
hotplug functionality. It injects an altmap argument deep into the
architecture specific vmemmap implementation to allow allocating from
specific reserved pages, and it has Linux specific assumptions about
page structure reference counting relative to get_user_pages() and
get_user_pages_fast(). It was an oversight and a mistake that this was
not marked EXPORT_SYMBOL_GPL from the outset.
Again, devm_memremap_pagex() exposes and relies upon core kernel
internal assumptions and will continue to evolve along with 'struct
page', memory hotplug, and support for new memory types / topologies.
Only an in-kernel GPL-only driver is expected to keep up with this
ongoing evolution. This interface, and functionality derived from this
interface, is not suitable for kernel-external drivers.
Cc: Michal Hocko
Cc: "Jérôme Glisse"
Reviewed-by: Christoph Hellwig
Signed-off-by: Dan Williams
---
kernel/memremap.c |2 +-
tools/testing/nvdimm/test/iomap.c |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 6ec81e0d7a33..1bbb2e892941 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -232,7 +232,7 @@ void *devm_memremap_pages(struct device *dev, struct
dev_pagemap *pgmap)
err_array:
return ERR_PTR(error);
}
-EXPORT_SYMBOL(devm_memremap_pages);
+EXPORT_SYMBOL_GPL(devm_memremap_pages);
unsigned long vmem_altmap_offset(struct vmem_altmap *altmap)
{
diff --git a/tools/testing/nvdimm/test/iomap.c
b/tools/testing/nvdimm/test/iomap.c
index ff9d3a5825e1..ed18a0cbc0c8 100644
--- a/tools/testing/nvdimm/test/iomap.c
+++ b/tools/testing/nvdimm/test/iomap.c
@@ -113,7 +113,7 @@ void *__wrap_devm_memremap_pages(struct device *dev, struct
dev_pagemap *pgmap)
return nfit_res->buf + offset - nfit_res->res.start;
return devm_memremap_pages(dev, pgmap);
}
-EXPORT_SYMBOL(__wrap_devm_memremap_pages);
+EXPORT_SYMBOL_GPL(__wrap_devm_memremap_pages);
pfn_t __wrap_phys_to_pfn_t(phys_addr_t addr, unsigned long flags)
{
[PATCH v7 1/7] mm, devm_memremap_pages: Mark devm_memremap_pages() EXPORT_SYMBOL_GPL
devm_memremap_pages() is a facility that can create struct page entries
for any arbitrary range and give drivers the ability to subvert core
aspects of page management.
Specifically the facility is tightly integrated with the kernel's memory
hotplug functionality. It injects an altmap argument deep into the
architecture specific vmemmap implementation to allow allocating from
specific reserved pages, and it has Linux specific assumptions about
page structure reference counting relative to get_user_pages() and
get_user_pages_fast(). It was an oversight and a mistake that this was
not marked EXPORT_SYMBOL_GPL from the outset.
Again, devm_memremap_pagex() exposes and relies upon core kernel
internal assumptions and will continue to evolve along with 'struct
page', memory hotplug, and support for new memory types / topologies.
Only an in-kernel GPL-only driver is expected to keep up with this
ongoing evolution. This interface, and functionality derived from this
interface, is not suitable for kernel-external drivers.
Cc: Michal Hocko
Cc: "Jérôme Glisse"
Reviewed-by: Christoph Hellwig
Signed-off-by: Dan Williams
---
kernel/memremap.c |2 +-
tools/testing/nvdimm/test/iomap.c |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 6ec81e0d7a33..1bbb2e892941 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -232,7 +232,7 @@ void *devm_memremap_pages(struct device *dev, struct
dev_pagemap *pgmap)
err_array:
return ERR_PTR(error);
}
-EXPORT_SYMBOL(devm_memremap_pages);
+EXPORT_SYMBOL_GPL(devm_memremap_pages);
unsigned long vmem_altmap_offset(struct vmem_altmap *altmap)
{
diff --git a/tools/testing/nvdimm/test/iomap.c
b/tools/testing/nvdimm/test/iomap.c
index ff9d3a5825e1..ed18a0cbc0c8 100644
--- a/tools/testing/nvdimm/test/iomap.c
+++ b/tools/testing/nvdimm/test/iomap.c
@@ -113,7 +113,7 @@ void *__wrap_devm_memremap_pages(struct device *dev, struct
dev_pagemap *pgmap)
return nfit_res->buf + offset - nfit_res->res.start;
return devm_memremap_pages(dev, pgmap);
}
-EXPORT_SYMBOL(__wrap_devm_memremap_pages);
+EXPORT_SYMBOL_GPL(__wrap_devm_memremap_pages);
pfn_t __wrap_phys_to_pfn_t(phys_addr_t addr, unsigned long flags)
{
