linux-kernel - Re: [PATCH] [RFC] KSM: numa awareness sysfs knob

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20111130154719.57154fdd.akpm@linux-foundation.org>
Date:	Wed, 30 Nov 2011 15:47:19 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Petr Holasek <pholasek@...hat.com>
Cc:	Hugh Dickins <hughd@...gle.com>,
	Andrea Arcangeli <aarcange@...hat.com>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Anton Arapov <anton@...hat.com>
Subject: Re: [PATCH] [RFC] KSM: numa awareness sysfs knob

On Wed, 30 Nov 2011 11:37:26 +0100
Petr Holasek <pholasek@...hat.com> wrote:

> Introduce a new sysfs knob /sys/kernel/mm/ksm/max_node_dist, whose
> value will be used as the limitation for node distance of merged pages.
> 

The changelog doesn't really describe why you think Linux needs this
feature?  What's the reasoning?  Use cases?  What value does it provide?

> index b392e49..b882140 100644
> --- a/Documentation/vm/ksm.txt
> +++ b/Documentation/vm/ksm.txt
> @@ -58,6 +58,10 @@ sleep_millisecs  - how many milliseconds ksmd should sleep before next scan
>                     e.g. "echo 20 > /sys/kernel/mm/ksm/sleep_millisecs"
>                     Default: 20 (chosen for demonstration purposes)
>  
> +max_node_dist    - maximum node distance between two pages which could be
> +                   merged.
> +                   Default: 255 (without any limitations)

And this doesn't explain to our users why they might want to alter it,
and what effects they would see from doing so.  Maybe that's obvious to
them...

>  run              - set 0 to stop ksmd from running but keep merged pages,
>                     set 1 to run ksmd e.g. "echo 1 > /sys/kernel/mm/ksm/run",
>                     set 2 to stop ksmd and unmerge all pages currently merged,
>
> ...
>
> +#ifdef CONFIG_NUMA
> +static inline int node_dist(int from, int to)
> +{
> +	int dist = node_distance(from, to);
> +
> +	return dist == -1 ? 0 : dist;
> +}

So I spent some time grubbing around trying to work out what a return
value of -1 from node_distance() means, and wasn't successful.  Perhaps
an explanatory comment here would have helped.

> +#else
> +static inline int node_dist(int from, int to)
> +{
> +	return 0;
> +}
> +#endif
>
> ...
>
> +static ssize_t max_node_dist_store(struct kobject *kobj,
> +				   struct kobj_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int err;
> +	unsigned long node_dist;
> +
> +	err = kstrtoul(buf, 10, &node_dist);
> +	if (err || node_dist > 255)
> +		return -EINVAL;

If kstrtoul() returned an errno we should propagate that back rather than
overwriting it with -EINVAL.

> +	ksm_node_distance = node_dist;
> +
> +	return count;
> +}
>
> ...
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/