Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 3FC4EC433F1
Subject: Re: [PATCH] trace: Fix race in trace_open and buffer resize call
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     mingo@redhat.com, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org
References: <1599199797-25978-1-git-send-email-gkohli@codeaurora.org>
 <d4691a90-9a47-b946-f2cd-bb1fce3981b0@codeaurora.org>
 <2fe2a843-e2b5-acf8-22e4-7231d24a9382@codeaurora.org>
 <20200915092353.5b805468@gandalf.local.home>
From:   Gaurav Kohli <gkohli@codeaurora.org>
Message-ID: <cc65142f-3896-588e-b0bd-52c0329d6175@codeaurora.org>
Date:   Tue, 15 Sep 2020 23:35:13 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <20200915092353.5b805468@gandalf.local.home>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Sorry for spam, saw some failure so sending mail again.

On 9/15/2020 6:53 PM, Steven Rostedt wrote:
 > On Tue, 15 Sep 2020 10:38:03 +0530
 > Gaurav Kohli <gkohli@codeaurora.org> wrote:
 >
 >>
 >>   >>> +void ring_buffer_mutex_release(struct trace_buffer *buffer)
 >>   >>> +{
 >>   >>> +    mutex_unlock(&buffer->mutex);
 >>   >>> +}
 >>   >>> +EXPORT_SYMBOL_GPL(ring_buffer_mutex_release);
 >>   >
 >>   > I really do not like to export these.
 >>   >
 >>
 >> Actually available reader lock is not helping
 >> here(&cpu_buffer->reader_lock), So i took ring buffer mutex lock to
 >> resolve this(this came on 4.19/5.4), in latest tip it is trace buffer
 >> lock. Due to this i have exported api.
 >
 > I'm saying, why don't you take the buffer->mutex in the
 > ring_buffer_reset_online_cpus() function? And remove all the 
protection in
 > tracing_reset_online_cpus()?

Yes, got your point. then we can avoid export. Actually we are seeing 
issue in older kernel like 4.19/4.14/5.4 and there below patch is not 
present in stable branches:

ommit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
 > avoiding synchronize_rcu for each CPU")

Actually i have also thought to take mutex lock in ring_buffer_reset_cpu
while doing individual cpu reset, but this could cause another problem:

Different cpu buffer may have different state, so i have taken lock in 
tracing_reset_online_cpus.
 >
 > void tracing_reset_online_cpus(struct array_buffer *buf)
 > {
 >     struct trace_buffer *buffer = buf->buffer;
 >
 >     if (!buffer)
 >         return;
 >
 >     buf->time_start = buffer_ftrace_now(buf, buf->cpu);
 >
 >     ring_buffer_reset_online_cpus(buffer);
 > }
 >
 > The reset_online_cpus() is already doing the synchronization, we 
don't need
 > to do it twice.
 >
 > I believe commit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
 > avoiding synchronize_rcu for each CPU") made the synchronization in
 > tracing_reset_online_cpus() obsolete.
 >
 > -- Steve
 >

Yes, with above patch no need to take lock in tracing_reset_online_cpus.
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.