lkml.org 
[lkml]   [2018]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] coresight: etm_perf: Rework alloc/free methods to avoid lockep warning
From
Date
Hi Mathieu,

On 07/18/2018 08:43 PM, Mathieu Poirier wrote:
> When enabling the lockdep mechanic and working with CPU-wide scenarios we
> get the following console output:
>

> This is fixed by working with the cpu_present_mask, avoinding at the same
> the need to use get/put_online_cpus() that triggers lockdep.

The patch looks correct to me. In fact I have a patch [1], which
does the same thing and switches to using per-cpu variable for the
paths.

>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
> drivers/hwtracing/coresight/coresight-etm-perf.c | 39 +++++++++++++-----------
> 1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 677695635211..16b9c87d9d00 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -108,26 +108,32 @@ static int etm_event_init(struct perf_event *event)
> static void free_event_data(struct work_struct *work)
> {
> int cpu;
> + void *snk_config;
> cpumask_t *mask;
> struct etm_event_data *event_data;
> struct coresight_device *sink;
>
> event_data = container_of(work, struct etm_event_data, work);
> mask = &event_data->mask;
> - /*
> - * First deal with the sink configuration. See comment in
> - * etm_setup_aux() about why we take the first available path.
> - */
> - if (event_data->snk_config) {
> - cpu = cpumask_first(mask);
> - sink = coresight_get_sink(event_data->path[cpu]);
> - if (sink_ops(sink)->free_buffer)
> - sink_ops(sink)->free_buffer(event_data->snk_config);
> - }
> + snk_config = event_data->snk_config;
>
> for_each_cpu(cpu, mask) {
> - if (!(IS_ERR_OR_NULL(event_data->path[cpu])))
> - coresight_release_path(event_data->path[cpu]);
> + if (IS_ERR_OR_NULL(event_data->path[cpu]))
> + continue;
> +
> + /*
> + * Free sink configuration - there can only be one sink
> + * per event.
> + */
> + if (snk_config) {
> + sink = coresight_get_sink(event_data->path[cpu]);
> + if (sink_ops(sink)->free_buffer) {
> + sink_ops(sink)->free_buffer(snk_config);
> + snk_config = NULL;
> + }
> + }
> +
> + coresight_release_path(event_data->path[cpu]);
> }

>
> kfree(event_data->path);
> @@ -145,16 +151,13 @@ static void *alloc_event_data(int cpu)
> if (!event_data)
> return NULL;
>
> - /* Make sure nothing disappears under us */
> - get_online_cpus();
> - size = num_online_cpus();
> -
> mask = &event_data->mask;
> + size = num_present_cpus();
> +
> if (cpu != -1)
> cpumask_set_cpu(cpu, mask);
> else
> - cpumask_copy(mask, cpu_online_mask);
> - put_online_cpus();
> + cpumask_copy(mask, cpu_present_mask);

I think it is safe to use cpu_online_mask outside the
get/put_online_cpus(). Using present_mask may fail as
we could fail to create a path for a CPU that is not online.


Please could you check if [1] fixes the problem for you ?

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/591606.html

Cheers
Suzuki

\
 
 \ /
  Last update: 2018-07-18 23:32    [W:0.217 / U:0.468 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site