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

[Anshuman Khandual <khandual@linux.vnet.ibm.com>]

On 02/23/2017 11:36 AM, Naoya Horiguchi wrote:
> 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?

Sure, will change it up next time around.

> 
>> +
>> +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.)

Sure, will do.

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

Yes.

> If cpumask_weight() is 1, cthreads is 0, that could cause zero division.
> So you had better making sure to prevent it.

If cpumask_weight() is 1, then min_t(unsigned int, 8, 1) should be
greater that equal to 1. Then cthreads can end up in 0. That is
possible. But how there is a chance of zero division ? May be its
possible if we are trying move into a CPU less memory only node
where cpumask_weight() is 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>