[PATCH 0/4] add struct page and Direct I/O support to reserved memory

[PATCH 0/4] add struct page and Direct I/O support to reserved memory

[Li Chen]

This patch series use ZONE_DEVICE and mhp to add Direct I/O support to reserved memory
when rmem is as dio's src buffer.

Our use case is when isp generates frame and writes to given memory region, arm(kernel) will
try to read frames from the reserved memory region. If throughput is low, frame loss
will be serious.

Before this patch series, we can only use bufferd I/O and the throughput is very low even
with the help of AIO/io_uring.

This patch is tested on v5.15.35 + no-map rmem region, and can be git am into
5.19-rc5 without conflicts.

Li Chen (4):
  of: add struct page support to rmem
  mm/sparse: skip no-map memblock check when fill_subsection_map
  arm64: mm: move memblock_clear_nomap after __add_pages
  sample/reserved_mem: Introduce a sample of struct page and dio support
    to no-map rmem

 arch/arm64/mm/mmu.c             |   2 +-
 drivers/of/Kconfig              |   9 ++
 drivers/of/of_reserved_mem.c    | 218 +++++++++++++++++++++++++++++++-
 include/linux/of_reserved_mem.h |  11 ++
 mm/sparse.c                     |   4 +-
 samples/Kconfig                 |   7 +
 samples/Makefile                |   1 +
 samples/reserved_mem/Makefile   |   2 +
 samples/reserved_mem/rmem_dio.c | 116 +++++++++++++++++
 9 files changed, 367 insertions(+), 3 deletions(-)
 create mode 100755 samples/reserved_mem/Makefile
 create mode 100755 samples/reserved_mem/rmem_dio.c

-- 
2.25.1

[PATCH 1/4] of: add struct page support to rmem

[Li Chen]

From: Li Chen <lchen@ambarella.com>

This commit add a new config OF_RESERVED_MEM_DIO_SUPPORT and
some utilities to enables consumers to build struct pages on rmem.

