Re: [PATCH RFC 1/2] perf tools: Support single perf.data file directory

[Adrian Hunter]

On 24/09/19 2:12 PM, Jiri Olsa wrote:
> On Tue, Sep 24, 2019 at 12:12:25PM +0300, Adrian Hunter wrote:
>> On 24/09/19 12:34 AM, Jiri Olsa wrote:
>>> On Mon, Sep 16, 2019 at 11:56:45AM +0300, Adrian Hunter wrote:
>>>> Support directory output that contains a regular perf.data file. This is
>>>> preparation for adding support for putting a copy of /proc/kcore in that
>>>> directory.
>>>>
>>>> Distinguish the multiple file case from the regular (single) perf.data file
>>>> case by adding data->is_multi_file.
>>>
>>> SNIP
>>>
>>>>  static int open_file_read(struct perf_data *data)
>>>>  {
>>>>  	struct stat st;
>>>> @@ -302,12 +312,17 @@ static int open_dir(struct perf_data *data)
>>>>  {
>>>>  	int ret;
>>>>  
>>>> -	/*
>>>> -	 * So far we open only the header, so we can read the data version and
>>>> -	 * layout.
>>>> -	 */
>>>> -	if (asprintf(&data->file.path, "%s/header", data->path) < 0)
>>>> -		return -1;
>>>> +	if (perf_data__is_multi_file(data)) {
>>>> +		/*
>>>> +		 * So far we open only the header, so we can read the data version and
>>>> +		 * layout.
>>>> +		 */
>>>> +		if (asprintf(&data->file.path, "%s/header", data->path) < 0)
>>>> +			return -1;
>>>> +	} else {
>>>> +		if (asprintf(&data->file.path, "%s/perf.data", data->path) < 0)
>>>> +			return -1;
>>>> +	}
>>>
>>
>> Thanks for replying :-)
>>
>>> first, please note that there's support for perf.data directory code,
>>> but it's not been enabled yet, so we can do any changes there without
>>> breaking existing users
>>>
>>> currently the logic is prepared to have perf.data DIR_FORMAT feature
>>> to define the layout of the directory
>>>
>>> it'd be great to have just single point where we get directory layout,
>>> not checking on files names first and checking on DIR_FORMAT later
>>
>> Ok, but what are you suggesting?  Naming the data file "header" seems a bit
>> counter-intuitive in this case.
> 
> don't know ;-)

So what about calling it "data" instead of "header"?

> 
> but I'd like to have one way of finding out the directory layout
> 
> the code for threaded record uses DIR_FORMAT feature value
> to ensure the directory contains the expected files, which
> is data file with 'data.<cpu>' name for every cpu
> 
>>
>>>
>>> also the kcore will be beneficial for other layouts,
>>> so would be great to make it somehow optional/switchable
>>
>> In these patches it is, because it is not related to the DIR_FORMAT.
>>
>>> one of the options could be to have DIR_FORMAT feature as the source
>>> of directory layout and it'd have bitmask of files/dirs (like kcore_dir)
>>> available in the directory
>>
>> Is there an advantage to making optional files/dirs part of the format?
>> i.e. if they are there, use them otherwise don't.
> 
> ok, that might work, but please make that somehow explicit/visible
> what files/directories are possible in the directory, so we could
> easily see them and add new ones

At the moment, what can exist is what can be removed i.e. see
rm_rf_perf_data().  Will that do?