On Fri, Jul 29, 2022 at 11:38:40AM +0200, Daniel Bristot de Oliveira wrote:
> +static int __rv_disable_monitor(struct rv_monitor_def *mdef, bool sync)
> +{
> + lockdep_assert_held(&rv_interface_lock);
> +
> + if (mdef->monitor->enabled) {
> + mdef->monitor->enabled = 0;
> + mdef->monitor->disable();
If call disable(), the @enabled is set 0 there.
> +
> + /*
> + * Wait for the execution of all events to finish.
> + * Otherwise, the data used by the monitor could
> + * be inconsistent. i.e., if the monitor is re-enabled.
> + */
> + if (sync)
> + tracepoint_synchronize_unregister();
> + return 1;
Return 0 indicate the actually disabling and successed.
> + }
> + return 0;
If disable a diabled monitor, return error(negative).
> +}
> +
> +/**
> + * rv_disable_monitor - disable a given runtime monitor
> + *
> + * Returns 0 on success.
> + */
> +int rv_disable_monitor(struct rv_monitor_def *mdef)
> +{
> + __rv_disable_monitor(mdef, true);
> + return 0;
Always return 0 here, whatever the return value of __rv_disable_monitor().
And this enforce me to look more here, see below.
> +}
> +static ssize_t enabled_monitors_write(struct file *filp, const char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + char buff[MAX_RV_MONITOR_NAME_SIZE + 2];
> + struct rv_monitor_def *mdef;
> + int retval = -EINVAL;
> + bool enable = true;
> + char *ptr = buff;
> + int len;
> +
> + if (count < 1 || count > MAX_RV_MONITOR_NAME_SIZE + 1)
> + return -EINVAL;
> +
> + memset(buff, 0, sizeof(buff));
> +
> + retval = simple_write_to_buffer(buff, sizeof(buff) - 1, ppos, user_buf, count);
> + if (retval < 0)
> + return -EFAULT;
> +
> + ptr = strim(buff);
> +
> + if (ptr[0] == '!') {
> + enable = false;
> + ptr++;
> + }
> +
> + len = strlen(ptr);
> + if (!len)
> + return count;
> +
> + mutex_lock(&rv_interface_lock);
> +
> + retval = -EINVAL;
> +
> + list_for_each_entry(mdef, &rv_monitors_list, list) {
> + if (strcmp(ptr, mdef->monitor->name) != 0)
> + continue;
> +
> + /*
> + * Monitor found!
> + */
> + if (enable)
> + retval = rv_enable_monitor(mdef);
> + else
> + retval = rv_disable_monitor(mdef);
About the retval here. If count == 1 and retval == 0, then
`retval = count` --> retval == 1. This retval will be returned to
user space and dedicate that how many character read and success
If retval is 1(it is not possiable, the return value of
da_monitor_init_*() called in enable callback in rv_enable_monitor()
will be 0, so that return value check is not needed, or any other functions
called in enable callback need to check the return value then, so I checked
the WARN_ONCE() called in macro rv_attach_trace_probe() which is called in
enable callback, if the WARN_ONCE is called, it means that something go wrong.
We need to check the return value of WARN_ONCE() in enable callback), the
return value will be returned to user space but actually the error(warn) happened.
User space do not know. They treat the two kind of return value 1 the same
but one is the write count value successed and another is the write error
value returned.
In enable callback, check rv_attach_trace_probe():
static int enable_wip(void)
{
int retval = 1;
/*
* Delete the check of return value of da_monitor_init_wip()
* because it is always 0
*/
da_monitor_init_wip();
retval &= rv_attach_trace_probe("wip", preempt_enable, handle_preempt_enable);
retval &= rv_attach_trace_probe("wip", sched_waking, handle_sched_waking);
retval &= rv_attach_trace_probe("wip", preempt_disable, handle_preempt_disable);
/*
* If the retval is not 0, it mean at least one rv_attach_trace_probe()
* is WARN_ONCE(). I am not sure that if the first WARN_ONCE() happened,
* then return directly or at here after all rv_attach_trace_probe() is
* called and check the retval is 0 or 1.
*/
if (retval)
return -1;
return retval;
}
> +
> + if (!retval)
> + retval = count;
> +
> + break;
> + }
> +/**
> + * rv_register_monitor - register a rv monitor.
> + * @monitor: The rv_monitor to be registered.
> + *
> + * Returns 0 if successful, error otherwise.
> + */
> +int rv_register_monitor(struct rv_monitor *monitor)
> +{
> + struct rv_monitor_def *r;
> + int retval = 0;
> +
> + if (strlen(monitor->name) >= MAX_RV_MONITOR_NAME_SIZE) {
s/>=/>/ no? The same check happened in patch 2. Thanks,
> + pr_info("Monitor %s has a name longer than %d\n", monitor->name,
> + MAX_RV_MONITOR_NAME_SIZE);
