Re: [PATCH v29 07/13] mm/damon: Implement a debugfs-based user space interface

[SeongJae Park <sj38.park@gmail.com>]

Thank you for the comments!

On Fri, 11 Jun 2021 20:59:04 +0200 <sieberf@amazon.com> wrote:

> + Returns non-empty string on success, negarive error code otherwise.
> 
> Nit: typo, 'negarive'

Good catch!  I will fix this in the next spin.

> 
> 
> +    if (ctx->kdamond) {
> +        if (targetid_is_pid(ctx))
> +            dbgfs_put_pids(targets, nr_targets);
> +        ret = -EBUSY;
> +        goto unlock_out;
> 
> I don't understand this block, what does EBUSY mean in this context?

That means DAMON is busy because someone already requested running the
monitoring context.  So, the user will be able to wait a little bit and try
later, or try to figure out who requested the monitoring.

> 
> +    if (sscanf(kbuf, "%s", kbuf) != 1) {
> +        kfree(kbuf);
> +        return -EINVAL;
> +    }
> 
> Nit, this diverts from other patterns use elsewhere where error 
> conditions that need to free memory use a goto label.

I want to use 'goto' only if it makes the code shorter and easier to read.  In
this case, this is the only one case that could use the 'goto' statement.  And,
this part would still be two lines like below:

    if (sscanf(kbuf, "%s", kbuf) != 1) {
    	ret = -EINVAL;
    	goto out;
    }

Meanwhile, to use the 'goto', we would need to add the label below, so we will
only add one more line.  Therefore, I think using 'goto' here doesn't really
reduce the lines of code or make it easier to read.

So, if you don't care, I'd like to use current code as is.

If I missed something, please let me know.


Thanks,
SeongJae Park