Re: [RFC PATCH 03/14] mm/migrate: Add copy_pages_mthread function

[Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>]

On Fri, Feb 17, 2017 at 10:05:40AM -0500, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
> 
> This change adds a new function copy_pages_mthread to enable multi threaded
> page copy which can be utilized during migration. This function splits the
> page copy request into multiple threads which will handle individual chunk
> and send them as jobs to system_highpri_wq work queue.
> 
> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> ---
>  include/linux/highmem.h |  2 ++
>  mm/Makefile             |  2 ++
>  mm/copy_pages.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 90 insertions(+)
>  create mode 100644 mm/copy_pages.c
> 
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index bb3f3297062a..e1f4f1b82812 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -236,6 +236,8 @@ static inline void copy_user_highpage(struct page *to, struct page *from,
>  
>  #endif
>  
> +int copy_pages_mthread(struct page *to, struct page *from, int nr_pages);
> +
>  static inline void copy_highpage(struct page *to, struct page *from)
>  {
>  	char *vfrom, *vto;
> diff --git a/mm/Makefile b/mm/Makefile
> index aa0aa17cb413..cdd4bab9cc66 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -43,6 +43,8 @@ obj-y			:= filemap.o mempool.o oom_kill.o \
>  
>  obj-y += init-mm.o
>  
> +obj-y += copy_pages.o
> +
>  ifdef CONFIG_NO_BOOTMEM
>  	obj-y		+= nobootmem.o
>  else
> diff --git a/mm/copy_pages.c b/mm/copy_pages.c
> new file mode 100644
> index 000000000000..c357e7b01042
> --- /dev/null
> +++ b/mm/copy_pages.c
> @@ -0,0 +1,86 @@
> +/*
> + * This implements parallel page copy function through multi threaded
> + * work queues.
> + *
> + * Zi Yan <ziy@nvidia.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + */
> +#include <linux/highmem.h>
> +#include <linux/workqueue.h>
> +#include <linux/slab.h>
> +#include <linux/freezer.h>
> +
> +/*
> + * nr_copythreads can be the highest number of threads for given node
> + * on any architecture. The actual number of copy threads will be
> + * limited by the cpumask weight of the target node.
> + */
> +unsigned int nr_copythreads = 8;

If you give this as a constant, how about defining as macro?

> +
> +struct copy_info {
> +	struct work_struct copy_work;
> +	char *to;
> +	char *from;
> +	unsigned long chunk_size;
> +};
> +
> +static void copy_pages(char *vto, char *vfrom, unsigned long size)
> +{
> +	memcpy(vto, vfrom, size);
> +}
> +
> +static void copythread(struct work_struct *work)
> +{
> +	struct copy_info *info = (struct copy_info *) work;
> +
> +	copy_pages(info->to, info->from, info->chunk_size);
> +}
> +
> +int copy_pages_mthread(struct page *to, struct page *from, int nr_pages)
> +{
> +	unsigned int node = page_to_nid(to);
> +	const struct cpumask *cpumask = cpumask_of_node(node);
> +	struct copy_info *work_items;
> +	char *vto, *vfrom;
> +	unsigned long i, cthreads, cpu, chunk_size;
> +	int cpu_id_list[32] = {0};

Why 32? Maybe you can set the array size with nr_copythreads (macro version.)

> +
> +	cthreads = nr_copythreads;
> +	cthreads = min_t(unsigned int, cthreads, cpumask_weight(cpumask));

nitpick, but looks a little wordy, can it be simply like below?

  cthreads = min_t(unsigned int, nr_copythreads, cpumask_weight(cpumask));

> +	cthreads = (cthreads / 2) * 2;

I'm not sure the intention here. # of threads should be even number?
If cpumask_weight() is 1, cthreads is 0, that could cause zero division.
So you had better making sure to prevent it.

Thanks,
Naoya Horiguchi

> +	work_items = kcalloc(cthreads, sizeof(struct copy_info), GFP_KERNEL);
> +	if (!work_items)
> +		return -ENOMEM;
> +
> +	i = 0;
> +	for_each_cpu(cpu, cpumask) {
> +		if (i >= cthreads)
> +			break;
> +		cpu_id_list[i] = cpu;
> +		++i;
> +	}
> +
> +	vfrom = kmap(from);
> +	vto = kmap(to);
> +	chunk_size = PAGE_SIZE * nr_pages / cthreads;
> +
> +	for (i = 0; i < cthreads; ++i) {
> +		INIT_WORK((struct work_struct *) &work_items[i], copythread);
> +
> +		work_items[i].to = vto + i * chunk_size;
> +		work_items[i].from = vfrom + i * chunk_size;
> +		work_items[i].chunk_size = chunk_size;
> +
> +		queue_work_on(cpu_id_list[i], system_highpri_wq,
> +					  (struct work_struct *) &work_items[i]);
> +	}
> +
> +	for (i = 0; i < cthreads; ++i)
> +		flush_work((struct work_struct *) &work_items[i]);
> +
> +	kunmap(to);
> +	kunmap(from);
> +	kfree(work_items);
> +	return 0;
> +}
> -- 
> 2.11.0
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>