Re: [PATCH -next] mm: introduce per-node proactive reclaim interface

[Davidlohr Bueso <dave@stgolabs.net>]

On Wed, 04 Sep 2024, Andrew Morton wrote:\n
>On Wed,  4 Sep 2024 09:27:40 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> This adds support for allowing proactive reclaim in general on a
>> NUMA system. A per-node interface extends support for beyond a
>> memcg-specific interface, respecting the current semantics of
>> memory.reclaim: respecting aging LRU and not supporting
>> artificially triggering eviction on nodes belonging to non-bottom
>> tiers.
>>
>> This patch allows userspace to do:
>>
>>      echo 512M swappiness=10 > /sys/devices/system/node/nodeX/reclaim
>
>One value per sysfs file is a rule.

I wasn't aware of it as a rule - is this documented somewhere?

I ask because I see some others are using space-separated parameters, ie:

/sys/bus/usb/drivers/foo/new_id

... or colons. What would be acceptable? echo "512M:10" > ... ?

>> +What:		/sys/devices/system/node/nodeX/reclaim
>> +Date:		September 2024
>> +Contact:	Linux Memory Management list <linux-mm@kvack.org>
>> +Description:
>> +		This is write-only nested-keyed file which accepts the number of
>
>"is a write-only".
>
>What does "nested keyed" mean?

Will re-phrase.

>
>> +		bytes to reclaim as well as the swappiness for this particular
>> +		operation. Write the amount of bytes to induce memory reclaim in
>> +		this node. When it completes successfully, the specified amount
>> +		or more memory will have been reclaimed, and -EAGAIN if less
>> +		bytes are reclaimed than the specified amount.
>
>Could be that this feature would benefit from a more expansive
>treatment under Documentation/somewhere.

Sure.

>
>>
>> ...
>>
>> +#if defined(CONFIG_SYSFS) && defined(CONFIG_NUMA)
>> +
>> +enum {
>> +	MEMORY_RECLAIM_SWAPPINESS = 0,
>> +	MEMORY_RECLAIM_NULL,
>> +};
>> +
>> +static const match_table_t tokens = {
>> +	{ MEMORY_RECLAIM_SWAPPINESS, "swappiness=%d"},
>> +	{ MEMORY_RECLAIM_NULL, NULL },
>> +};
>> +
>> +static ssize_t reclaim_store(struct device *dev,
>> +			     struct device_attribute *attr,
>> +			     const char *buf, size_t count)
>> +{
>> +	int nid = dev->id;
>> +	gfp_t gfp_mask = GFP_KERNEL;
>> +	struct pglist_data *pgdat = NODE_DATA(nid);
>> +	unsigned long nr_to_reclaim, nr_reclaimed = 0;
>> +	unsigned int nr_retries = MAX_RECLAIM_RETRIES;
>> +	int swappiness = -1;
>> +	char *old_buf, *start;
>> +	substring_t args[MAX_OPT_ARGS];
>> +	struct scan_control sc = {
>> +		.gfp_mask = current_gfp_context(gfp_mask),
>> +		.reclaim_idx = gfp_zone(gfp_mask),
>> +		.priority = DEF_PRIORITY,
>> +		.may_writepage = !laptop_mode,
>> +		.may_unmap = 1,
>> +		.may_swap = 1,
>> +		.proactive = 1,
>> +	};
>> +
>> +	buf = strstrip((char *)buf);
>> +
>> +	old_buf = (char *)buf;
>> +	nr_to_reclaim = memparse(buf, (char **)&buf) / PAGE_SIZE;
>> +	if (buf == old_buf)
>> +		return -EINVAL;
>> +
>> +	buf = strstrip((char *)buf);
>> +
>> +	while ((start = strsep((char **)&buf, " ")) != NULL) {
>> +		if (!strlen(start))
>> +			continue;
>> +		switch (match_token(start, tokens, args)) {
>> +		case MEMORY_RECLAIM_SWAPPINESS:
>> +			if (match_int(&args[0], &swappiness))
>> +				return -EINVAL;
>> +			if (swappiness < MIN_SWAPPINESS || swappiness > MAX_SWAPPINESS)
>> +				return -EINVAL;
>
>Code forgot to use local `swappiness' for any purpose?

Bleh, yeah sc.proactive_swappiness needs to be set here.

>
>> +			break;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>>
>> ...
>>