[PATCH v6 4/7] start/stop btrace with coresight etm and collect etm buffer on linux os
Zied Guermazi
zied.guermazi@trande.de
Mon Jul 18 19:06:00 GMT 2022
hello Markus,
the proposal solves the issue for scoped_fd, we can use it and create a
second scoped_fd using the returned file descriptor since scoped_fd has
this constructor
explicit scoped_fd (int fd = -1) noexcept : m_fd (fd) {}.
for scoped_mmap we can not do the same since the class is missing a
constructor taking void *mem and size_t length as arguments.
will it be fine to add such a constructor to the class? will it be
enough to assign the memory pointer and the length to the class members?
Kind Regards
Zied Guermazi
On 20.06.22 14:52, Metzger, Markus T wrote:
>
> Hello Zied,
>
> /* open the scoped fd. */
>
> static scoped_fd *
> perf_event_open ( const struct perf_event_attr *event_attributes,
> const int pid)
>
> The purpose of those scoped_ classes is to allocated objects on the
> stack to have them destroy their content in case of exceptions.
>
> In the above example, perf_event_open() may use a scoped_fd internally
> but would return the actual file descriptor after releasing it. The
> caller may put the returned file descriptor into its own scoped_fd
> object on its stack.
>
> regards,
>
> markus.
>
> /* open the scoped fd. */
>
> static scoped_fd *
> perf_event_open ( const struct perf_event_attr *event_attributes,
> const int pid)
>
> /*Open the scoped mmap data */
>
> static scoped_mmap*
> perf_event_mmap_data (const scoped_fd* fd, size_t* size, size_t page_size)
>
> and
>
> /*Open the scoped mmap aux */
>
> static scoped_mmap*
> perf_event_mmap_aux (const scoped_fd* fd, struct perf_event_mmap_page
> *header, size_t* size, size_t page_size)
>
> and call them within linux_enable_bts, linux_enable_pt and
> linux_enable_etm
>
> I tried two alternatives and both of them failed:
>
> - Alternative 1: instantiate the returned scoped_mmap on the stack of
> the perf_event_mmap_data and perf_event_mmap_aux: the compiler refused
> to assign the scoped_mmap variables and they will not be valid anymore
> in the linux_enable_bts, linux_enable_pt and linux_enable_etm
>
> - Alternative 2: instantiate the returned scoped_mmap on the heap by
> allocating them. the compiler compiles but, once we stop the tracing
> we ca can not start it again cause the resources are busy.
>
> for the time being I will put this refactoring action on hold.
>
> Any idea or support to progress further are welcome
>
> Kind Regards
>
> Zied Guermazi
>
> On 13.05.22 07:31, Metzger, Markus T wrote:
>
> Hello Zied,
>
> +cs_etm_get_register (int cpu, const char *path)
>
> +{
>
> [...]
>
> +
>
> + uint32_t val = 0;
>
> +
>
> + int found = fscanf (file.get (), "0x%x", &val);
>
> + if (found != 1)
>
> + error (_("Failed to read coresight register from %s."), filename);
>
> + return val;
>
>
>
> [Zied] existing code in this file has an empty line before the
> last return everywhere. shall I stick to this convention?
> eliminating the empty lines before returns will bring
> inconsistency in the file. which coding convention do we have to
> apply here?
>
>
> Yes, let’s stick to that and add the empty line.
>
> so both perf_event_mmap_data perf_event_mmap_aux can be in fact
> reduced to one function if we give size_t size, size_t page_size,
> int offset as parameters.
>
> BTS allocates the data buffer including the header, whereas PT
> allocates the aux buffer plus the header. We can still make it
> one function, of course, but I don’t think that it will be any
> easier to read that way.
>
> 2- how to bring this change to the mainstream. here we have two
> alternatives : either I do this restructuring for etm only and
> then you take care of using the helper functions for bts and pt or
> I commit it as it is and then I issue a patch for this point only
> for bts PT and ETM. What do you prefer?
>
> Ideally, we’d first restructure the existing code, then add the
> new use. Are you able to test BTS and PT?
>
> Regards,
>
> Markus.
>
> *From:* Zied Guermazi <zied.guermazi@trande.de>
> <mailto:zied.guermazi@trande.de>
> *Sent:* Friday, May 13, 2022 12:52 AM
> *To:* Metzger, Markus T <markus.t.metzger@intel.com>
> <mailto:markus.t.metzger@intel.com>; gdb-patches@sourceware.org
> *Subject:* Re: [PATCH v6 4/7] start/stop btrace with coresight etm
> and collect etm buffer on linux os
>
> Hello Markus,
>
> thanks for your feedback, below are the reworking comments.
>
> /Zied
>
> On 23.06.21 10:00, Metzger, Markus T wrote:
>
> Hello Zied,
>
>
>
> This patch implement the lower layer for starting ad stopping
>
> ARM CoreSight tracing on linux targets for arm and aarch64
>
>
>
> The patch looks good overall. There are a few style nits and I'd ask you to
>
> split the PAGE_SIZE changes into a separate patch as they are unrelated.
>
>
>
> Then, there's the discussion about sharing perf_event buffer mapping.
>
> I pointed out which parts I believe can be shared.
>
>
>
>
>
> +/* Teardown branch tracing. */
>
> +
>
> +void
>
> +arm_linux_nat_target::teardown_btrace (struct btrace_target_info *tinfo)
>
> +{
>
> + /* Ignore errors. */
>
> + linux_disable_btrace (tinfo);
>
> +}
>
> +
>
> +enum btrace_error
>
> +arm_linux_nat_target::read_btrace (struct btrace_data *data,
>
> + struct btrace_target_info *btinfo,
>
> + enum btrace_read_type type)
>
> +{
>
> + return linux_read_btrace (data, btinfo, type);
>
> +}
>
> +
>
> +/* See to_btrace_conf in target.h. */
>
> +
>
> +const struct btrace_config *
>
> +arm_linux_nat_target::btrace_conf (const struct btrace_target_info *btinfo)
>
> +{
>
> + return linux_btrace_conf (btinfo);
>
> +}
>
>
>
> There's some inconsistency in comments on functions ranging from no comment
>
> over referring to the original target struct, to an own comment.
>
> [Zied] I will align the comments. please notice that the same
> applies to x86-linux-nat.c (it was a copy-paste from it)
>
>
>
>
>
>
>
>
> @@ -483,10 +487,11 @@ linux_enable_bts (ptid_t ptid, const struct
>
> btrace_config_bts *conf)
>
> scoped_fd fd (syscall (SYS_perf_event_open, &bts->attr, pid, -1, -1, 0));
>
> if (fd.get () < 0)
>
> diagnose_perf_event_open_fail ();
>
> + long page_size = sysconf (_SC_PAGESIZE);
>
>
>
> Please split those PAGE_SIZE changes into a separate patch. This is unrelated
>
> to what this patch is doing.
>
>
>
> Note that PAGE_SIZE was unsigned whereas sysconf () returns a signed integer.
>
> I'd expect compilers to require proper casting.
>
> [Zied] done
>
>
>
>
>
>
>
>
> +/* Enable ARM CoreSight ETM tracing. */
>
> +
>
> +static struct btrace_target_info *
>
> +linux_enable_etm (ptid_t ptid, const struct btrace_config_etm *conf)
>
> +{
>
> [...]
>
> + etm->attr.sample_type = PERF_SAMPLE_CPU;
>
> + etm->attr.read_format = PERF_FORMAT_ID;
>
> + etm->attr.sample_id_all = 1;
>
>
>
> You enable sampling. Wouldn't you need to mmap the data buffer, as well?
>
> [Zied] it is not needed for current implementation. removed.
>
>
>
>
>
>
>
>
> This ...
>
>
>
> + errno = 0;
>
> + scoped_fd fd (syscall (SYS_perf_event_open, &etm->attr, pid, -1, -1, 0));
>
> + if (fd.get () < 0)
>
> + diagnose_perf_event_open_fail ();
>
> +
>
> + /* Allocate the configuration page. */
>
> + long page_size = sysconf (_SC_PAGESIZE);
>
> + scoped_mmap data (nullptr, page_size, PROT_READ | PROT_WRITE,
>
> MAP_SHARED,
>
> + fd.get (), 0);
>
> + if (data.get () == MAP_FAILED)
>
> + error (_("Failed to map trace user page: %s."), safe_strerror (errno));
>
> +
>
> + struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
>
> + data.get ();
>
> +
>
> + header->aux_offset = header->data_offset + header->data_size;
>
> + /* Convert the requested size in bytes to pages (rounding up). */
>
> + pages = ((size_t) conf->size / page_size
>
> + + ((conf->size % page_size) == 0 ? 0 : 1));
>
> + /* We need at least one page. */
>
> + if (pages == 0)
>
> + pages = 1;
>
> +
>
> + /* The buffer size can be requested in powers of two pages. Adjust PAGES
>
> + to the next power of two. */
>
> + for (pg = 0; pages != ((size_t) 1 << pg); ++pg)
>
> + if ((pages & ((size_t) 1 << pg)) != 0)
>
> + pages += ((size_t) 1 << pg);
>
> +
>
> + /* We try to allocate the requested size.
>
> + If that fails, try to get as much as we can. */
>
> + scoped_mmap aux;
>
> + for (; pages > 0; pages >>= 1)
>
> + {
>
> + size_t length;
>
> + __u64 data_size;
>
> + data_size = (__u64) pages * page_size;
>
> +
>
> + /* Don't ask for more than we can represent in the configuration. */
>
> + if ((__u64) UINT_MAX < data_size)
>
> + continue;
>
> +
>
> + length = (size_t) data_size;
>
> +
>
> + /* Check for overflows. */
>
> + if ((__u64) length != data_size)
>
> + continue;
>
> +
>
> + header->aux_size = data_size;
>
> +
>
> + errno = 0;
>
> + aux.reset (nullptr, length, PROT_READ, MAP_SHARED, fd.get (),
>
> + header->aux_offset);
>
> + if (aux.get () != MAP_FAILED)
>
> + break;
>
> + }
>
> + if (pages == 0)
>
> + error (_("Failed to map trace buffer: %s."), safe_strerror (errno));
>
> +
>
> + etm->etm.size = aux.size ();
>
> + etm->etm.mem = (const uint8_t *) aux.release ();
>
> + etm->etm.data_head = &header->aux_head;
>
> + etm->etm.last_head = header->aux_tail;
>
> + etm->header = (struct perf_event_mmap_page *) data.release ();
>
> + gdb_assert (etm->header == header);
>
>
>
> ... can be shared with btrace_enable_pt () by introducing some
>
>
>
> perf_event_open_aux (struct perf_event_buffer *, const struct perf_event_attr *)
>
>
>
> helper.
>
>
>
> And if you indeed need to mmap the data buffer, as well, we can share that with
>
> btrace_enable_bts (), although we'd need some more restructuring to leave
>
> perf_event_open to the caller and just allocate the data and aux buffers using
>
> two helpers - they would again look very similar but need to touch a different
>
> set of fields in the header, so I'd keep those separate.
>
>
>
> btrace_enable_foo () would then become
>
> {
>
> perf_event_open ()
>
> perf_event_mmap_data ()
>
> perf_event_mmap_aux () /* not for bts */
>
> }
>
>
>
> [Zied] I like the idea, there are two aspects that we need to
> consider to bring it to the mainstream code:
>
> - 1: interface and scope definition
>
> static scoped_fd perf_event_open ( const struct perf_event_attr
> *event_attributes, const int pid )
>
> {
>
> errno = 0;
> scoped_fd fd (syscall (SYS_perf_event_open, event_attributes,
> pid, -1, -1, 0));
> if (fd.get () < 0)
> diagnose_perf_event_open_fail ();
>
> return fd;
>
> }
>
> this function will only open the file descriptor and return it
>
> static scoped_mmap data perf_event_mmap_data (const scoped_fd fd,
> size_t size, size_t page_size, int offset)
>
> {
>
> //alternative 1 just create it and return it
>
> /* Allocate the configuration page. */
> scoped_mmap data (nullptr, page_size, PROT_READ | PROT_WRITE,
> MAP_SHARED,
> fd.get (), 0);
> if (data.get () == MAP_FAILED)
> error (_("Failed to map trace user page: %s."), safe_strerror
> (errno));
>
> return data;
>
> //alternative 2, create it it and make sure that we resize it to
> the highest possible power of 2 supported by the system
>
> /* Convert the requested size in bytes to pages (rounding up). */
> pages = size / page_size
> + (size % page_size) == 0 ? 0 : 1));
> /* We need at least one page. */
> if (pages == 0)
> pages = 1;
>
> /* The buffer size can be requested in powers of two pages.
> Adjust PAGES
> to the next power of two. */
> for (pg = 0; pages != ((size_t) 1 << pg); ++pg)
> if ((pages & ((size_t) 1 << pg)) != 0)
> pages += ((size_t) 1 << pg);
>
> /* We try to allocate the requested size.
> If that fails, try to get as much as we can. */
> scoped_mmap data;
> for (; pages > 0; pages >>= 1)
> {
> size_t length;
> __u64 data_size;
>
> data_size = (__u64) pages * page_size;
>
> /* Don't ask for more than we can represent in the
> configuration. */
> if ((__u64) UINT_MAX < data_size)
> continue;
>
> size = (size_t) data_size;
> length = size + page_size;
>
> /* Check for overflows. */
> if ((__u64) length != data_size + page_size)
> continue;
>
> errno = 0;
> /* The number of pages we request needs to be a power of
> two. */
> data.reset (nullptr, length, PROT_READ, MAP_SHARED, fd.get
> (), offset);
> if (data.get () != MAP_FAILED)
> break;
> }
>
> if (pages == 0)
> error (_("Failed to map trace buffer: %s."), safe_strerror
> (errno));
>
> }
>
> static scoped_mmap perf_event_mmap_aux (const scoped_fd fd, size_t
> size, size_t page_size, int offset)
>
> {
>
> //idem, the function is similar to previous one it is only the
> offset in data.reset call that changes
>
> }
>
> so both perf_event_mmap_data perf_event_mmap_aux can be in fact
> reduced to one function if we give size_t size, size_t page_size,
> int offset as parameters.
>
> 2- how to bring this change to the mainstream. here we have two
> alternatives : either I do this restructuring for etm only and
> then you take care of using the helper functions for bts and pt or
> I commit it as it is and then I issue a patch for this point only
> for bts PT and ETM. What do you prefer?
>
>
>
> + length = fread (buffer, 1, length, file.get ());
>
> + buffer[length]='\0';
>
>
>
> Spaces around =.
>
> [Zied] done.
>
>
>
>
>
>
> + while ((--length) != 0)
>
> + {
>
> + if ((buffer[length] == ',') || (buffer[length] == '-'))
>
> + {
>
> + length++;
>
> + break;
>
> + }
>
> + }
>
> +
>
> + int cpu_count;
>
> + int found = sscanf (&buffer[length], "%d", &cpu_count);
>
> + if (found < 1)
>
> + error (_("Failed to get cpu count in %s: %s."),
>
> + buffer, safe_strerror (errno));
>
> +
>
> + cpu_count ++;
>
> + return (cpu_count);
>
>
>
> No need for ().
>
> [Zied] done.
>
>
>
>
>
>
>
>
> + char filename[PATH_MAX];
>
> + snprintf (filename, PATH_MAX,
>
>
>
> sizeof (filename)
>
> [Zied] done.
>
>
>
>
>
>
> + char filename[PATH_MAX];
>
> +
>
> + /* Get coresight register from sysfs. */
>
> + snprintf (filename, PATH_MAX,
>
>
>
> sizeof (filename)
>
> [Zied] done
>
>
>
>
>
>
> + "/sys/bus/event_source/devices/cs_etm/cpu%d/%s", cpu, path);
>
> + errno = 0;
>
> + gdb_file_up file = gdb_fopen_cloexec (filename, "r");
>
> + if (file.get () == nullptr)
>
> + error (_("Failed to open %s: %s."), filename, safe_strerror (errno));
>
> +
>
> + uint32_t val = 0;
>
> +
>
> + int found = fscanf (file.get (), "0x%x", &val);
>
> + if (found != 1)
>
> + error (_("Failed to read coresight register from %s."), filename);
>
> + return val;
>
> +}
>
>
>
> Empty line before return? I'd also remove the empty line between the
>
> declaration of val and the call to fscanf ().
>
>
>
> There are several very similar functions in this patch and each is structured
>
> differently:
>
>
>
> +perf_event_etm_event_type ()
>
> +{
>
> [...]
>
> +
>
> + int type, found = fscanf (file.get (), "%d", &type);
>
> + if (found != 1)
>
> + error (_("Failed to read the ETM event type from %s."), filename);
>
> +
>
> + return type;
>
>
>
> +get_cpu_count (void)
>
> +{
>
> [...]
>
> +
>
> + int cpu_count;
>
> + int found = sscanf (&buffer[length], "%d", &cpu_count);
>
> + if (found < 1)
>
> + error (_("Failed to get cpu count in %s: %s."),
>
> + buffer, safe_strerror (errno));
>
> +
>
> + cpu_count ++;
>
> + return (cpu_count);
>
>
>
> +perf_event_etm_event_sink (const struct btrace_config_etm *conf)
>
> +{
>
> [...]
>
> +
>
> + unsigned int sink;
>
> + int found = fscanf (file.get (), "0x%x", &sink);
>
> + if (found != 1)
>
> + error (_("Failed to read the ETM sink from %s."), filename);
>
> +
>
> + return sink;
>
>
>
> +cs_etm_get_register (int cpu, const char *path)
>
> +{
>
> [...]
>
> +
>
> + uint32_t val = 0;
>
> +
>
> + int found = fscanf (file.get (), "0x%x", &val);
>
> + if (found != 1)
>
> + error (_("Failed to read coresight register from %s."), filename);
>
> + return val;
>
>
>
> [Zied] existing code in this file has an empty line before the
> last return everywhere. shall I stick to this convention?
> eliminating the empty lines before returns will bring
> inconsistency in the file. which coding convention do we have to
> apply here?
>
>
>
>
> +
>
> +#define CORESIGHT_ETM_PMU_SEED 0x10
>
> +
>
> +/* Calculate trace_id for this cpu
>
> + to be kept aligned with coresight-pmu.h. */
>
> +
>
> +static inline int
>
> +coresight_get_trace_id (int cpu)
>
> +{
>
> + return (CORESIGHT_ETM_PMU_SEED + (cpu * 2));
>
>
>
> In patch 3, you wrote
>
>
>
> + /* Trace id for this thread.
>
> + On a linux system, trace_id is assigned per cpu. The kernel copies
>
> + the traces of each thread in a dedicated ring buffer. By this,
>
> + traces belonging to different threads are de-multiplexed.
>
> + On an RTOS system, especially when routing the traces outside of the SoC,
>
> + the OS has no other mean for de-multiplexing the traces than
>
> + the trace_id. The hardware (ETM IP) reserves 7 bits for the trace_id.
>
> + On linux system trace id is not needed, set it to 0xFF to ignore it
>
> + during parsing. */
>
> + uint8_t trace_id;
>
>
>
> Should this function return uint8_t and check that the ID is 7 bit max?
>
> [Zied] done, reserved and not allowed values are also generating a
> warning now.
>
>
>
>
>
>
>
>
> +static void
>
> +fill_etm_trace_params (struct cs_etm_trace_params *etm_trace_params, int
>
> cpu)
>
> +{
>
> + if (cs_etm_is_etmv4 (cpu) == true)
>
>
>
> No need for explicit checks on bool.
>
> [Zied] done.
>
>
>
>
>
>
>
>
> +static void
>
> +linux_fill_btrace_etm_config (struct btrace_target_info *tinfo,
>
> + struct btrace_data_etm_config *conf)
>
> +{
>
> +
>
> + cs_etm_trace_params etm_trace_params;
>
>
>
> Please declare at initialization time.
>
> [Zied] pushed forwards before the for loop
>
>
>
>
>
>
> + conf->cpu_count = get_cpu_count ();
>
> + conf->etm_trace_params = new std::vector<cs_etm_trace_params>;
>
> + for (int i = 0; i < conf->cpu_count; i++)
>
> + {
>
> + fill_etm_trace_params (&etm_trace_params,i);
>
> + conf->etm_trace_params->push_back (etm_trace_params);
>
> + }
>
>
>
> We need to avoid leaking the vector when fill_etm_trace_params () throws.
>
>
>
>
>
>
>
> +static enum btrace_error
>
> +linux_read_etm (struct btrace_data_etm *btrace,
>
> + struct btrace_target_info *tinfo,
>
> + enum btrace_read_type type)
>
> +{
>
> + struct perf_event_buffer *etm;
>
> + etm = &tinfo->variant.etm.etm;
>
>
>
> Please combine. No forward declarations anymore. The old code was written
>
> when GDB was still C.
>
> [Zied] done.
>
>
>
>
>
>
>
>
> regards,
>
> markus.
>
> Intel Deutschland GmbH
>
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
>
> Tel: +49 89 99 8853-0,www.intel.de <http://www.intel.de> <http://www.intel.de> <http://www.intel.de>
>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
>
> Chairperson of the Supervisory Board: Nicole Lau
>
> Registered Office: Munich
>
> Commercial Register: Amtsgericht Muenchen HRB 186928
>
>
>
> --
>
>
> *Zied Guermazi*
> founder
>
> Trande GmbH
> Leuschnerstraße 2
> 69469 Weinheim/Germany
>
> Mobile: +491722645127
> mailto:zied.guermazi@trande.de <mailto:zied.guermazi@trande.de>
>
> *Trande GmbH*
> Leuschnerstraße 2, D-69469 Weinheim; Telefon: +491722645127
> Sitz der Gesellschaft: Weinheim- Registergericht: AG Mannheim HRB
> 736209 - Geschäftsführung: Zied Guermazi
>
> *Confidentiality Note*
> This message is intended only for the use of the named
> recipient(s) and may contain confidential and/or privileged
> information. If you are not the intended recipient, please contact
> the sender and delete the message. Any unauthorized use of the
> information contained in this message is prohibited.
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany
> Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>
--
*Zied Guermazi*
founder
Trande GmbH
Leuschnerstraße 2
69469 Weinheim/Germany
Mobile: +491722645127
mailto:zied.guermazi@trande.de <mailto:zied.guermazi@trande.de>
*Trande GmbH*
Leuschnerstraße 2, D-69469 Weinheim; Telefon: +491722645127
Sitz der Gesellschaft: Weinheim- Registergericht: AG Mannheim HRB 736209
- Geschäftsführung: Zied Guermazi
*Confidentiality Note*
This message is intended only for the use of the named recipient(s) and
may contain confidential and/or privileged information. If you are not
the intended recipient, please contact the sender and delete the
message. Any unauthorized use of the information contained in this
message is prohibited.
More information about the Gdb-patches
mailing list