Re: [PATCH] mm/damon/dbgfs: Optimize target_ids interface write operation

[SeongJae Park <sj@xxxxxxxxxx>]

Hello Xin,

On Thu, 21 Oct 2021 16:56:11 +0800 Xin Hao <xhao@xxxxxxxxxxxxxxxxx> wrote:

> When writing some pids to target_ids interface, calling scanf()
> to get 'id' may be failed. If the value of '*nr_ids' is 0 at this time,
> there is no need to return 'ids' here, we just need to release it and
> return NULL pointer to improve related code operation efficiency.

Thank you for the patch!  But, I don't think this patch makes sense, because
the case (*nr_ids == 0) means not error but an ask to remove previously set
target ids.  For example, it works as below now:

    # echo 42 > target_ids
    # cat target_ids
    42
    # echo > target_ids
    # cat target_ids

    #

But, with your patch, the behavior will be changed as below:

    # echo 42 > target_ids
    # cat target_ids
    42
    # echo > target_ids
    bash: echo: write error: Cannot allocate memory
    # cat target_ids
    42
    #

Also, this patch makes the DAMON selftest fails as below:

    $ sudo make -C tools/testing/selftests/damon run_tests
    make: Entering directory '/home/sjpark/linux/tools/testing/selftests/damon'
    TAP version 13
    1..2
    # selftests: damon: debugfs_attrs.sh
    # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
    # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
    # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
    # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
    # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument
    # ./debugfs_attrs.sh: line 11: echo: write error: Cannot allocate memory
    # writing abc 2 3 to /sys/kernel/debug/damon/target_ids doesn't return 0
    # expected because: the file allows wrong input
    not ok 1 selftests: damon: debugfs_attrs.sh # exit=1

If I'm missing something, please let me know.


Thanks,
SJ

> 
> Signed-off-by: Xin Hao <xhao@xxxxxxxxxxxxxxxxx>
> ---
>  mm/damon/dbgfs.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> index a02cf6bee8e8..2d77bf579ffb 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -308,21 +308,25 @@ static unsigned long *str_to_target_ids(const char *str, ssize_t len,
>  	unsigned long *ids;
>  	const int max_nr_ids = 32;
>  	unsigned long id;
> -	int pos = 0, parsed, ret;
> +	int pos = 0, parsed;
>  
>  	*nr_ids = 0;
>  	ids = kmalloc_array(max_nr_ids, sizeof(id), GFP_KERNEL);
>  	if (!ids)
>  		return NULL;
>  	while (*nr_ids < max_nr_ids && pos < len) {
> -		ret = sscanf(&str[pos], "%lu%n", &id, &parsed);
> -		pos += parsed;
> -		if (ret != 1)
> +		if (sscanf(&str[pos], "%lu%n", &id, &parsed) != 1)
>  			break;
> +		pos += parsed;
>  		ids[*nr_ids] = id;
>  		*nr_ids += 1;
>  	}
>  
> +	if (!*nr_ids) {
> +		kfree(ids);
> +		return NULL;
> +	}
> +
>  	return ids;
>  }
>  
> -- 
> 2.31.0