Re: [PATCH v3 2/3] perf: Userspace event
Hi Peter and Pawel, On Tue, 4 Nov 2014 19:40:31 +0100, Peter Zijlstra wrote: > On Tue, Nov 04, 2014 at 04:42:11PM +, Pawel Moll wrote: >> >> 1. I'm wrong and the record doesn't have to be padded to make it 8 bytes >> aligned. Then I can drop the additional size field. > > No, you're right, we're supposed to stay 8 byte aligned. > >> 2. I could impose a limitation on the prctl API that the data size must >> be 8 bytes aligned. Bad idea in my opinion, I'd rather not. > > Agreed. > >> 3. The additional size (for the data part) field stays. Notice that >> PERF_SAMPLE_RAW has it as well :-) > > Right, with binary data there is no other day. With \0 terminated > strings there won't be a problem, but I think we decided we wanted to > allow any binary blow. Ah, I missed that. Thank you guys for explanation. Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/3] perf: Userspace event
On Tue, Nov 04, 2014 at 04:42:11PM +, Pawel Moll wrote: > > 1. I'm wrong and the record doesn't have to be padded to make it 8 bytes > aligned. Then I can drop the additional size field. No, you're right, we're supposed to stay 8 byte aligned. > 2. I could impose a limitation on the prctl API that the data size must > be 8 bytes aligned. Bad idea in my opinion, I'd rather not. Agreed. > 3. The additional size (for the data part) field stays. Notice that > PERF_SAMPLE_RAW has it as well :-) Right, with binary data there is no other day. With \0 terminated strings there won't be a problem, but I think we decided we wanted to allow any binary blow. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/3] perf: Userspace event
On Tue, 2014-11-04 at 06:33 +, Namhyung Kim wrote:
> Hi Pawel,
>
> On Tue, 4 Nov 2014 00:28:37 +, Pawel Moll wrote:
> > + /*
> > +* Data in userspace event record is transparent for the kernel
> > +*
> > +* Userspace perf tool code maintains a list of known types with
> > +* reference implementations of parsers for the data field.
> > +*
> > +* Overall size of the record (including type and size fields)
> > +* is always aligned to 8 bytes by adding padding after the data.
> > +*
> > +* struct {
> > +* struct perf_event_headerheader;
> > +* u32 type;
> > +* u32 size;
>
> The struct perf_event_header also has 'size' field and it has the entire
> length of the record so it's redundant.
Well, is it? Correct me if I'm wrong, but as far as I remember the
record size must be always aligned to 8 bytes. Thus you can't reliably
derive the data size from it - if I my data is 3 bytes long, I have to
add 5 bytes of padding thus making the header.size = 24 (I'm ignoring
sample_id here), right? So now, decoding the record, all I can do is:
header.size - sizeof(header) - sizeof(type) - sizeof(size) = 24 - 8 - 8
= 8. So, basing on the header.size the data is 8 bytes long. But only 3
first bytes are valid...
To summarize, there are three options:
1. I'm wrong and the record doesn't have to be padded to make it 8 bytes
aligned. Then I can drop the additional size field.
2. I could impose a limitation on the prctl API that the data size must
be 8 bytes aligned. Bad idea in my opinion, I'd rather not.
3. The additional size (for the data part) field stays. Notice that
PERF_SAMPLE_RAW has it as well :-)
> Also there's 'misc' field in the
> perf_event_header and I guess it can be used as 'type' info as it's
> mostly for cpumode and we are in user mode by definition.
Hm. First of all, I don't really like the idea of "overloading" the misc
meaning. It's a set of flags and I'd rather see it staying like this.
Secondly, I'm not sure that it can be reused - we are in user mode,
true, but it can be either PERF_RECORD_MISC_USER or
PERF_RECORD_MISC_GUEST_USER.
Thirdly, misc is "only" 16 bits wide, and someone even asked for the
type to be 64 bit long! (I suspect he wanted to use it in some special,
hacky way though :-) 32 bit length seems like a reasonable choice,
though.
Do you feel that the "unnecessary" type field is a big problem?
Thanks for your time!
Pawel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/3] perf: Userspace event
On Tue, 2014-11-04 at 06:33 +, Namhyung Kim wrote:
Hi Pawel,
On Tue, 4 Nov 2014 00:28:37 +, Pawel Moll wrote:
+ /*
+* Data in userspace event record is transparent for the kernel
+*
+* Userspace perf tool code maintains a list of known types with
+* reference implementations of parsers for the data field.
+*
+* Overall size of the record (including type and size fields)
+* is always aligned to 8 bytes by adding padding after the data.
+*
+* struct {
+* struct perf_event_headerheader;
+* u32 type;
+* u32 size;
The struct perf_event_header also has 'size' field and it has the entire
length of the record so it's redundant.
Well, is it? Correct me if I'm wrong, but as far as I remember the
record size must be always aligned to 8 bytes. Thus you can't reliably
derive the data size from it - if I my data is 3 bytes long, I have to
add 5 bytes of padding thus making the header.size = 24 (I'm ignoring
sample_id here), right? So now, decoding the record, all I can do is:
header.size - sizeof(header) - sizeof(type) - sizeof(size) = 24 - 8 - 8
= 8. So, basing on the header.size the data is 8 bytes long. But only 3
first bytes are valid...
To summarize, there are three options:
1. I'm wrong and the record doesn't have to be padded to make it 8 bytes
aligned. Then I can drop the additional size field.
2. I could impose a limitation on the prctl API that the data size must
be 8 bytes aligned. Bad idea in my opinion, I'd rather not.
3. The additional size (for the data part) field stays. Notice that
PERF_SAMPLE_RAW has it as well :-)
Also there's 'misc' field in the
perf_event_header and I guess it can be used as 'type' info as it's
mostly for cpumode and we are in user mode by definition.
Hm. First of all, I don't really like the idea of overloading the misc
meaning. It's a set of flags and I'd rather see it staying like this.
Secondly, I'm not sure that it can be reused - we are in user mode,
true, but it can be either PERF_RECORD_MISC_USER or
PERF_RECORD_MISC_GUEST_USER.
Thirdly, misc is only 16 bits wide, and someone even asked for the
type to be 64 bit long! (I suspect he wanted to use it in some special,
hacky way though :-) 32 bit length seems like a reasonable choice,
though.
Do you feel that the unnecessary type field is a big problem?
Thanks for your time!
Pawel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/3] perf: Userspace event
On Tue, Nov 04, 2014 at 04:42:11PM +, Pawel Moll wrote: 1. I'm wrong and the record doesn't have to be padded to make it 8 bytes aligned. Then I can drop the additional size field. No, you're right, we're supposed to stay 8 byte aligned. 2. I could impose a limitation on the prctl API that the data size must be 8 bytes aligned. Bad idea in my opinion, I'd rather not. Agreed. 3. The additional size (for the data part) field stays. Notice that PERF_SAMPLE_RAW has it as well :-) Right, with binary data there is no other day. With \0 terminated strings there won't be a problem, but I think we decided we wanted to allow any binary blow. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/3] perf: Userspace event
Hi Peter and Pawel, On Tue, 4 Nov 2014 19:40:31 +0100, Peter Zijlstra wrote: On Tue, Nov 04, 2014 at 04:42:11PM +, Pawel Moll wrote: 1. I'm wrong and the record doesn't have to be padded to make it 8 bytes aligned. Then I can drop the additional size field. No, you're right, we're supposed to stay 8 byte aligned. 2. I could impose a limitation on the prctl API that the data size must be 8 bytes aligned. Bad idea in my opinion, I'd rather not. Agreed. 3. The additional size (for the data part) field stays. Notice that PERF_SAMPLE_RAW has it as well :-) Right, with binary data there is no other day. With \0 terminated strings there won't be a problem, but I think we decided we wanted to allow any binary blow. Ah, I missed that. Thank you guys for explanation. Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/3] perf: Userspace event
Hi Pawel,
On Tue, 4 Nov 2014 00:28:37 +, Pawel Moll wrote:
> + /*
> + * Data in userspace event record is transparent for the kernel
> + *
> + * Userspace perf tool code maintains a list of known types with
> + * reference implementations of parsers for the data field.
> + *
> + * Overall size of the record (including type and size fields)
> + * is always aligned to 8 bytes by adding padding after the data.
> + *
> + * struct {
> + * struct perf_event_headerheader;
> + * u32 type;
> + * u32 size;
The struct perf_event_header also has 'size' field and it has the entire
length of the record so it's redundant. Also there's 'misc' field in the
perf_event_header and I guess it can be used as 'type' info as it's
mostly for cpumode and we are in user mode by definition.
Thanks,
Namhyung
> + * chardata[size];
> + * char__padding[-size & 7];
> + * struct sample_idsample_id;
> + * };
> + */
> + PERF_RECORD_UEVENT = 11,
> +
> PERF_RECORD_MAX,/* non-ABI */
> };
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/3] perf: Userspace event
Hi Pawel,
On Tue, 4 Nov 2014 00:28:37 +, Pawel Moll wrote:
+ /*
+ * Data in userspace event record is transparent for the kernel
+ *
+ * Userspace perf tool code maintains a list of known types with
+ * reference implementations of parsers for the data field.
+ *
+ * Overall size of the record (including type and size fields)
+ * is always aligned to 8 bytes by adding padding after the data.
+ *
+ * struct {
+ * struct perf_event_headerheader;
+ * u32 type;
+ * u32 size;
The struct perf_event_header also has 'size' field and it has the entire
length of the record so it's redundant. Also there's 'misc' field in the
perf_event_header and I guess it can be used as 'type' info as it's
mostly for cpumode and we are in user mode by definition.
Thanks,
Namhyung
+ * chardata[size];
+ * char__padding[-size 7];
+ * struct sample_idsample_id;
+ * };
+ */
+ PERF_RECORD_UEVENT = 11,
+
PERF_RECORD_MAX,/* non-ABI */
};
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
