On Tue, Sep 24, 2019 at 02:51:28PM +0300, Adrian Hunter wrote:
> 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"?
ok, it actualy contains data in threaded record as well,
so no problem there..
>
> >
> > 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?
ok, but also please some comments in data.h and perf.data doc update ;-)
jirka
