linux-kernel - Re: [PATCH] coresight: etm_perf: Rework alloc/free methods to avoid lockep warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANLsYkw5eXvVg+8QGBFsQ_kawL-Okuny-e4VPY=q6w3_27Dq_Q@mail.gmail.com>
Date:   Wed, 18 Jul 2018 16:11:16 -0600
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     "Suzuki K. Poulose" <suzuki.poulose@....com>
Cc:     linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Subject: Re: [PATCH] coresight: etm_perf: Rework alloc/free methods to avoid
 lockep warning

On Wed, 18 Jul 2018 at 15:31, Suzuki K Poulose <suzuki.poulose@....com> wrote:
>
> 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.

Not only did you beat me to the punch but I think using a per-cpu
variable is cleaner, so let's go with yours.

Mathieu

>
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@...aro.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