This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH v2 03/13] btrace, linux: add perf event buffer abstraction
- From: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Tue, 27 Jan 2015 15:31:06 +0000
- Subject: RE: [PATCH v2 03/13] btrace, linux: add perf event buffer abstraction
- Authentication-results: sourceware.org; auth=none
- References: <1416480444-9943-1-git-send-email-markus dot t dot metzger at intel dot com> <1416480444-9943-4-git-send-email-markus dot t dot metzger at intel dot com> <54C773CE dot 8040708 at redhat dot com>
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Tuesday, January 27, 2015 12:18 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 03/13] btrace, linux: add perf event buffer abstraction
>
> On 11/20/2014 10:47 AM, Markus Metzger wrote:
>
> > - volatile struct perf_event_mmap_page *header;
> > + struct perf_event_buffer *pevent;
> > const uint8_t *begin, *end, *start;
> > - unsigned long data_head, data_tail, retries = 5;
> > - size_t buffer_size, size;
> > + unsigned long long data_head, data_tail, buffer_size, size;
> > + size_t retries = 5;
>
> I notice that this is changing types from long to long long.
> Also, a bit odd that retries is now size_t, given it's just a counter.
I changed retries to unsigned int.
data_head and data_tail correspond to respective fields in
struct perf_event_mmap_page, a kernel data structure.
buffer_size corresponds to the data_size field in the same
struct.
I can't say why they had been unsigned long before. It was likely
a bug.
> > +#if HAVE_LINUX_PERF_EVENT_H
> > +/* A Linux perf event buffer. */
> > +struct perf_event_buffer
> > +{
> > + /* The mapped memory. */
> > + const uint8_t *mem;
> > +
> > + /* The size of the mapped memory in bytes. */
> > + unsigned long long size;
> > +
> > + /* A pointer to the data_head field for this buffer. */
> > + volatile unsigned long long *data_head;
> > +
> > + /* The data_head value from the last read. */
> > + unsigned long long last_head;
> > +};
>
> Isn't there a better type to use here instead of "long long"?
> Why not size_t, if a host buffer size; uint64_t, if it's a fixed
> buffer format, assuming 64-bit here; or ULONGEST, the widest
> integer we support?
I'm using the same data type as the respective fields in
struct perf_event_mmap_page (after resolving a few typedefs).
I'm also fine to directly use __u64 or to use uint64_t.
Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052