Re: [PATCH v6 4/7] start/stop btrace with coresight etm and collect etm buffer on linux os

[Zied Guermazi <zied.guermazi@trande.de>]

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>
> 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.