Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/2] perf mmap: Fix perf backward recording
To:     Namhyung Kim <namhyung@kernel.org>
References: <20171101055327.141281-1-wangnan0@huawei.com>
 <20171101055327.141281-2-wangnan0@huawei.com>
 <20171101094929.GB25146@danjae.aot.lge.com>
CC:     <linux-kernel@vger.kernel.org>, <kan.liang@intel.com>,
        <acme@kernel.org>, <jolsa@redhat.com>, <kernel-team@lge.com>
From:   "Wangnan (F)" <wangnan0@huawei.com>
Message-ID: <e6079124-0171-adc4-742f-cd3f9b2d1df4@huawei.com>
Date:   Wed, 1 Nov 2017 18:32:50 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
In-Reply-To: <20171101094929.GB25146@danjae.aot.lge.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 2017/11/1 17:49, Namhyung Kim wrote:
> Hi,
>
> On Wed, Nov 01, 2017 at 05:53:26AM +0000, Wang Nan wrote:
>> perf record backward recording doesn't work as we expected: it never
>> overwrite when ring buffer full.
>>
>> Test:
>>
>> (Run a busy printing python task background like this:
>>
>>   while True:
>>       print 123
>>
>> send SIGUSR2 to perf to capture snapshot.)

[SNIP]

>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> ---
>>   tools/perf/util/evlist.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index c6c891e..4c5daba 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -799,22 +799,28 @@ perf_evlist__should_poll(struct perf_evlist *evlist __maybe_unused,
>>   }
>>   
>>   static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>> -				       struct mmap_params *mp, int cpu_idx,
>> +				       struct mmap_params *_mp, int cpu_idx,
>>   				       int thread, int *_output, int *_output_backward)
>>   {
>>   	struct perf_evsel *evsel;
>>   	int revent;
>>   	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
>> +	struct mmap_params *mp;
>>   
>>   	evlist__for_each_entry(evlist, evsel) {
>>   		struct perf_mmap *maps = evlist->mmap;
>> +		struct mmap_params rdonly_mp;
>>   		int *output = _output;
>>   		int fd;
>>   		int cpu;
>>   
>> +		mp = _mp;
>>   		if (evsel->attr.write_backward) {
>>   			output = _output_backward;
>>   			maps = evlist->backward_mmap;
>> +			rdonly_mp = *_mp;
>> +			rdonly_mp.prot &= ~PROT_WRITE;
>> +			mp = &rdonly_mp;
>>   
>>   			if (!maps) {
>>   				maps = perf_evlist__alloc_mmap(evlist);
>> -- 
> What about this instead (not tested)?
>
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index c6c891e154a6..27ebe355e794 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>                  if (*output == -1) {
>                          *output = fd;
>   
> +                       if (evsel->attr.write_backward)
> +                               mp->prot = PROT_READ;
> +                       else
> +                               mp->prot = PROT_READ | PROT_WRITE;
> +

If evlist->overwrite is true, PROT_WRITE should be unset even if 
write_backward is
not set. If you want to delay the setting of mp->prot, you need to consider
both evlist->overwrite and evsel->attr.write_backward.

Then why not removing mp->prot totally, and add a prot argument to 
perf_mmap__mmap,
since prot setting is always decided independently?

>                          if (perf_mmap__mmap(&maps[idx], mp, *output)  < 0)
>                                  return -1;
>                  } else {
> @@ -1058,9 +1063,7 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
>          struct perf_evsel *evsel;
>          const struct cpu_map *cpus = evlist->cpus;
>          const struct thread_map *threads = evlist->threads;
> -       struct mmap_params mp = {
> -               .prot = PROT_READ | (overwrite ? 0 : PROT_WRITE),
> -       };
> +       struct mmap_params mp;
>   
>          if (!evlist->mmap)
>                  evlist->mmap = perf_evlist__alloc_mmap(evlist);

The 'overwrite' argument in perf_evlist__mmap_ex() controls 
evlist->overwrite.

Thank you.


From 1582856934106290327@xxx Wed Nov 01 09:53:24 +0000 2017
X-GM-THRID: 1582841972981207976
X-Gmail-Labels: Inbox,Category Forums