Signed-off-by: Li Chen <lchen@ambarella.com>
Change-Id: Iaba8874775c6d3a7096ac19575bb884db13351d1
---
 drivers/of/Kconfig              |   9 ++
 drivers/of/of_reserved_mem.c    | 218 +++++++++++++++++++++++++++++++-
 include/linux/of_reserved_mem.h |  11 ++
 3 files changed, 237 insertions(+), 1 deletion(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 80b5fd44ab1c..0297c03328d8 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -73,6 +73,15 @@ config OF_IRQ
 config OF_RESERVED_MEM
 	def_bool OF_EARLY_FLATTREE
 
+config OF_RESERVED_MEM_DIO_SUPPORT
+	bool "add Direct I/O support to reserved_mem"
+	depends on ZONE_DEVICE && ARCH_KEEP_MEMBLOCK
+	help
+	   By default, reserved memory don't get struct page support, which
+		 means you cannot do Direct I/O from this region. This config takes
+		 uses of ZONE_DEVICE and treats rmem as hotplug mem to get struct
+		 page and DIO support.
+
 config OF_RESOLVE
 	bool
 
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 9da8835ba5a5..f4974da6df98 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -72,7 +72,6 @@ void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname,
 	rmem->size = size;
 
 	reserved_mem_count++;
-	return;
 }
 
 /*
@@ -447,3 +446,220 @@ struct reserved_mem *of_reserved_mem_lookup(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(of_reserved_mem_lookup);
+
+/**
+ * get_reserved_mem_from_dev() - get reserved_mem from a device node
+ * @dev: device pointer
+ *
+ * This function look for reserved_mem from given device.
+ *
+ * Returns a reserved_mem pointer, or NULL on error.
+ */
+struct reserved_mem *get_reserved_mem_from_dev(struct device *dev)
+{
+	struct device_node *np = dev_of_node(dev);
+	struct device_node *rmem_np;
+	struct reserved_mem *rmem = NULL;
+
+	rmem_np = of_parse_phandle(np, "memory-region", 0);
+	if (!rmem_np) {
+		dev_err(dev, "failed to get memory region node\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	rmem = of_reserved_mem_lookup(rmem_np);
+	if (!rmem) {
+		dev_err(dev, "Failed to lookup reserved memory\n");
+		return ERR_PTR(EINVAL);
+	}
+	return rmem;
+}
+EXPORT_SYMBOL_GPL(get_reserved_mem_from_dev);
+
+#ifdef CONFIG_OF_RESERVED_MEM_DIO_SUPPORT
+
+static int reserved_mem_dio_in_region(unsigned long addr,
+				      unsigned long size,
+				      const struct reserved_mem *rmem)
+{
+	if ((rmem && (addr >= rmem->base) &&
+	    ((addr + size) <= (rmem->base + rmem->size))))
+		return 0;
+
+	return -EINVAL;
+}
+
+static int reserved_mem_dio_get_page(struct mm_struct *mm,
+				     unsigned long start,
+				     struct page **page,
+				     const struct reserved_mem *rmem)
+{
+	unsigned long vaddr = start & PAGE_MASK, pfn;
+	int ret = -EFAULT;
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+	pte_t *pte;
+	p4d_t *p4d;
+
+	pgd = pgd_offset(mm, vaddr);
+	if (pgd_none(*pgd))
+		return ret;
+
+	p4d = p4d_offset(pgd, vaddr);
+	if (p4d_none(*p4d))
+		return ret;
+
+	pud = pud_offset(p4d, vaddr);
+	if (pud_none(*pud))
+		return ret;
+
+	pmd = pmd_offset(pud, vaddr);
+	if (pmd_none(*pmd))
+		return ret;
+
+	pte = pte_offset_map(pmd, vaddr);
+	if (pte_none(*pte))
+		goto out;
+
+	pfn = pte_pfn(*pte);
+	if (!pfn_valid(pfn))
+		goto out;
+
+	if (likely(reserved_mem_dio_in_region(pfn << PAGE_SHIFT, PAGE_SIZE, rmem) <
+		   0))
+		goto out;
+
+	if (page) {
+		*page = pfn_to_page(pfn);
+		get_page(*page);
+	}
+
+	ret = 0;
+
+out:
+	pte_unmap(pte);
+	return ret;
+}
+
+static struct page *reserved_mem_dio_find_special_page(struct vm_area_struct *vma,
+						       unsigned long addr)
+{
+	struct page *page = NULL;
+
+	reserved_mem_dio_get_page(vma->vm_mm, addr, &page,
+				  (struct reserved_mem *)vma->vm_private_data);
+	return page;
+}
+
+static const struct vm_operations_struct rmem_dio_vmops = {
+	.find_special_page = reserved_mem_dio_find_special_page,
+};
+
+/**
+ * reserved_mem_dio_mmap() - mmap helper function to map given rmem to userspace
+ *					   with struct pages support
+ * @file: file pointing to address space structure to wait for
+ * @vma:  the vm area in which the mapping is added
+ * @rmem: reserved memory region from dts, which can be obtained from
+ *        get_reserved_mem_from_dev(dev)
+ *
+ * Returns: 0 on success or a negative error-code on failure.
+ */
+int reserved_mem_dio_mmap(struct file *file, struct vm_area_struct *vma, struct reserved_mem *rmem)
+{
+	int ret = 0;
+	unsigned long nr_pages;
+
+	if (!rmem) {
+		pr_err("%s: failed to get rmem from private data\n", __func__);
+		return -ENOMEM;
+	}
+	if (!rmem->pages) {
+		pr_err("%s: failed to get struct pages from reserved mem\n", __func__);
+		return -ENOMEM;
+	}
+
+	if (!rmem->nr_pages) {
+		pr_err("%s: error: rmem nr_pages is 0\n", __func__);
+		return -ENOMEM;
+	}
+
+	if (vma->vm_end - vma->vm_start > rmem->size)
+		return -EINVAL;
+
+	vma->vm_private_data = rmem;
+
+	/* duplicitate nr_pages in that vm_insert_pages can change nr_pages */
+	nr_pages = rmem->nr_pages;
+
+	/*
+	 * use vm_insert_pages instead of add remap_pfn_range variant
+	 * because vm_insert_pages will invoke rmap functions to inc _mapcount,
+	 * while latter don't do it. When unmap,
+	 * kernel will warn if page's _mapcount is <= -1.
+	 */
+	ret = vm_insert_pages(vma, vma->vm_start, rmem->pages, &nr_pages);
+	if (ret < 0)
+		pr_err("%s vm_insert_pages fail, error is %d\n", __func__, ret);
+
+	vma->vm_ops = &rmem_dio_vmops;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(reserved_mem_dio_mmap);
+
+/**
+ * reserved_mem_memremap_pages() - build struct pages for reserved mem
+ * @dev: device pointer
+ * @rmem: reserved memory region from dts, which can be get by
+ *        get_reserved_mem_from_dev(dev)
+ *
+ * Returns: 0 on success or a negative error-code on failure.
+ */
+void *reserved_mem_memremap_pages(struct device *dev, struct reserved_mem *rmem)
+{
+	struct dev_pagemap *pgmap_rmem_dio;
+	void *vaddr;
+	struct page **pages;
+	int i;
+	unsigned long offset = 0;
+	struct page *page;
+
+	rmem->nr_pages = DIV_ROUND_UP(rmem->size, PAGE_SIZE);
+	pages = kvmalloc_array(rmem->nr_pages, sizeof(*pages), GFP_KERNEL);
+	if (!pages)
+		return ERR_PTR(-ENOMEM);
+
+	pgmap_rmem_dio = devm_kzalloc(dev, sizeof(*pgmap_rmem_dio), GFP_KERNEL);
+
+	pgmap_rmem_dio->range.start = rmem->base;
+	pgmap_rmem_dio->range.end = rmem->base + rmem->size - 1;
+	pgmap_rmem_dio->nr_range = 1;
+	pgmap_rmem_dio->type = MEMORY_DEVICE_GENERIC;
+
+	pr_debug("%s, will do devm_memremap_pages, start from %llx, to %llx\n",
+		 __func__, pgmap_rmem_dio->range.start, pgmap_rmem_dio->range.end);
+
+	vaddr = devm_memremap_pages(dev, pgmap_rmem_dio);
+
+	if (IS_ERR_OR_NULL(vaddr)) {
+		dev_err(dev, "%s %d: %ld", __func__, __LINE__, PTR_ERR(vaddr));
+		return vaddr;
+	}
+
+	rmem->pages = pages;
+
+	for (i = 0; i < rmem->nr_pages; offset += PAGE_SIZE) {
+		page = virt_to_page((unsigned long)vaddr + offset);
+		if (!page) {
+			pr_err("%s: virt_to_page fail\n", __func__);
+			return ERR_PTR(-ENOMEM);
+		}
+		pages[i++] = page;
+	}
+
+	return vaddr;
+}
+EXPORT_SYMBOL_GPL(reserved_mem_memremap_pages);
+#endif
diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
index 4de2a24cadc9..0aa1ef883060 100644
--- a/include/linux/of_reserved_mem.h
+++ b/include/linux/of_reserved_mem.h
@@ -16,6 +16,10 @@ struct reserved_mem {
 	phys_addr_t			base;
 	phys_addr_t			size;
 	void				*priv;
+#ifdef CONFIG_OF_RESERVED_MEM_DIO_SUPPORT
+	struct page                     **pages; /* point to array of struct pages of this region */
+	unsigned long                   nr_pages; /* number of struct page* */
+#endif
 };
 
 struct reserved_mem_ops {
@@ -81,4 +85,11 @@ static inline int of_reserved_mem_device_init(struct device *dev)
 	return of_reserved_mem_device_init_by_idx(dev, dev->of_node, 0);
 }
 
+struct reserved_mem *get_reserved_mem_from_dev(struct device *dev);
+
+#ifdef CONFIG_OF_RESERVED_MEM_DIO_SUPPORT
+int reserved_mem_dio_mmap(struct file *file, struct vm_area_struct *vma, struct reserved_mem *rmem);
+void *reserved_mem_memremap_pages(struct device *dev, struct reserved_mem *rmem);
+#endif
+
 #endif /* __OF_RESERVED_MEM_H */
-- 
2.25.1

Re: [PATCH 1/4] of: add struct page support to rmem

[Arnd Bergmann]

On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:

> +config OF_RESERVED_MEM_DIO_SUPPORT
> +       bool "add Direct I/O support to reserved_mem"
> +       depends on ZONE_DEVICE && ARCH_KEEP_MEMBLOCK
> +       help
> +          By default, reserved memory don't get struct page support, which
> +                means you cannot do Direct I/O from this region. This config takes
> +                uses of ZONE_DEVICE and treats rmem as hotplug mem to get struct
> +                page and DIO support.

This probably does not need to be user visible, it's enough to select it from
the drivers that need it.

> @@ -72,7 +72,6 @@ void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname,
>         rmem->size = size;
>
>         reserved_mem_count++;
> -       return;
>  }

This change is not wrong, but it does not belong into the same patch
as the rest, just drop it.

> +/**
> + * get_reserved_mem_from_dev() - get reserved_mem from a device node
> + * @dev: device pointer
> + *
> + * This function look for reserved_mem from given device.
> + *
> + * Returns a reserved_mem pointer, or NULL on error.
> + */
> +struct reserved_mem *get_reserved_mem_from_dev(struct device *dev)
> +{
> +       struct device_node *np = dev_of_node(dev);
> +       struct device_node *rmem_np;
> +       struct reserved_mem *rmem = NULL;
> +
> +       rmem_np = of_parse_phandle(np, "memory-region", 0);
> +       if (!rmem_np) {
> +               dev_err(dev, "failed to get memory region node\n");
> +               return ERR_PTR(-ENODEV);
> +       }
> +
> +       rmem = of_reserved_mem_lookup(rmem_np);
> +       if (!rmem) {
> +               dev_err(dev, "Failed to lookup reserved memory\n");
> +               return ERR_PTR(EINVAL);

This needs to be a negative error code rather than the positive EINVAL.
No need to initialize rmem=NULL first if you override it here.

> +       if (likely(reserved_mem_dio_in_region(pfn << PAGE_SHIFT, PAGE_SIZE, rmem) <
> +                  0))
> +               goto out;

It's not performance critical, so just drop the 'likely()' and put the
rest into one line.


> +       if (page) {
> +               *page = pfn_to_page(pfn);
> +               get_page(*page);
> +       }
> +
> +       ret = 0;
> +
> +out:
> +       pte_unmap(pte);
> +       return ret;
> +}

Should you perhaps return an error when 'page' is NULL?

> +#ifdef CONFIG_OF_RESERVED_MEM_DIO_SUPPORT
> +int reserved_mem_dio_mmap(struct file *file, struct vm_area_struct *vma, struct reserved_mem *rmem);
> +void *reserved_mem_memremap_pages(struct device *dev, struct reserved_mem *rmem);
> +#endif

The '#ifdef' check can be dropped here, declarations are normally
not hidden like this.

         Arnd

Re: [PATCH 1/4] of: add struct page support to rmem

[Li Chen]

Hi Arnd,

Thanks for your review!
 ---- On Mon, 11 Jul 2022 21:36:12 +0800  Arnd Bergmann <arnd@arndb.de> wrote --- 
 > On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:
 > 
 > > +config OF_RESERVED_MEM_DIO_SUPPORT
 > > +       bool "add Direct I/O support to reserved_mem"
 > > +       depends on ZONE_DEVICE && ARCH_KEEP_MEMBLOCK
 > > +       help
 > > +          By default, reserved memory don't get struct page support, which
 > > +                means you cannot do Direct I/O from this region. This config takes
 > > +                uses of ZONE_DEVICE and treats rmem as hotplug mem to get struct
 > > +                page and DIO support.
 > 
 > This probably does not need to be user visible, it's enough to select it from
 > the drivers that need it.

When you say "user visible", do you mean the config can be dropped or something else like Kconfig type other than bool?

 > 
 > > @@ -72,7 +72,6 @@ void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname,
 > >         rmem->size = size;
 > >
 > >         reserved_mem_count++;
 > > -       return;
 > >  }
 > 
 > This change is not wrong, but it does not belong into the same patch
 > as the rest, just drop it.
 > 
 > > +/**
 > > + * get_reserved_mem_from_dev() - get reserved_mem from a device node
 > > + * @dev: device pointer
 > > + *
 > > + * This function look for reserved_mem from given device.
 > > + *
 > > + * Returns a reserved_mem pointer, or NULL on error.
 > > + */
 > > +struct reserved_mem *get_reserved_mem_from_dev(struct device *dev)
 > > +{
 > > +       struct device_node *np = dev_of_node(dev);
 > > +       struct device_node *rmem_np;
 > > +       struct reserved_mem *rmem = NULL;
 > > +
 > > +       rmem_np = of_parse_phandle(np, "memory-region", 0);
 > > +       if (!rmem_np) {
 > > +               dev_err(dev, "failed to get memory region node\n");
 > > +               return ERR_PTR(-ENODEV);
 > > +       }
 > > +
 > > +       rmem = of_reserved_mem_lookup(rmem_np);
 > > +       if (!rmem) {
 > > +               dev_err(dev, "Failed to lookup reserved memory\n");
 > > +               return ERR_PTR(EINVAL);
 > 
 > This needs to be a negative error code rather than the positive EINVAL.
 > No need to initialize rmem=NULL first if you override it here.
 > 
 > > +       if (likely(reserved_mem_dio_in_region(pfn << PAGE_SHIFT, PAGE_SIZE, rmem) <
 > > +                  0))
 > > +               goto out;
 > 
 > It's not performance critical, so just drop the 'likely()' and put the
 > rest into one line.
 > 
 > 
 > > +       if (page) {
 > > +               *page = pfn_to_page(pfn);
 > > +               get_page(*page);
 > > +       }
 > > +
 > > +       ret = 0;
 > > +
 > > +out:
 > > +       pte_unmap(pte);
 > > +       return ret;
 > > +}
 > 
 > Should you perhaps return an error when 'page' is NULL?
 > 
 > > +#ifdef CONFIG_OF_RESERVED_MEM_DIO_SUPPORT
 > > +int reserved_mem_dio_mmap(struct file *file, struct vm_area_struct *vma, struct reserved_mem *rmem);
 > > +void *reserved_mem_memremap_pages(struct device *dev, struct reserved_mem *rmem);
 > > +#endif
 > 
 > The '#ifdef' check can be dropped here, declarations are normally
 > not hidden like this.
 > 
 >          Arnd
 > 

These will be fixed in v2.

Regards,
Li

Re: [PATCH 1/4] of: add struct page support to rmem

[Arnd Bergmann]

On Mon, Jul 11, 2022 at 4:51 PM Li Chen <me@linux.beauty> wrote:
>  ---- On Mon, 11 Jul 2022 21:36:12 +0800  Arnd Bergmann <arnd@arndb.de> wrote ---
>  > On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:
>  >
>  > > +config OF_RESERVED_MEM_DIO_SUPPORT
>  > > +       bool "add Direct I/O support to reserved_mem"
>  > > +       depends on ZONE_DEVICE && ARCH_KEEP_MEMBLOCK
>  > > +       help
>  > > +          By default, reserved memory don't get struct page support, which
>  > > +                means you cannot do Direct I/O from this region. This config takes
>  > > +                uses of ZONE_DEVICE and treats rmem as hotplug mem to get struct
>  > > +                page and DIO support.
>  >
>  > This probably does not need to be user visible, it's enough to select it from
>  > the drivers that need it.
>
> When you say "user visible", do you mean the config can be dropped or something else like Kconfig type other than bool?

I mean this can be a hidden option, which you can do by leaving out the
one-line description after the 'bool' keyword. The option will still
be selectable
in Kconfig files from other options, but not shown in 'make menuconfig'.

        Arnd

Re: [PATCH 1/4] of: add struct page support to rmem

[Li Chen]

Hi Arnd,
 ---- On Mon, 11 Jul 2022 23:06:50 +0800  Arnd Bergmann <arnd@arndb.de> wrote --- 
 > On Mon, Jul 11, 2022 at 4:51 PM Li Chen <me@linux.beauty> wrote:
 > >  ---- On Mon, 11 Jul 2022 21:36:12 +0800  Arnd Bergmann <arnd@arndb.de> wrote ---
 > >  > On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:
 > >  >
 > >  > > +config OF_RESERVED_MEM_DIO_SUPPORT
 > >  > > +       bool "add Direct I/O support to reserved_mem"
 > >  > > +       depends on ZONE_DEVICE && ARCH_KEEP_MEMBLOCK
 > >  > > +       help
 > >  > > +          By default, reserved memory don't get struct page support, which
 > >  > > +                means you cannot do Direct I/O from this region. This config takes
 > >  > > +                uses of ZONE_DEVICE and treats rmem as hotplug mem to get struct
 > >  > > +                page and DIO support.
 > >  >
 > >  > This probably does not need to be user visible, it's enough to select it from
 > >  > the drivers that need it.
 > >
 > > When you say "user visible", do you mean the config can be dropped or something else like Kconfig type other than bool?
 > 
 > I mean this can be a hidden option, which you can do by leaving out the
 > one-line description after the 'bool' keyword. The option will still
 > be selectable
 > in Kconfig files from other options, but not shown in 'make menuconfig'.
 > 
 >         Arnd
 > 

Roger that. I will do it in v2.

Regards,
Li

Re: [PATCH 1/4] of: add struct page support to rmem

[kernel test robot]

Hi Li,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on arm64/for-next/core arm-perf/for-next/perf linus/master v5.19-rc6 next-20220715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Li-Chen/add-struct-page-and-Direct-I-O-support-to-reserved-memory/20220711-202957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20220716/202207160854.nSdKYSY8-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/8b66b4b9614f1c7bb8b2d8fac17d5a2a73acf954
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Li-Chen/add-struct-page-and-Direct-I-O-support-to-reserved-memory/20220711-202957
        git checkout 8b66b4b9614f1c7bb8b2d8fac17d5a2a73acf954
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/mailbox/ drivers/of/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/of/of_reserved_mem.c: In function 'reserved_mem_memremap_pages':
   drivers/of/of_reserved_mem.c:633:50: error: invalid application of 'sizeof' to incomplete type 'struct dev_pagemap'
     633 |         pgmap_rmem_dio = devm_kzalloc(dev, sizeof(*pgmap_rmem_dio), GFP_KERNEL);
         |                                                  ^
   drivers/of/of_reserved_mem.c:635:23: error: invalid use of undefined type 'struct dev_pagemap'
     635 |         pgmap_rmem_dio->range.start = rmem->base;
         |                       ^~
   drivers/of/of_reserved_mem.c:636:23: error: invalid use of undefined type 'struct dev_pagemap'
     636 |         pgmap_rmem_dio->range.end = rmem->base + rmem->size - 1;
         |                       ^~
   drivers/of/of_reserved_mem.c:637:23: error: invalid use of undefined type 'struct dev_pagemap'
     637 |         pgmap_rmem_dio->nr_range = 1;
         |                       ^~
   drivers/of/of_reserved_mem.c:638:23: error: invalid use of undefined type 'struct dev_pagemap'
     638 |         pgmap_rmem_dio->type = MEMORY_DEVICE_GENERIC;
         |                       ^~
   drivers/of/of_reserved_mem.c:638:32: error: 'MEMORY_DEVICE_GENERIC' undeclared (first use in this function)
     638 |         pgmap_rmem_dio->type = MEMORY_DEVICE_GENERIC;
         |                                ^~~~~~~~~~~~~~~~~~~~~
   drivers/of/of_reserved_mem.c:638:32: note: each undeclared identifier is reported only once for each function it appears in
   In file included from include/linux/printk.h:584,
                    from include/linux/kernel.h:29,
                    from include/linux/cpumask.h:10,
                    from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/mutex.h:17,
                    from include/linux/kernfs.h:11,
                    from include/linux/sysfs.h:16,
                    from include/linux/kobject.h:20,
                    from include/linux/of.h:17,
                    from drivers/of/of_reserved_mem.c:15:
   drivers/of/of_reserved_mem.c:641:42: error: invalid use of undefined type 'struct dev_pagemap'
     641 |                  __func__, pgmap_rmem_dio->range.start, pgmap_rmem_dio->range.end);
         |                                          ^~
   include/linux/dynamic_debug.h:134:29: note: in definition of macro '__dynamic_func_call'
     134 |                 func(&id, ##__VA_ARGS__);               \
         |                             ^~~~~~~~~~~
   include/linux/dynamic_debug.h:162:9: note: in expansion of macro '_dynamic_func_call'
     162 |         _dynamic_func_call(fmt, __dynamic_pr_debug,             \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:599:9: note: in expansion of macro 'dynamic_pr_debug'
     599 |         dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~
   drivers/of/of_reserved_mem.c:640:9: note: in expansion of macro 'pr_debug'
     640 |         pr_debug("%s, will do devm_memremap_pages, start from %llx, to %llx\n",
         |         ^~~~~~~~
   drivers/of/of_reserved_mem.c:641:71: error: invalid use of undefined type 'struct dev_pagemap'
     641 |                  __func__, pgmap_rmem_dio->range.start, pgmap_rmem_dio->range.end);
         |                                                                       ^~
   include/linux/dynamic_debug.h:134:29: note: in definition of macro '__dynamic_func_call'
     134 |                 func(&id, ##__VA_ARGS__);               \
         |                             ^~~~~~~~~~~
   include/linux/dynamic_debug.h:162:9: note: in expansion of macro '_dynamic_func_call'
     162 |         _dynamic_func_call(fmt, __dynamic_pr_debug,             \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:599:9: note: in expansion of macro 'dynamic_pr_debug'
     599 |         dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~
   drivers/of/of_reserved_mem.c:640:9: note: in expansion of macro 'pr_debug'
     640 |         pr_debug("%s, will do devm_memremap_pages, start from %llx, to %llx\n",
         |         ^~~~~~~~
   drivers/of/of_reserved_mem.c:643:17: error: implicit declaration of function 'devm_memremap_pages'; did you mean 'devm_free_pages'? [-Werror=implicit-function-declaration]
     643 |         vaddr = devm_memremap_pages(dev, pgmap_rmem_dio);
         |                 ^~~~~~~~~~~~~~~~~~~
         |                 devm_free_pages
>> drivers/of/of_reserved_mem.c:643:15: warning: assignment to 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     643 |         vaddr = devm_memremap_pages(dev, pgmap_rmem_dio);
         |               ^
   cc1: some warnings being treated as errors


vim +643 drivers/of/of_reserved_mem.c

   610	
   611	/**
   612	 * reserved_mem_memremap_pages() - build struct pages for reserved mem
   613	 * @dev: device pointer
   614	 * @rmem: reserved memory region from dts, which can be get by
   615	 *        get_reserved_mem_from_dev(dev)
   616	 *
   617	 * Returns: 0 on success or a negative error-code on failure.
   618	 */
   619	void *reserved_mem_memremap_pages(struct device *dev, struct reserved_mem *rmem)
   620	{
   621		struct dev_pagemap *pgmap_rmem_dio;
   622		void *vaddr;
   623		struct page **pages;
   624		int i;
   625		unsigned long offset = 0;
   626		struct page *page;
   627	
   628		rmem->nr_pages = DIV_ROUND_UP(rmem->size, PAGE_SIZE);
   629		pages = kvmalloc_array(rmem->nr_pages, sizeof(*pages), GFP_KERNEL);
   630		if (!pages)
   631			return ERR_PTR(-ENOMEM);
   632	
   633		pgmap_rmem_dio = devm_kzalloc(dev, sizeof(*pgmap_rmem_dio), GFP_KERNEL);
   634	
   635		pgmap_rmem_dio->range.start = rmem->base;
   636		pgmap_rmem_dio->range.end = rmem->base + rmem->size - 1;
   637		pgmap_rmem_dio->nr_range = 1;
   638		pgmap_rmem_dio->type = MEMORY_DEVICE_GENERIC;
   639	
   640		pr_debug("%s, will do devm_memremap_pages, start from %llx, to %llx\n",
   641			 __func__, pgmap_rmem_dio->range.start, pgmap_rmem_dio->range.end);
   642	
 > 643		vaddr = devm_memremap_pages(dev, pgmap_rmem_dio);

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 1/4] of: add struct page support to rmem

[Dan Carpenter]

Hi Li,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Li-Chen/add-struct-page-and-Direct-I-O-support-to-reserved-memory/20220711-202957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: nios2-randconfig-m031-20220717 (https://download.01.org/0day-ci/archive/20220718/202207181758.YyEzUSPl-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/of/of_reserved_mem.c:472 get_reserved_mem_from_dev() error: passing non negative 22 to ERR_PTR

vim +472 drivers/of/of_reserved_mem.c

8b66b4b9614f1c7 Li Chen 2022-07-11  457  struct reserved_mem *get_reserved_mem_from_dev(struct device *dev)
8b66b4b9614f1c7 Li Chen 2022-07-11  458  {
8b66b4b9614f1c7 Li Chen 2022-07-11  459  	struct device_node *np = dev_of_node(dev);
8b66b4b9614f1c7 Li Chen 2022-07-11  460  	struct device_node *rmem_np;
8b66b4b9614f1c7 Li Chen 2022-07-11  461  	struct reserved_mem *rmem = NULL;
8b66b4b9614f1c7 Li Chen 2022-07-11  462  
8b66b4b9614f1c7 Li Chen 2022-07-11  463  	rmem_np = of_parse_phandle(np, "memory-region", 0);
8b66b4b9614f1c7 Li Chen 2022-07-11  464  	if (!rmem_np) {
8b66b4b9614f1c7 Li Chen 2022-07-11  465  		dev_err(dev, "failed to get memory region node\n");
8b66b4b9614f1c7 Li Chen 2022-07-11  466  		return ERR_PTR(-ENODEV);
8b66b4b9614f1c7 Li Chen 2022-07-11  467  	}
8b66b4b9614f1c7 Li Chen 2022-07-11  468  
8b66b4b9614f1c7 Li Chen 2022-07-11  469  	rmem = of_reserved_mem_lookup(rmem_np);
8b66b4b9614f1c7 Li Chen 2022-07-11  470  	if (!rmem) {
8b66b4b9614f1c7 Li Chen 2022-07-11  471  		dev_err(dev, "Failed to lookup reserved memory\n");
8b66b4b9614f1c7 Li Chen 2022-07-11 @472  		return ERR_PTR(EINVAL);

Missing - character on -EINVAL.

8b66b4b9614f1c7 Li Chen 2022-07-11  473  	}
8b66b4b9614f1c7 Li Chen 2022-07-11  474  	return rmem;
8b66b4b9614f1c7 Li Chen 2022-07-11  475  }

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH 2/4] mm/sparse: skip no-map memblock check when fill_subsection_map

[Li Chen]

From: Li Chen <lchen@ambarella.com>

When mhp use sparse_add_section, don't check no-map region,
so that to allow no-map reserved memory to get struct page
support.

Signed-off-by: Li Chen <lchen@ambarella.com>
Change-Id: I0d2673cec1b66adf695251037a00c240976b226f
---
 mm/sparse.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 120bc8ea5293..a29cd1e7014f 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -690,7 +690,9 @@ static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
 
 	if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
 		rc = -EINVAL;
-	else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
+	else if (memblock_is_map_memory(PFN_PHYS(pfn)) &&
+		 bitmap_intersects(map, subsection_map,
+				   SUBSECTIONS_PER_SECTION))
 		rc = -EEXIST;
 	else
 		bitmap_or(subsection_map, map, subsection_map,
-- 
2.25.1

Re: [PATCH 2/4] mm/sparse: skip no-map memblock check when fill_subsection_map

[David Hildenbrand]

On 11.07.22 14:24, Li Chen wrote:
> From: Li Chen <lchen@ambarella.com>
> 
> When mhp use sparse_add_section, don't check no-map region,
> so that to allow no-map reserved memory to get struct page
> support.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: I0d2673cec1b66adf695251037a00c240976b226f
> ---
>  mm/sparse.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 120bc8ea5293..a29cd1e7014f 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -690,7 +690,9 @@ static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
>  
>  	if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
>  		rc = -EINVAL;
> -	else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
> +	else if (memblock_is_map_memory(PFN_PHYS(pfn)) &&
> +		 bitmap_intersects(map, subsection_map,
> +				   SUBSECTIONS_PER_SECTION))
>  		rc = -EEXIST;
>  	else
>  		bitmap_or(subsection_map, map, subsection_map,

I'm not sure I follow completely what you are trying to achieve. But if
you have to add memblock hacks into mm/sparse.c you're most probably
doing something wrong.

Please explain why that change is necessary, and why it is safe.

If the subsection map already spans memory (iow, subsection map is set)
you intend to add, then something already added memory in that range?

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/4] mm/sparse: skip no-map memblock check when fill_subsection_map

[Li Chen]

Hi David,
 ---- On Mon, 11 Jul 2022 22:53:36 +0800  David Hildenbrand <david@redhat.com> wrote --- 
 > On 11.07.22 14:24, Li Chen wrote:
 > > From: Li Chen <lchen@ambarella.com>
 > > 
 > > When mhp use sparse_add_section, don't check no-map region,
 > > so that to allow no-map reserved memory to get struct page
 > > support.
 > > 
 > > Signed-off-by: Li Chen <lchen@ambarella.com>
 > > Change-Id: I0d2673cec1b66adf695251037a00c240976b226f
 > > ---
 > >  mm/sparse.c | 4 +++-
 > >  1 file changed, 3 insertions(+), 1 deletion(-)
 > > 
 > > diff --git a/mm/sparse.c b/mm/sparse.c
 > > index 120bc8ea5293..a29cd1e7014f 100644
 > > --- a/mm/sparse.c
 > > +++ b/mm/sparse.c
 > > @@ -690,7 +690,9 @@ static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
 > >  
 > >      if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
 > >          rc = -EINVAL;
 > > -    else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
 > > +    else if (memblock_is_map_memory(PFN_PHYS(pfn)) &&
 > > +         bitmap_intersects(map, subsection_map,
 > > +                   SUBSECTIONS_PER_SECTION))
 > >          rc = -EEXIST;
 > >      else
 > >          bitmap_or(subsection_map, map, subsection_map,
 > 
 > I'm not sure I follow completely what you are trying to achieve. But if
 > you have to add memblock hacks into mm/sparse.c you're most probably
 > doing something wrong.
 > 
 > Please explain why that change is necessary, and why it is safe.

In the current sparse memory model, free_area_init will insert all memblock.memory into subsection_map and no-map rmem is also a 
memblock.memory. So, without this change, fill_subsection_map will return -EEXIST.

I would say it's not a good idea to insert no-map memblock into subsection_map, and I have no idea why sparse do this.
So, I simply skip no-map region here.

As for safety:
1. The caller of fill_subsection_map are mhp and *_memremap_pages functions, no-map regions are not related to them, so existing codes won't be broken.
2. This change doesn't change memblock and subsection_map.

 > 
 > If the subsection map already spans memory (iow, subsection map is set)
 > you intend to add, then something already added memory in that range?

No matter with or without this patch, fill_subsection_map will always return -EEXIST for mapped memory, so that's not a problem.
The key point here is I allowed no-map region to pass the check and didn't change anything to the mapped region.

Regards,
Li

Re: [PATCH 2/4] mm/sparse: skip no-map memblock check when fill_subsection_map

[David Hildenbrand]

On 12.07.22 06:23, Li Chen wrote:
> Hi David,
>  ---- On Mon, 11 Jul 2022 22:53:36 +0800  David Hildenbrand <david@redhat.com> wrote --- 
>  > On 11.07.22 14:24, Li Chen wrote:
>  > > From: Li Chen <lchen@ambarella.com>
>  > > 
>  > > When mhp use sparse_add_section, don't check no-map region,
>  > > so that to allow no-map reserved memory to get struct page
>  > > support.
>  > > 
>  > > Signed-off-by: Li Chen <lchen@ambarella.com>
>  > > Change-Id: I0d2673cec1b66adf695251037a00c240976b226f
>  > > ---
>  > >  mm/sparse.c | 4 +++-
>  > >  1 file changed, 3 insertions(+), 1 deletion(-)
>  > > 
>  > > diff --git a/mm/sparse.c b/mm/sparse.c
>  > > index 120bc8ea5293..a29cd1e7014f 100644
>  > > --- a/mm/sparse.c
>  > > +++ b/mm/sparse.c
>  > > @@ -690,7 +690,9 @@ static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
>  > >  
>  > >      if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
>  > >          rc = -EINVAL;
>  > > -    else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
>  > > +    else if (memblock_is_map_memory(PFN_PHYS(pfn)) &&
>  > > +         bitmap_intersects(map, subsection_map,
>  > > +                   SUBSECTIONS_PER_SECTION))
>  > >          rc = -EEXIST;
>  > >      else
>  > >          bitmap_or(subsection_map, map, subsection_map,
>  > 
>  > I'm not sure I follow completely what you are trying to achieve. But if
>  > you have to add memblock hacks into mm/sparse.c you're most probably
>  > doing something wrong.
>  > 
>  > Please explain why that change is necessary, and why it is safe.
> 
> In the current sparse memory model, free_area_init will insert all memblock.memory into subsection_map and no-map rmem is also a 
> memblock.memory. So, without this change, fill_subsection_map will return -EEXIST.
> 
> I would say it's not a good idea to insert no-map memblock into subsection_map, and I have no idea why sparse do this.
> So, I simply skip no-map region here.

The thing is:

if the subsection map is set, then there already *is* a memmap and you
would simply be ignoring it (and overwriting a memmap in e.g.,
ZONE_NORMAL to be in ZONE_DEVICE suddenly, which is wrong).


Reading memblock_mark_nomap():

"The memory regions marked with %MEMBLOCK_NOMAP will not be added to the
direct mapping of the physical memory. These regions will still be
covered by the memory map. The struct page representing NOMAP memory
frames in the memory map will be PageReserved()"


So having a memmap for these ranges is expected, and a direct map is not
desired. What you propose is a hack. You either have to reuse the
existing memmap (which is !ZONE_DEVICE -- not sure if that's a problem)
or we'd have to look into teaching init code to not allocate a memmap
for sub-sections that are fully nomap.

But not sure who depends on the existing memmap for nomap memory.

> 
> As for safety:
> 1. The caller of fill_subsection_map are mhp and *_memremap_pages functions, no-map regions are not related to them, so existing codes won't be broken.
> 2. This change doesn't change memblock and subsection_map.
> 

Sorry, but AFAIKT it's a hack and we need a clean way to deal with nomap
memory that already has a memmap instead.


-- 
Thanks,

David / dhildenb

Re: [PATCH 2/4] mm/sparse: skip no-map memblock check when fill_subsection_map

[Li Chen]

Hi David,
 ---- On Tue, 12 Jul 2022 15:31:08 +0800  David Hildenbrand <david@redhat.com> wrote --- 
 > On 12.07.22 06:23, Li Chen wrote:
 > > Hi David,
 > >  ---- On Mon, 11 Jul 2022 22:53:36 +0800  David Hildenbrand <david@redhat.com> wrote --- 
 > >  > On 11.07.22 14:24, Li Chen wrote:
 > >  > > From: Li Chen <lchen@ambarella.com>
 > >  > > 
 > >  > > When mhp use sparse_add_section, don't check no-map region,
 > >  > > so that to allow no-map reserved memory to get struct page
 > >  > > support.
 > >  > > 
 > >  > > Signed-off-by: Li Chen <lchen@ambarella.com>
 > >  > > Change-Id: I0d2673cec1b66adf695251037a00c240976b226f
 > >  > > ---
 > >  > >  mm/sparse.c | 4 +++-
 > >  > >  1 file changed, 3 insertions(+), 1 deletion(-)
 > >  > > 
 > >  > > diff --git a/mm/sparse.c b/mm/sparse.c
 > >  > > index 120bc8ea5293..a29cd1e7014f 100644
 > >  > > --- a/mm/sparse.c
 > >  > > +++ b/mm/sparse.c
 > >  > > @@ -690,7 +690,9 @@ static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
 > >  > >  
 > >  > >      if (bitmap_empty(map, SUBSECTIONS_PER_SECTION))
 > >  > >          rc = -EINVAL;
 > >  > > -    else if (bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION))
 > >  > > +    else if (memblock_is_map_memory(PFN_PHYS(pfn)) &&
 > >  > > +         bitmap_intersects(map, subsection_map,
 > >  > > +                   SUBSECTIONS_PER_SECTION))
 > >  > >          rc = -EEXIST;
 > >  > >      else
 > >  > >          bitmap_or(subsection_map, map, subsection_map,
 > >  > 
 > >  > I'm not sure I follow completely what you are trying to achieve. But if
 > >  > you have to add memblock hacks into mm/sparse.c you're most probably
 > >  > doing something wrong.
 > >  > 
 > >  > Please explain why that change is necessary, and why it is safe.
 > > 
 > > In the current sparse memory model, free_area_init will insert all memblock.memory into subsection_map and no-map rmem is also a 
 > > memblock.memory. So, without this change, fill_subsection_map will return -EEXIST.
 > > 
 > > I would say it's not a good idea to insert no-map memblock into subsection_map, and I have no idea why sparse do this.
 > > So, I simply skip no-map region here.
 > 
 > The thing is:
 > 
 > if the subsection map is set, then there already *is* a memmap and you
 > would simply be ignoring it (and overwriting a memmap in e.g.,
 > ZONE_NORMAL to be in ZONE_DEVICE suddenly, which is wrong).
 > 
 > 
 > Reading memblock_mark_nomap():
 > 
 > "The memory regions marked with %MEMBLOCK_NOMAP will not be added to the
 > direct mapping of the physical memory. These regions will still be
 > covered by the memory map. The struct page representing NOMAP memory
 > frames in the memory map will be PageReserved()"
 > 
 > 
 > So having a memmap for these ranges is expected, and a direct map is not
 > desired. What you propose is a hack. You either have to reuse the
 > existing memmap (which is !ZONE_DEVICE -- not sure if that's a problem)
 > or we'd have to look into teaching init code to not allocate a memmap
 > for sub-sections that are fully nomap.
 > 
 > But not sure who depends on the existing memmap for nomap memory.

 Points taken, thanks! I will try to dig into it.

Regards,
Li
 > > 
 > > As for safety:
 > > 1. The caller of fill_subsection_map are mhp and *_memremap_pages functions, no-map regions are not related to them, so existing codes won't be broken.
 > > 2. This change doesn't change memblock and subsection_map.
 > > 
 > 
 > Sorry, but AFAIKT it's a hack and we need a clean way to deal with nomap
 > memory that already has a memmap instead.
 > 
 > 
 > -- 
 > Thanks,
 > 
 > David / dhildenb
 > 
 > 

Re: [PATCH 2/4] mm/sparse: skip no-map memblock check when fill_subsection_map

[kernel test robot]

Hi Li,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on arm64/for-next/core arm-perf/for-next/perf linus/master v5.19-rc6 next-20220714]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Li-Chen/add-struct-page-and-Direct-I-O-support-to-reserved-memory/20220711-202957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-randconfig-k001 (https://download.01.org/0day-ci/archive/20220715/202207150209.3Svjqq9D-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 5e61b9c556267086ef9b743a0b57df302eef831b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d9809d17afee6693084b417325807c7123432fab
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Li-Chen/add-struct-page-and-Direct-I-O-support-to-reserved-memory/20220711-202957
        git checkout d9809d17afee6693084b417325807c7123432fab
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> WARNING: modpost: vmlinux.o(.text+0x346d8f): Section mismatch in reference from the function fill_subsection_map() to the function .meminit.text:memblock_is_map_memory()
The function fill_subsection_map() references
the function __meminit memblock_is_map_memory().
This is often because fill_subsection_map lacks a __meminit
annotation or the annotation of memblock_is_map_memory is wrong.

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH 3/4] arm64: mm: move memblock_clear_nomap after __add_pages

[Li Chen]

From: Li Chen <lchen@ambarella.com>

I'm trying to add struct page support to nomap reserved memory,
and have skipped nomap bitmap_intersects check in fill_subsection_map,
so just move memblock_clear_nomap after __add_pages.

Signed-off-by: Li Chen <lchen@ambarella.com>
Change-Id: I5e26fdc3f3e55b12f1acc1adb47fb262c4877ff3
---
 arch/arm64/mm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 6680689242df..2e7f503837e4 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1537,10 +1537,10 @@ int arch_add_memory(int nid, u64 start, u64 size,
 			     size, params->pgprot, __pgd_pgtable_alloc,
 			     flags);
 
-	memblock_clear_nomap(start, size);
 
 	ret = __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
 			   params);
+	memblock_clear_nomap(start, size);
 	if (ret)
 		__remove_pgd_mapping(swapper_pg_dir,
 				     __phys_to_virt(start), size);
-- 
2.25.1

[PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Li Chen]

From: Li Chen <lchen@ambarella.com>

This sample driver shows how to build struct pages support to no-map rmem.

Signed-off-by: Li Chen <lchen@ambarella.com>
Change-Id: Ie78494fa86fda40ceb73eab3b8ba505d0ad851a1
---
 samples/Kconfig                 |   7 ++
 samples/Makefile                |   1 +
 samples/reserved_mem/Makefile   |   2 +
 samples/reserved_mem/rmem_dio.c | 116 ++++++++++++++++++++++++++++++++
 4 files changed, 126 insertions(+)
 create mode 100755 samples/reserved_mem/Makefile
 create mode 100755 samples/reserved_mem/rmem_dio.c

diff --git a/samples/Kconfig b/samples/Kconfig
index b0503ef058d3..d83ba02ec215 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -6,6 +6,13 @@ menuconfig SAMPLES
 
 if SAMPLES
 
+config RESERVED_MEM_DIO
+	tristate "Build example reserved mem dio support"
+	depends on OF_RESERVED_MEM_DIO_SUPPORT
+	help
+	   Build kernel space sample module to show how to add struct
+		 page and dio support to reserved memory.
+
 config SAMPLE_AUXDISPLAY
 	bool "auxdisplay sample"
 	depends on CC_CAN_LINK
diff --git a/samples/Makefile b/samples/Makefile
index 087e0988ccc5..106a386869eb 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Linux samples code
 
+obj-$(CONFIG_RESERVED_MEM_DIO)		+= reserved_mem/
 subdir-$(CONFIG_SAMPLE_AUXDISPLAY)	+= auxdisplay
 subdir-$(CONFIG_SAMPLE_ANDROID_BINDERFS) += binderfs
 obj-$(CONFIG_SAMPLE_CONFIGFS)		+= configfs/
diff --git a/samples/reserved_mem/Makefile b/samples/reserved_mem/Makefile
new file mode 100755
index 000000000000..a4f5c8bc08dc
--- /dev/null
+++ b/samples/reserved_mem/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_RESERVED_MEM_DIO) += rmem_dio.o
diff --git a/samples/reserved_mem/rmem_dio.c b/samples/reserved_mem/rmem_dio.c
new file mode 100755
index 000000000000..ffb3395de63c
--- /dev/null
+++ b/samples/reserved_mem/rmem_dio.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Sample driver for reserved memory with struct page and dio support.
+ *
+ * wsps is abbr for with struct page support
+ *
+ * Copyright (C) 2022 Li Chen <lchen@ambarella.com>
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/fs.h>
+#include <linux/errno.h>
+#include <linux/types.h>
+#include <linux/mm.h>
+#include <linux/cdev.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/miscdevice.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+
+/*
+ * dts example
+ * rmem: rmem@1 {
+ *			compatible = "shared-dma-pool";
+ *			no-map;
+ *			size = <0x0 0x20000000>;
+ *		};
+ * perf {
+ *		compatible = "example,rmem";
+ *		memory-region = <&rmem>;
+ *	};
+ */
+
+static struct reserved_mem *rmem;
+
+static int rmem_wsps_open(struct inode *inode, struct file *filp)
+{
+	return 0;
+}
+
+static int rmem_wsps_release(struct inode *inode,
+					  struct file *filp)
+{
+	return 0;
+}
+
+static int rmem_wsps_mmap(struct file *file,
+					     struct vm_area_struct *vma)
+{
+	return reserved_mem_dio_mmap(file, vma, rmem);
+}
+
+static const struct file_operations rmem_wsps_remap_ops = {
+	.owner = THIS_MODULE,
+	.open = rmem_wsps_open,
+	.release = rmem_wsps_release,
+	.mmap = rmem_wsps_mmap,
+};
+
+static struct miscdevice rmem_wsps_miscdev = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "rmem_wsps",
+	.fops = &rmem_wsps_remap_ops,
+};
+
+static int rmem_wsps_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	void *vaddr;
+	int ret;
+
+	ret = misc_register(&rmem_wsps_miscdev);
+	if (ret) {
+		dev_err(dev, "%s: misc_register failed, %d\n", __func__, ret);
+		return -ENODEV;
+	}
+
+	rmem = get_reserved_mem_from_dev(dev);
+	if (!rmem) {
+		dev_err(dev, "%s: failed to find reserved\n", __func__);
+		return -ENODEV;
+	}
+
+	vaddr = reserved_mem_memremap_pages(dev, rmem);
+	if (IS_ERR_OR_NULL(vaddr))
+		return PTR_ERR(vaddr);
+
+	return 0;
+}
+
+static const struct of_device_id rmem_dio_match[] = {
+	{
+		.compatible = "example,rmem",
+	},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, rmem_dio_match);
+
+static struct platform_driver rmem_wsps_driver = {
+	.probe = rmem_wsps_probe,
+	.driver = {
+		.name = "rmem_wsps",
+		.of_match_table = rmem_dio_match,
+	},
+};
+
+static int __init rmem_wsps_init(void)
+{
+	return platform_driver_register(&rmem_wsps_driver);
+}
+
+module_init(rmem_wsps_init);
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Arnd Bergmann]

On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:
>
> From: Li Chen <lchen@ambarella.com>
>
> This sample driver shows how to build struct pages support to no-map rmem.
>
> Signed-off-by: Li Chen <lchen@ambarella.com>

Not sure what a sample driver helps here if there are no actual users in-tree.

It would make more sense to merge the driver that wants to actually use this
first, and then add the additional feature.

> Change-Id: Ie78494fa86fda40ceb73eab3b8ba505d0ad851a1

Please drop these lines, the Change-Id fields are useless in a public
repository.

> +/*
> + * dts example
> + * rmem: rmem@1 {
> + *                     compatible = "shared-dma-pool";
> + *                     no-map;
> + *                     size = <0x0 0x20000000>;
> + *             };
> + * perf {
> + *             compatible = "example,rmem";
> + *             memory-region = <&rmem>;
> + *     };

The problem here is that the DT is meant to describe the platform in an OS
independent way, so having a binding that just corresponds to a user space
interface is not a good abstraction.

> +       vaddr = reserved_mem_memremap_pages(dev, rmem);
> +       if (IS_ERR_OR_NULL(vaddr))
> +               return PTR_ERR(vaddr);

 Using IS_ERR_OR_NULL() is usually an indication of a bad interface.

For the reserved_mem_memremap_pages(), you should decide whether to return
NULL on error or an error pointer, but not both.

       Arnd

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Li Chen]

Hi Arnd,
 ---- On Mon, 11 Jul 2022 21:28:10 +0800  Arnd Bergmann <arnd@arndb.de> wrote --- 
 > On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:
 > >
 > > From: Li Chen <lchen@ambarella.com>
 > >
 > > This sample driver shows how to build struct pages support to no-map rmem.
 > >
 > > Signed-off-by: Li Chen <lchen@ambarella.com>
 > 
 > Not sure what a sample driver helps here if there are no actual users in-tree.
 > 
 > It would make more sense to merge the driver that wants to actually use this
 > first, and then add the additional feature.

Totally agree, but we plan to start rewriting our video driver in a long time, it has many legacy codes and I need to rewrite a lot of codes to migrate to v4l2.
That's why I also submit a sample driver here: to make the review progress easier and don't need reviewers to read video driver codes.

 > > +/*
 > > + * dts example
 > > + * rmem: rmem@1 {
 > > + *                     compatible = "shared-dma-pool";
 > > + *                     no-map;
 > > + *                     size = <0x0 0x20000000>;
 > > + *             };
 > > + * perf {
 > > + *             compatible = "example,rmem";
 > > + *             memory-region = <&rmem>;
 > > + *     };
 > 
 > The problem here is that the DT is meant to describe the platform in an OS
 > independent way, so having a binding that just corresponds to a user space
 > interface is not a good abstraction.

Gotcha, but IMO dts + rmem is the only choice for our use case. In our real case, we use reg instead of size to specify the physical address, so memremap cannot be used.

 > 
 > > +       vaddr = reserved_mem_memremap_pages(dev, rmem);
 > > +       if (IS_ERR_OR_NULL(vaddr))
 > > +               return PTR_ERR(vaddr);
 > 
 >  Using IS_ERR_OR_NULL() is usually an indication of a bad interface.
 > 
 > For the reserved_mem_memremap_pages(), you should decide whether to return
 > NULL on error or an error pointer, but not both.

Thanks, will fix in v2.

 > 
 >        Arnd
 > 

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Arnd Bergmann]

On Tue, Jul 12, 2022 at 2:26 AM Li Chen <me@linux.beauty> wrote:
>  ---- On Mon, 11 Jul 2022 21:28:10 +0800  Arnd Bergmann <arnd@arndb.de> wrote ---
>  > On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:
>  > >
>  > > From: Li Chen <lchen@ambarella.com>
>  > >
>  > > This sample driver shows how to build struct pages support to no-map rmem.
>  > >
>  > > Signed-off-by: Li Chen <lchen@ambarella.com>
>  >
>  > Not sure what a sample driver helps here if there are no actual users in-tree.
>  >
>  > It would make more sense to merge the driver that wants to actually use this
>  > first, and then add the additional feature.
>
> Totally agree, but we plan to start rewriting our video driver in a long time, it
> has many legacy codes and I need to rewrite a lot of codes to migrate to v4l2.
> That's why I also submit a sample driver here: to make the review progress
> easier and don't need reviewers to read video driver codes.

The problem is that this patch may not be the right solution for your new
driver either.  As Christoph also commented, what you do here is rather
unusual, and without seeing the video driver first, we have no way of
knowing whether there is something the driver should be doing
differently to solve the original problem.

>  > > +/*
>  > > + * dts example
>  > > + * rmem: rmem@1 {
>  > > + *                     compatible = "shared-dma-pool";
>  > > + *                     no-map;
>  > > + *                     size = <0x0 0x20000000>;
>  > > + *             };
>  > > + * perf {
>  > > + *             compatible = "example,rmem";
>  > > + *             memory-region = <&rmem>;
>  > > + *     };
>  >
>  > The problem here is that the DT is meant to describe the platform in an OS
>  > independent way, so having a binding that just corresponds to a user space
>  > interface is not a good abstraction.
>
> Gotcha, but IMO dts + rmem is the only choice for our use case. In our real
> case, we use reg instead of size to specify the physical address, so
> memremap cannot be used.

Does your hardware require a fixed address for the buffer? If it can be
anywhere in memory (or at least within a certain range) but just has to
be physically contiguous, the normal way would be to use a CMA area
to allocate from, which gives you 'struct page' backed pages.

         Arnd

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Li Chen]

Hi Arnd,
 ---- On Tue, 12 Jul 2022 15:50:46 +0800  Arnd Bergmann <arnd@arndb.de> wrote --- 
 > On Tue, Jul 12, 2022 at 2:26 AM Li Chen <me@linux.beauty> wrote:
 > >  ---- On Mon, 11 Jul 2022 21:28:10 +0800  Arnd Bergmann <arnd@arndb.de> wrote ---
 > >  > On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:
 > >  > >
 > >  > > From: Li Chen <lchen@ambarella.com>
 > >  > >
 > >  > > This sample driver shows how to build struct pages support to no-map rmem.
 > >  > >
 > >  > > Signed-off-by: Li Chen <lchen@ambarella.com>
 > >  >
 > >  > Not sure what a sample driver helps here if there are no actual users in-tree.
 > >  >
 > >  > It would make more sense to merge the driver that wants to actually use this
 > >  > first, and then add the additional feature.
 > >
 > > Totally agree, but we plan to start rewriting our video driver in a long time, it
 > > has many legacy codes and I need to rewrite a lot of codes to migrate to v4l2.
 > > That's why I also submit a sample driver here: to make the review progress
 > > easier and don't need reviewers to read video driver codes.
 > 
 > The problem is that this patch may not be the right solution for your new
 > driver either.  As Christoph also commented, what you do here is rather
 > unusual, and without seeing the video driver first, we have no way of
 > knowing whether there is something the driver should be doing
 > differently to solve the original problem.

Ok, I will update the patch series after rewriting and upstreaming our video driver.

 > 
 > >  > > +/*
 > >  > > + * dts example
 > >  > > + * rmem: rmem@1 {
 > >  > > + *                     compatible = "shared-dma-pool";
 > >  > > + *                     no-map;
 > >  > > + *                     size = <0x0 0x20000000>;
 > >  > > + *             };
 > >  > > + * perf {
 > >  > > + *             compatible = "example,rmem";
 > >  > > + *             memory-region = <&rmem>;
 > >  > > + *     };
 > >  >
 > >  > The problem here is that the DT is meant to describe the platform in an OS
 > >  > independent way, so having a binding that just corresponds to a user space
 > >  > interface is not a good abstraction.
 > >
 > > Gotcha, but IMO dts + rmem is the only choice for our use case. In our real
 > > case, we use reg instead of size to specify the physical address, so
 > > memremap cannot be used.
 > 
 > Does your hardware require a fixed address for the buffer? If it can be
 > anywhere in memory (or at least within a certain range) but just has to
 > be physically contiguous, the normal way would be to use a CMA area
 > to allocate from, which gives you 'struct page' backed pages.

The limitation is our DSP can only access 32bit memory, but total dram is > 4G, so I cannot use
"size = <...>" in our real case (it might get memory above 4G). I'm not sure if other vendors' DSP also has
this limitation, if so, how do they deal with it if throughput matters.

Regards,
Li

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Arnd Bergmann]

On Tue, Jul 12, 2022 at 11:58 AM Li Chen <me@linux.beauty> wrote:
>  > On Tue, Jul 12, 2022 at 2:26 AM Li Chen <me@linux.beauty> wrote:
>  > >  ---- On Mon, 11 Jul 2022 21:28:10 +0800  Arnd Bergmann <arnd@arndb.de> wrote ---
>  > >  > On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:
>  > >  > The problem here is that the DT is meant to describe the platform in an OS
>  > >  > independent way, so having a binding that just corresponds to a user space
>  > >  > interface is not a good abstraction.
>  > >
>  > > Gotcha, but IMO dts + rmem is the only choice for our use case. In our real
>  > > case, we use reg instead of size to specify the physical address, so
>  > > memremap cannot be used.
>  >
>  > Does your hardware require a fixed address for the buffer? If it can be
>  > anywhere in memory (or at least within a certain range) but just has to
>  > be physically contiguous, the normal way would be to use a CMA area
>  > to allocate from, which gives you 'struct page' backed pages.
>
> The limitation is our DSP can only access 32bit memory, but total dram is > 4G, so I cannot use
> "size = <...>" in our real case (it might get memory above 4G). I'm not sure if other vendors' DSP also has
> this limitation, if so, how do they deal with it if throughput matters.

This is a common limitation that gets handled automatically by setting
the dma_mask of the device through the dma-ranges property in DT.
When the driver does dma_alloc_coherent() or similar to gets its buffer,
it will then allocate pages below this boundary.

If you need a large contiguous memory area, then using CMA allows
you to specify a region of memory that is kept reserved for DMA
allocations, so a call to dma_alloc_coherent() on your device will
get contiguous pages from that area, and move other data in those
pages elsewhere if necessary. non-movable data is allocated from
pages outside of the CMA reserved area in this case.

          Arnd

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Arnd Bergmann]

On Tue, Jul 12, 2022 at 12:08 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jul 12, 2022 at 11:58 AM Li Chen <me@linux.beauty> wrote:
> >
> > The limitation is our DSP can only access 32bit memory, but total dram is > 4G, so I cannot use
> > "size = <...>" in our real case (it might get memory above 4G). I'm not sure if other vendors' DSP also has
> > this limitation, if so, how do they deal with it if throughput matters.
>
> This is a common limitation that gets handled automatically by setting
> the dma_mask of the device through the dma-ranges property in DT.
> When the driver does dma_alloc_coherent() or similar to gets its buffer,
> it will then allocate pages below this boundary.

To clarify: in the DT, you can either add a 'alloc-ranges' property to limit
where the CMA area gets allocated, or use a 'reg' property instead of the
'size' property to force a specific address. I would expect that in either
case, you get the type of memory area you want (a reserved set of
addresses with page structures that you can use for contiguous
allocations within the first 4GB), but it's possible that I'm missing
some more details here.

         Arnd

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Li Chen]

Hi Arnd,
 ---- On Tue, 12 Jul 2022 18:08:10 +0800  Arnd Bergmann <arnd@arndb.de> wrote --- 
 > On Tue, Jul 12, 2022 at 11:58 AM Li Chen <me@linux.beauty> wrote:
 > >  > On Tue, Jul 12, 2022 at 2:26 AM Li Chen <me@linux.beauty> wrote:
 > >  > >  ---- On Mon, 11 Jul 2022 21:28:10 +0800  Arnd Bergmann <arnd@arndb.de> wrote ---
 > >  > >  > On Mon, Jul 11, 2022 at 2:24 PM Li Chen <me@linux.beauty> wrote:
 > >  > >  > The problem here is that the DT is meant to describe the platform in an OS
 > >  > >  > independent way, so having a binding that just corresponds to a user space
 > >  > >  > interface is not a good abstraction.
 > >  > >
 > >  > > Gotcha, but IMO dts + rmem is the only choice for our use case. In our real
 > >  > > case, we use reg instead of size to specify the physical address, so
 > >  > > memremap cannot be used.
 > >  >
 > >  > Does your hardware require a fixed address for the buffer? If it can be
 > >  > anywhere in memory (or at least within a certain range) but just has to
 > >  > be physically contiguous, the normal way would be to use a CMA area
 > >  > to allocate from, which gives you 'struct page' backed pages.
 > >
 > > The limitation is our DSP can only access 32bit memory, but total dram is > 4G, so I cannot use
 > > "size = <...>" in our real case (it might get memory above 4G). I'm not sure if other vendors' DSP also has
 > > this limitation, if so, how do they deal with it if throughput matters.
 > 
 > This is a common limitation that gets handled automatically by setting
 > the dma_mask of the device through the dma-ranges property in DT.
 > When the driver does dma_alloc_coherent() or similar to gets its buffer,
 > it will then allocate pages below this boundary.
 
Thanks for the tip! I wasn't aware that dma-ranges can be used by devices other than PCIe controllers.

 > If you need a large contiguous memory area, then using CMA allows
 > you to specify a region of memory that is kept reserved for DMA
 > allocations, so a call to dma_alloc_coherent() on your device will
 > get contiguous pages from that area, and move other data in those
 > pages elsewhere if necessary. non-movable data is allocated from
 > pages outside of the CMA reserved area in this case.

We need a large memory pool, around 2G. I will try CMA and dma-ranges later!

Regards,
Li

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Arnd Bergmann]

On Tue, Jul 12, 2022 at 12:55 PM Li Chen <me@linux.beauty> wrote:
>  >
>  > This is a common limitation that gets handled automatically by setting
>  > the dma_mask of the device through the dma-ranges property in DT.
>  > When the driver does dma_alloc_coherent() or similar to gets its buffer,
>  > it will then allocate pages below this boundary.
>
> Thanks for the tip! I wasn't aware that dma-ranges can be used by devices other than PCIe controllers.

You should actually have dma-ranges listed in the parent of any DMA master
capable device, to list the exact DMA capabilities. Without this, devices
fall back to the default 32-bit address limit, which would be correct for your
video device but is often wrong in other devices.

        Arnd

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Li Chen]

Hi Arnd,
---- On Tue, 12 Jul 2022 16:50:46 +0900  Arnd Bergmann  wrote --- 
 > Does your hardware require a fixed address for the buffer? If it can be
 > anywhere in memory (or at least within a certain range) but just has to
 > be physically contiguous, the normal way would be to use a CMA area
 > to allocate from, which gives you 'struct page' backed pages.

CMA does support Direct I/O, but it has its own issue: 
It does not guarantee that the memory previously borrowed by the OS will be returned to the device.

We've been plagued by examples like this in the past:
Many other kernel modules/subsystems have already allocated much memory from both non-CMA and CMA memory,
When our DSP driver got probed then, cma_alloc will fail in that non-CMA system memory is not enough
for CMA memory to migrate.  

Regards,
Li

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Arnd Bergmann]

On Thu, Aug 4, 2022 at 9:17 AM Li Chen <me@linux.beauty> wrote:
> ---- On Tue, 12 Jul 2022 16:50:46 +0900  Arnd Bergmann  wrote ---
>  > Does your hardware require a fixed address for the buffer? If it can be
>  > anywhere in memory (or at least within a certain range) but just has to
>  > be physically contiguous, the normal way would be to use a CMA area
>  > to allocate from, which gives you 'struct page' backed pages.
>
> CMA does support Direct I/O, but it has its own issue:
> It does not guarantee that the memory previously borrowed by the OS will be returned to the device.
>
> We've been plagued by examples like this in the past:
> Many other kernel modules/subsystems have already allocated much memory from both non-CMA and CMA memory,
> When our DSP driver got probed then, cma_alloc will fail in that non-CMA system memory is not enough
> for CMA memory to migrate.

This part should at least be possible to solve by declaring the amount
and location of
CMA areas in the right way. It's not great to fine-tune the DT for a
particular kernel's
use, but if you know which other drivers require CMA type allocations
you can find a lower
bound that should suffice.

Most coherent allocations tend to be long-lived and only for very
small memory regions.
If you have another driver that uses large or periodic
dma_alloc_coherent() type allocations,
you can consider either giving that device its own CMA area, or fixing
it to use streaming
mappings.

        Arnd

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Li Chen]

 ---- On Thu, 04 Aug 2022 17:24:20 +0900  Arnd Bergmann  wrote --- 
 > On Thu, Aug 4, 2022 at 9:17 AM Li Chen me@linux.beauty> wrote:
 > > ---- On Tue, 12 Jul 2022 16:50:46 +0900  Arnd Bergmann  wrote ---
 > >  > Does your hardware require a fixed address for the buffer? If it can be
 > >  > anywhere in memory (or at least within a certain range) but just has to
 > >  > be physically contiguous, the normal way would be to use a CMA area
 > >  > to allocate from, which gives you 'struct page' backed pages.
 > >
 > > CMA does support Direct I/O, but it has its own issue:
 > > It does not guarantee that the memory previously borrowed by the OS will be returned to the device.
 > >
 > > We've been plagued by examples like this in the past:
 > > Many other kernel modules/subsystems have already allocated much memory from both non-CMA and CMA memory,
 > > When our DSP driver got probed then, cma_alloc will fail in that non-CMA system memory is not enough
 > > for CMA memory to migrate.
 > 
 > This part should at least be possible to solve by declaring the amount
 > and location of
 > CMA areas in the right way. It's not great to fine-tune the DT for a
 > particular kernel's
 > use, but if you know which other drivers require CMA type allocations
 > you can find a lower
 > bound that should suffice.

That's the problem, haha. End users(customers) may modprobe many other modules and modprobe our
driver in the end. We cannot decide the probe order for end users.

Apart from our cases, I heard there are some other cases where cma_alloc failed even non-cma system memory has
enough memory because pages in CMA memory are pinned and cannot move out of CMA. There are some fixes like
1. move these memory out of CMA before pinned
2. only allow non-long-time pinned memory allocation from CMA.

But these two solutions are not merged into the mainline yet.

 > 
 > Most coherent allocations tend to be long-lived and only for very
 > small memory regions.
 > If you have another driver that uses large or periodic
 > dma_alloc_coherent() type allocations,
 > you can consider either giving that device its own CMA area, or fixing
 > it to use streaming
 > mappings.

Device-wise CMA also suffers from the problems I mentioned, other modules/subsystems may have already
alloc from this CMA area and refuse to return it back.

Regards,
Li

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[Arnd Bergmann]

On Thu, Aug 4, 2022 at 12:07 PM Li Chen <me@linux.beauty> wrote:

> Apart from our cases, I heard there are some other cases where cma_alloc
>  failed even non-cma system memory has enough memory because pages in
> CMA memory are pinned and cannot move out of CMA. There are some fixes like
> 1. move these memory out of CMA before pinned
> 2. only allow non-long-time pinned memory allocation from CMA.
>
> But these two solutions are not merged into the mainline yet.

Right, I think this has come up before, not sure why it wasn't implemented.
My feeling is that 2. cannot work because you don't know if memory will be
pinned in the future at the time of allocation, but 1. should be doable.

A possible alternative here would be to avoid the pinning. In most workloads
it should not be possible to pin a large number of pages, but I assume there
is a good reason to do so here.

        Arnd

Re: [PATCH 4/4] sample/reserved_mem: Introduce a sample of struct page and dio support to no-map rmem

[David Hildenbrand]

On 05.08.22 16:09, Arnd Bergmann wrote:
> On Thu, Aug 4, 2022 at 12:07 PM Li Chen <me@linux.beauty> wrote:
> 
>> Apart from our cases, I heard there are some other cases where cma_alloc
>>  failed even non-cma system memory has enough memory because pages in
>> CMA memory are pinned and cannot move out of CMA. There are some fixes like
>> 1. move these memory out of CMA before pinned
>> 2. only allow non-long-time pinned memory allocation from CMA.
>>
>> But these two solutions are not merged into the mainline yet.
> 
> Right, I think this has come up before, not sure why it wasn't implemented.
> My feeling is that 2. cannot work because you don't know if memory will be
> pinned in the future at the time of allocation, but 1. should be doable.

We disallow longterm pinning of CMA memory already and migrate it out of
the CMA region. If migration fails, we reject pinning.

See

9a4e9f3b2d73 ("mm: update get_user_pages_longterm to migrate pages
allocated from CMA region")

and recent

1c563432588d ("mm: fix is_pinnable_page against a cma page")


It's worth nothing that is_pinnable_page() will be renamed to
is_longterm_pinnable_page() soon to express what it actually means.

Note that some FOLL_GET users (vmsplice, O_DIRECT) still have to be
converted to FOLL_PIN, and especially also set FOLL_LONGTERM (vmsplice).

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/4] add struct page and Direct I/O support to reserved memory

[Christoph Hellwig]

Who is going to use it and how?  Because normally the reserved memory
is used through the DMA allocator, and you can't just do direct I/O
to that.

Re: [PATCH 0/4] add struct page and Direct I/O support to reserved memory

[Li Chen]

Hi Christoph,
 ---- On Mon, 11 Jul 2022 23:01:32 +0800  Christoph Hellwig <hch@infradead.org> wrote --- 
 > Who is going to use it and how?  Because normally the reserved memory
 > is used through the DMA allocator, and you can't just do direct I/O
 > to that.
 > 

My use case has been stated in the cover letter, but our driver is not ready for upstream yet.

With DMA allocator, we can access buffer in kernel space, not userspace, however, this patch
series servers for userspace direct I/O, so that you can mmap device file as src buffer from userspace,
and dio to file on disk.


There are some mmap + rmem cases in the kernel source tree which don't use the DMA allocator already.

I also found some users have asked for a solution of supporting direct I/O on file_operation->mmap like:
https://stackoverflow.com/questions/44820740/using-o-direct-with-io-memory
https://www.mail-archive.com/support-list@support.elphel.com/msg02314.html

I believe there are some other potential users who didn't post questions on the internet.

If the upstream kernel has this feature, these users can mmap their
reserved memory to userspace and get direct i/o support.

Regards,
Li

Re: [PATCH 0/4] add struct page and Direct I/O support to reserved memory

[Christoph Hellwig]

On Tue, Jul 12, 2022 at 12:05:06AM +0800, Li Chen wrote:
> My use case has been stated in the cover letter, but our driver is not ready for upstream yet.

Which means we can't review the use case.  I'd suggest you come back
when you submit your driver.

> With DMA allocator, we can access buffer in kernel space, not userspace, however, this patch

Take a look at dma_mmap_* on how to map DMA coherent allocations to
usersapce.  This is of course easily possible.

Re: [PATCH 0/4] add struct page and Direct I/O support to reserved memory

[Li Chen]

Hi Christoph,
 ---- On Tue, 12 Jul 2022 00:09:55 +0800  Christoph Hellwig <hch@infradead.org> wrote --- 
 > On Tue, Jul 12, 2022 at 12:05:06AM +0800, Li Chen wrote:
 > > My use case has been stated in the cover letter, but our driver is not ready for upstream yet.
 > 
 > Which means we can't review the use case.  I'd suggest you come back
 > when you submit your driver.

Totally agree, but we plan to start rewriting the code of our video driver in a long time, it has many legacy codes and I need to rewrite a lot of codes to migrate to v4l2.
That's why I also submit a sample driver here: to make the review progress easier and don't need reviewers to read video driver codes.

 > 
 > > With DMA allocator, we can access buffer in kernel space, not userspace, however, this patch
 > 
 > Take a look at dma_mmap_* on how to map DMA coherent allocations to
 > usersapce.  This is of course easily possible.
 > 

Yes, I know them. They take use of remap_pfn_range, which set VM_IO and VM_PFNMAP on vma, so dio is not possible with them.
IIUC, if you want to expose the kernel's reserved memory to userspace, it doesn't matter whether the memory came from dma allocator or something else.
The point is if they want to get better throughput, they can consider replacing their dma_mmap_* with my reserved_mem_memremap_pages + reserved_mem_dio_mmap.

Regards,
Li