Re: [PATCH v3 1/4] ethdev: add trace points

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 10/6/2022 4:18 PM, Ankur Dwivedi wrote:

Hi Ankur, Jerin,

I have some major questions, I can see some already queried by Andrew as
well:

1) How flexible are we on changing trace log output?
Should we take trace log as kind of user interface and don't break it
(as much as possible)? Or is it OK to change whenever we want?


2) What is the main purpose of the trace library.
Is it to record type and frequency of the calls? Or is it to log values
and state of the device/driver after each call?
This can guide us to decide
- which values to record (should record only pointers or values of structs)
- if to record API return values and failure paths
- if to add tracing all APIs or not, like
'rte_eth_dev_rx_offload_name()' kind of helper function that converts
offload to a string, which doesn't have any functional impact, or
'rte_eth_speed_bitflag()', and many more...


3) What is the runtime impact.
As far as I can see some values copied and some memory is used for this
support, even user is not interested in tracing. For control path APIs
we can ignore the additional cost by memcpy, but how big memory
requirement is?


4) Why we need to export trace point variables in the .map files,
like '__rte_eth_trace_allmulticast_disable' one...



5) (bonus Q) Can we have an 'rte_trace_point_emit_xx()' function to
record mac address (struct rte_ether_addr) as string?
And maybe another set for array types?


There are bunch of small comments inline, some are related to above
highlevel question.
But I stopped after some point, specifically after
'rte_eth_trace_timesync_adjust_time()' :), this is a big file, splitting
it may help reviewers.

> 
> Add trace points for ethdev functions.
> 
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>

<...>

> +RTE_TRACE_POINT_REGISTER(rte_eth_trace_rx_burst_mode_get,
> +       lib.ethdev.rx_burst_mode_get)
> +

Tx counterpart of this API is missing.

> +RTE_TRACE_POINT_REGISTER(rte_eth_trace_rx_queue_info_get,
> +       lib.ethdev.rx_queue_info_get)
> +
> +RTE_TRACE_POINT_REGISTER(rte_eth_trace_rx_queue_setup,
> +       lib.ethdev.rx_queue_setup)
> +

! rte_eth_trace_rx_queue_setup() is not called.

And Tx counterpart of this API is missing, these are important APIs, we
should have them.

<...>

> @@ -331,6 +336,7 @@ rte_eth_find_next(uint16_t port_id)
>         if (port_id >= RTE_MAX_ETHPORTS)
>                 return RTE_MAX_ETHPORTS;
> 
> +       rte_eth_trace_find_next(port_id);


For the output 'port_id', trace function is after "return
RTE_MAX_ETHPORTS;", so "there is no next port" case is ignored, need to
update function to get trace on each call.

<...>

> @@ -636,6 +656,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
>         RTE_ETH_FOREACH_VALID_DEV(pid)
>                 if (!strcmp(name, eth_dev_shared_data->data[pid].name)) {
>                         *port_id = pid;
> +                       rte_ethdev_trace_get_port_by_name(name, *port_id);


This completely ignores the path that not able to find port_id from name.

What about change the function as

```
ret = -ENODEV;
*port_id = RTE_MAX_ETHPORTS;

RTE_ETH_FOREACH_VALID_DEV(pid)
  if (!strcmp())
    *port_id = pid;
    ret = 0;
    break;

rte_ethdev_trace_get_port_by_name(name, *port_id, ret);

return ret;
```

>                         return 0;
>                 }
> 
> @@ -737,6 +758,7 @@ rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
>                 return 0;
>         }
> 
> +       rte_ethdev_trace_rx_queue_start(port_id, rx_queue_id);

Please include return value.

>         return eth_err(port_id, dev->dev_ops->rx_queue_start(dev, rx_queue_id));
>  }
> 
> @@ -770,6 +792,7 @@ rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id)
>                 return 0;
>         }
> 
> +       rte_ethdev_trace_rx_queue_stop(port_id, rx_queue_id);

Please include return value.

>         return eth_err(port_id, dev->dev_ops->rx_queue_stop(dev, rx_queue_id));
>  }
> 
> @@ -810,6 +833,7 @@ rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
>                 return 0;
>         }
> 
> +       rte_ethdev_trace_tx_queue_start(port_id, tx_queue_id);

Please include return value.

>         return eth_err(port_id, dev->dev_ops->tx_queue_start(dev, tx_queue_id));
>  }
> 
> @@ -843,12 +867,14 @@ rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id)
>                 return 0;
>         }
> 
> +       rte_ethdev_trace_tx_queue_stop(port_id, tx_queue_id);

Please include return value.

>         return eth_err(port_id, dev->dev_ops->tx_queue_stop(dev, tx_queue_id));
>  }
> 
>  uint32_t
>  rte_eth_speed_bitflag(uint32_t speed, int duplex)
>  {
> +       rte_eth_trace_speed_bitflag(speed, duplex);

I think this should have return value, but I can see this requires
change in the function.

I am for either having return value or drop the tracing of this function
completely.

<...>

> @@ -1552,6 +1581,7 @@ rte_eth_dev_set_link_up(uint16_t port_id)
> 
>         if (*dev->dev_ops->dev_set_link_up == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_set_link_up(port_id);

Please include return value.

>         return eth_err(port_id, (*dev->dev_ops->dev_set_link_up)(dev));
>  }
> 
> @@ -1565,6 +1595,7 @@ rte_eth_dev_set_link_down(uint16_t port_id)
> 
>         if (*dev->dev_ops->dev_set_link_down == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_set_link_down(port_id);

Please include return value.

<...>

> @@ -2327,6 +2371,7 @@ rte_eth_promiscuous_enable(uint16_t port_id)
>         diag = (*dev->dev_ops->promiscuous_enable)(dev);
>         dev->data->promiscuous = (diag == 0) ? 1 : 0;
> 
> +       rte_eth_trace_promiscuous_enable(port_id, dev->data->promiscuous);

Above is good, but to be consistent what about returning return value of
the API (diag (with record name 'return')), which is relavent with
'dev->data->promiscuous' value.

>         return eth_err(port_id, diag);
>  }
> 
> @@ -2350,6 +2395,7 @@ rte_eth_promiscuous_disable(uint16_t port_id)
>         if (diag != 0)
>                 dev->data->promiscuous = 1;
> 
> +       rte_eth_trace_promiscuous_disable(port_id, dev->data->promiscuous);

Above is good, but to be consistent what about returning return value of
the API (diag (with record name 'return')), which is relavent with
'dev->data->promiscuous' value.

<...>

> @@ -2561,6 +2616,7 @@ rte_eth_stats_reset(uint16_t port_id)
> 
>         dev->data->rx_mbuf_alloc_failed = 0;
> 
> +       rte_eth_trace_stats_reset(port_id);

Please include return value.

<...>

> @@ -3483,6 +3561,7 @@ rte_eth_dev_set_vlan_strip_on_queue(uint16_t port_id, uint16_t rx_queue_id,
> 
>         if (*dev->dev_ops->vlan_strip_queue_set == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_set_vlan_strip_on_queue(port_id, rx_queue_id, on);

Please include return value.

>         (*dev->dev_ops->vlan_strip_queue_set)(dev, rx_queue_id, on);
> 
>         return 0;
> @@ -3500,6 +3579,7 @@ rte_eth_dev_set_vlan_ether_type(uint16_t port_id,
> 
>         if (*dev->dev_ops->vlan_tpid_set == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_set_vlan_ether_type(port_id, vlan_type, tpid);

Please include return value.

<...>

> @@ -3632,6 +3714,7 @@ rte_eth_dev_set_vlan_pvid(uint16_t port_id, uint16_t pvid, int on)
> 
>         if (*dev->dev_ops->vlan_pvid_set == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_set_vlan_pvid(port_id, pvid, on);

Please include return value.

<...>

> @@ -3702,6 +3787,7 @@ rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id,
>                 return -EINVAL;
>         }
> 
> +       rte_ethdev_trace_priority_flow_ctrl_set(port_id, pfc_conf);

Please put the call very bottom of the function and include return value.

<...>

> @@ -4063,6 +4156,7 @@ rte_eth_dev_udp_tunnel_port_add(uint16_t port_id,
> 
>         if (*dev->dev_ops->udp_tunnel_port_add == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_udp_tunnel_port_add(port_id, udp_tunnel);

Please include return value.

>         return eth_err(port_id, (*dev->dev_ops->udp_tunnel_port_add)(dev,
>                                                                 udp_tunnel));
>  }
> @@ -4090,6 +4184,7 @@ rte_eth_dev_udp_tunnel_port_delete(uint16_t port_id,
> 
>         if (*dev->dev_ops->udp_tunnel_port_del == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_udp_tunnel_port_delete(port_id, udp_tunnel);

Please include return value.

>         return eth_err(port_id, (*dev->dev_ops->udp_tunnel_port_del)(dev,
>                                                                 udp_tunnel));
>  }
> @@ -4104,6 +4199,7 @@ rte_eth_led_on(uint16_t port_id)
> 
>         if (*dev->dev_ops->dev_led_on == NULL)
>                 return -ENOTSUP;
> +       rte_eth_trace_led_on(port_id);

Please include return value.

>         return eth_err(port_id, (*dev->dev_ops->dev_led_on)(dev));
>  }
> 
> @@ -4117,6 +4213,7 @@ rte_eth_led_off(uint16_t port_id)
> 
>         if (*dev->dev_ops->dev_led_off == NULL)
>                 return -ENOTSUP;
> +       rte_eth_trace_led_off(port_id);

Please include return value.

<...>

> @@ -4294,6 +4395,7 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>         } else if (index < 0)
>                 return 0;  /* Do nothing if address wasn't found */
> 
> +       rte_ethdev_trace_mac_addr_remove(port_id, addr);

better to keep trace APIs are last thing in the function for
consistency, as long as it is possible, and for this case it is possible.

<...>

> @@ -4438,6 +4542,7 @@ rte_eth_dev_uc_all_hash_table_set(uint16_t port_id, uint8_t on)
> 
>         if (*dev->dev_ops->uc_all_hash_table_set == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_uc_all_hash_table_set(port_id, on);

Please include return value.

>         return eth_err(port_id, (*dev->dev_ops->uc_all_hash_table_set)(dev,
>                                                                        on));
>  }
> @@ -4475,6 +4580,7 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
> 
>         if (*dev->dev_ops->set_queue_rate_limit == NULL)
>                 return -ENOTSUP;
> +       rte_eth_trace_set_queue_rate_limit(port_id, queue_idx, tx_rate);

Please include return value.

<...>

> @@ -4570,6 +4678,9 @@ rte_eth_dev_callback_register(uint16_t port_id,
>                 next_port = last_port = port_id;
>         }
> 
> +       rte_ethdev_trace_callback_register(port_id, event, cb_fn, cb_arg, next_port,
> +                                          last_port);
> +

I assume this added into middle to be able to use 'next_port' &
'last_port', but they are generated from given 'port_id', so what do you
think to have only 'port_id', and move call to the bottom of the function.

'rte_ethdev_trace_callback_unregister()' has same logic, and it has only
'port_id' already.



And I think better to standardise to record API return value or not,
because some has it, some don't.
What do you think to trace all return values, is there any downside of
this approach?

If we need fine grain on recording return values, what about following:
- All API that changes device config return values should be recorded
  * like, record return for set_mtu(), but ignore it for get_mtu()
- If return value has not API status but useful information, record it
  * like rte_eth_dev_is_valid_port() return value


>         rte_spinlock_lock(&eth_dev_cb_lock);
> 
>         do {
> @@ -4665,6 +4776,7 @@ rte_eth_dev_callback_unregister(uint16_t port_id,
>         } while (++next_port <= last_port);
> 
>         rte_spinlock_unlock(&eth_dev_cb_lock);
> +       rte_ethdev_trace_callback_unregister(port_id, event, cb_fn, cb_arg, ret);

Just a syntax comment, what do you think to have an empty line one
before and after the trace call, to separate this trace calls from
regular logic.

>         return ret;
>  }
> 
> @@ -4694,6 +4806,7 @@ rte_eth_dev_rx_intr_ctl(uint16_t port_id, int epfd, int op, void *data)
>         for (qid = 0; qid < dev->data->nb_rx_queues; qid++) {
>                 vec = rte_intr_vec_list_index_get(intr_handle, qid);
>                 rc = rte_intr_rx_ctl(intr_handle, epfd, op, vec, data);
> +               rte_ethdev_trace_rx_intr_ctl(port_id, epfd, op, data, rc);

Why this is in the for loop, API if for port level, this loop iterates
on queues?
What about moving this trace to the bottom of the function, and record
return value as return value of the API?

>                 if (rc && rc != -EEXIST) {
>                         RTE_ETHDEV_LOG(ERR,
>                                 "p %u q %u Rx ctl error op %d epfd %d vec %u\n",
> @@ -4737,6 +4850,7 @@ rte_eth_dev_rx_intr_ctl_q_get_fd(uint16_t port_id, uint16_t queue_id)
>                 (vec - RTE_INTR_VEC_RXTX_OFFSET) : vec;
>         fd = rte_intr_efds_index_get(intr_handle, efd_idx);
> 
> +       rte_ethdev_trace_rx_intr_ctl_q_get_fd(port_id, queue_id, fd);
>         return fd;
>  }
> 
> @@ -4770,6 +4884,7 @@ rte_eth_dev_rx_intr_ctl_q(uint16_t port_id, uint16_t queue_id,
> 
>         vec = rte_intr_vec_list_index_get(intr_handle, queue_id);
>         rc = rte_intr_rx_ctl(intr_handle, epfd, op, vec, data);
> +       rte_ethdev_trace_rx_intr_ctl_q(port_id, queue_id, epfd, op, data, rc);

Instead of trace API having in the middle, what about changing API as:

```
rc = ...

if (rc && rc != -EEXIST) {
	...
}

rte_ethdev_trace_rx_intr_ctl_q(port_id, queue_id, epfd, op, data, rc);

return rc;
```

>         if (rc && rc != -EEXIST) {
>                 RTE_ETHDEV_LOG(ERR,
>                         "p %u q %u Rx ctl error op %d epfd %d vec %u\n",
> @@ -4796,6 +4911,7 @@ rte_eth_dev_rx_intr_enable(uint16_t port_id,
> 
>         if (*dev->dev_ops->rx_queue_intr_enable == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_rx_intr_enable(port_id, queue_id);

Please include return value.

>         return eth_err(port_id, (*dev->dev_ops->rx_queue_intr_enable)(dev, queue_id));
>  }
> 
> @@ -4815,6 +4931,7 @@ rte_eth_dev_rx_intr_disable(uint16_t port_id,
> 
>         if (*dev->dev_ops->rx_queue_intr_disable == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_rx_intr_disable(port_id, queue_id);

Please include return value.

<...>

> @@ -5447,6 +5586,7 @@ rte_eth_dev_set_eeprom(uint16_t port_id, struct rte_dev_eeprom_info *info)
> 
>         if (*dev->dev_ops->set_eeprom == NULL)
>                 return -ENOTSUP;
> +       rte_ethdev_trace_set_eeprom(port_id, info);

Please include return value.

<...>

> @@ -5562,6 +5705,7 @@ rte_eth_dev_adjust_nb_rx_tx_desc(uint16_t port_id,
>         if (nb_tx_desc != NULL)
>                 eth_dev_adjust_nb_desc(nb_tx_desc, &dev_info.tx_desc_lim);
> 
> +       rte_ethdev_trace_adjust_nb_rx_tx_desc(port_id, *nb_rx_desc, *nb_rx_desc);


'nb_rx_desc' or 'nb_rx_desc' can be NULL, so we can't directly access them.

<...>

> @@ -5605,6 +5750,7 @@ rte_eth_dev_pool_ops_supported(uint16_t port_id, const char *pool)
>         if (*dev->dev_ops->pool_ops_supported == NULL)
>                 return 1; /* all pools are supported */
> 
> +       rte_ethdev_trace_pool_ops_supported(port_id, pool);

Please include return value.

<...>

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_flow_ctrl_get,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_fc_conf *fc_conf),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(fc_conf);
> +)

'fc_conf' is pointer for the config struct, its address is not much
useful, can you expand it similar to ones in
'rte_ethdev_trace_flow_ctrl_set()' API?

> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_flow_ctrl_set,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_fc_conf *fc_conf),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u32(fc_conf->high_water);
> +       rte_trace_point_emit_u32(fc_conf->low_water);
> +       rte_trace_point_emit_u16(fc_conf->pause_time);
> +       rte_trace_point_emit_u16(fc_conf->send_xon);
> +       rte_trace_point_emit_int(fc_conf->mode);
> +       rte_trace_point_emit_u8(fc_conf->mac_ctrl_frame_fwd);
> +       rte_trace_point_emit_u8(fc_conf->autoneg);
> +)
> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_fw_version_get,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, char *fw_version, size_t fw_size),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(fw_version);
> +       rte_trace_point_emit_size_t(fw_size);
> +)

'fw_version' is string, it help to display the string, but not sure if
the pointer value has a value.
Is there a way to display string in the trace point?
And 'fw_size' is normally just used to know the valid data in the
'fw_version', when 'fw_version' string is displayed, not sure if there
is a value to have 'fw_size'

> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_get_dcb_info,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_dcb_info *dcb_info),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(dcb_info);
> +)

'dcb_info' is pointer to a info struct, not useful as pointer value, we
should display content of the 'dcb_info' struct.
There are arrays in the 'dcb_info', perhaps it can be possible to trace
'dcb_info->nb_tcs', also 'dcb_info->prio_tc[]' & 'dcb_info->tc_bws[]'
since their size is small (8).

> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_get_eeprom,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_dev_eeprom_info *info),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(info);
> +)
> +

Similarly, content of the 'info' should be displayed, for '*data' it can
be possible to display address of it, but values for rest.

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_get_eeprom_length,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id),
> +       rte_trace_point_emit_u16(port_id);
> +)
> +

This should return the size of the eeprom, existing
'rte_eth_dev_get_eeprom_length()' needs to be updated for it.

<...>

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_get_reg_info,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_dev_reg_info *info),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(info);
> +)

What do you think to display content of the 'info', insted of pointer
value which is not very useful.

> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_get_sec_ctx,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id),
> +       rte_trace_point_emit_u16(port_id);
> +)

'security_ctx' is "void *", so can't record content, but what do you
think to record pointer value of it?

> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_get_supported_ptypes,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, uint32_t ptype_mask,
> +               uint32_t *ptypes, int num, int j),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u32(ptype_mask);
> +       rte_trace_point_emit_ptr(ptypes);
> +       rte_trace_point_emit_int(num);
> +       rte_trace_point_emit_int(j);
> +)

better to use 'supported_num' instead of 'j' which is not clear what it is.

The real information is in 'ptypes' but that is an array, I guess not
possible to record array values.

> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_get_vlan_offload,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, int ret),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_int(ret);
> +)
> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_info_get,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +               struct rte_eth_dev_info *dev_info),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_string(dev_info->driver_name);
> +       rte_trace_point_emit_u32(dev_info->if_index);
> +       rte_trace_point_emit_u16(dev_info->min_mtu);
> +       rte_trace_point_emit_u16(dev_info->max_mtu);
> +       rte_trace_point_emit_u32(dev_info->min_rx_bufsize);
> +       rte_trace_point_emit_u32(dev_info->max_rx_pktlen);
> +       rte_trace_point_emit_u64(dev_info->rx_offload_capa);
> +       rte_trace_point_emit_u64(dev_info->tx_offload_capa);
> +       rte_trace_point_emit_u64(dev_info->rx_queue_offload_capa);
> +       rte_trace_point_emit_u64(dev_info->tx_queue_offload_capa);
> +       rte_trace_point_emit_u16(dev_info->reta_size);
> +       rte_trace_point_emit_u8(dev_info->hash_key_size);
> +       rte_trace_point_emit_u16(dev_info->nb_rx_queues);
> +       rte_trace_point_emit_u16(dev_info->nb_tx_queues);
> +)
> +

It is a debate what to display, but I would remove 'if_index' and add
'max_rx_queues', 'max_tx_queues', 'max_mac_addrs',
'flow_type_rss_offloads', 'rx_desc_lim', 'tx_desc_lim', 'speed_capa',
'dev_capa'.

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_is_removed,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, int ret),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_int(ret);
> +)
> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_is_valid_port,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id),
> +       rte_trace_point_emit_u16(port_id);
> +)
> +

Should have return value, if the port is valid of not.

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_mac_addr_add,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_ether_addr *addr,
> +               uint32_t pool),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(addr);
> +       rte_trace_point_emit_u32(pool);
> +)
> +

Isn't it possible to return 'addr' as string? I am not sure about
benefit to record addr pointer value.
Should have return value.

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_mac_addr_remove,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_ether_addr *addr),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(addr);
> +)
> +

As above, can content of 'addr' be recorded, like as string?

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_pool_ops_supported,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, const char *pool),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(pool);
> +)
> +

'pool' should be string, instead of pointer.

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_priority_flow_ctrl_set,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_pfc_conf *pfc_conf),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u32(pfc_conf->fc.high_water);
> +       rte_trace_point_emit_u32(pfc_conf->fc.low_water);
> +       rte_trace_point_emit_u16(pfc_conf->fc.pause_time);
> +       rte_trace_point_emit_u16(pfc_conf->fc.send_xon);
> +       rte_trace_point_emit_int(pfc_conf->fc.mode);
> +       rte_trace_point_emit_u8(pfc_conf->fc.mac_ctrl_frame_fwd);
> +       rte_trace_point_emit_u8(pfc_conf->fc.autoneg);
> +       rte_trace_point_emit_u8(pfc_conf->priority);
> +)
> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_reset,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, int ret),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_int(ret);
> +)
> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_rss_hash_conf_get,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_rss_conf *rss_conf),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(rss_conf);
> +)
> +

Please record content of the 'rss_conf' instead of pointer value.

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_rss_hash_update,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_rss_conf *rss_conf),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(rss_conf->rss_key);
> +       rte_trace_point_emit_u8(rss_conf->rss_key_len);
> +       rte_trace_point_emit_u64(rss_conf->rss_hf);
> +)
> +

Please add return value.

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_rss_reta_query,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +               struct rte_eth_rss_reta_entry64 *reta_conf, uint16_t reta_size),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(reta_conf);
> +       rte_trace_point_emit_u16(reta_size);
> +)
> +
> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_rss_reta_update,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +               struct rte_eth_rss_reta_entry64 *reta_conf, uint16_t reta_size),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u64(reta_conf->mask);
> +       rte_trace_point_emit_u16(reta_size);
> +)

'reta_conf->mask' on its own is not much usefull, and 'reta_conf->reta'
is an array and hard to record.
As above 'rte_ethdev_trace_rss_reta_query' trace point only record
reta_config pointer, this can do the same.

Overall, I think better to make get/set pair of an API record same set
of values.

<...>

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_set_mc_addr_list,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +               struct rte_ether_addr *mc_addr_set,
> +               uint32_t nb_mc_addr),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(mc_addr_set);
> +       rte_trace_point_emit_u32(nb_mc_addr);
> +)
> +

Better to record content of 'mc_addr_set' but it is pointer array for
mac address, so I guess we can't record these mac addresses.

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_set_ptypes,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, uint32_t ptype_mask,
> +               uint32_t *set_ptypes, unsigned int num),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u32(ptype_mask);
> +       rte_trace_point_emit_ptr(set_ptypes);
> +       rte_trace_point_emit_u32(num);
> +)
> +

Similarly 'set_ptypes' is an array.

I wonder if there should be an rte_trace_point_emit_ function for
arrays? What do you think?

<...>

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_socket_id,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id),
> +       rte_trace_point_emit_u16(port_id);
> +)

Should include 'socket_id'.

<...>

> +RTE_TRACE_POINT(
> +       rte_ethdev_trace_uc_hash_table_set,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_ether_addr *addr,
> +               uint8_t on, int ret),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(addr);
> +       rte_trace_point_emit_u8(on);
> +       rte_trace_point_emit_int(ret);
> +)
> +

Can have 'addr' as string?
Do we need a specific rte_trace_point_emit_ function for mac addr?

<...>

> +RTE_TRACE_POINT(
> +       rte_eth_trace_find_next,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id),
> +       rte_trace_point_emit_u16(port_id);
> +)
> +

rte_eth_trace_find_next* functions gets 'port_id' as input parameter,
and returns next 'port_id', so both input and output is 'port_id' and
both should be recorded.

For this need to update these functions to create an interim variable
for input 'port_id'.

Above comments are for all 'rte_eth_trace_find_next*()' functions.

<...>

> +RTE_TRACE_POINT(
> +       rte_eth_trace_link_get,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_link *link),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u32(link->link_speed);
> +)

I think better to record other link values too.

> +
> +RTE_TRACE_POINT(
> +       rte_eth_trace_link_get_nowait,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_link *link),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u32(link->link_speed);
> +)
> +

I think better to record other link values too.

<...>

> +RTE_TRACE_POINT(
> +       rte_eth_trace_rx_burst_mode_get,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
> +               struct rte_eth_burst_mode *mode),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u16(queue_id);
> +       rte_trace_point_emit_ptr(mode);
> +)
> +

Can you please record content of 'mode' inst

> +RTE_TRACE_POINT(
> +       rte_eth_trace_rx_queue_info_get,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
> +               struct rte_eth_rxq_info *qinfo),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u16(queue_id);
> +       rte_trace_point_emit_ptr(qinfo->mp);
> +       rte_trace_point_emit_u8(qinfo->scattered_rx);
> +       rte_trace_point_emit_u8(qinfo->queue_state);
> +       rte_trace_point_emit_u16(qinfo->nb_desc);
> +       rte_trace_point_emit_u16(qinfo->rx_buf_size);
> +)
> +

Can you please add 'qinfo->conf->rx_drop_en' & 'qinfo->conf->offloads'.


> +RTE_TRACE_POINT(
> +       rte_eth_trace_rx_queue_setup,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t rx_queue_id,
> +               uint16_t nb_rx_desc, unsigned int socket_id,
> +               const struct rte_eth_rxconf *rx_conf,
> +               struct rte_mempool *mb_pool),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u16(rx_queue_id);
> +       rte_trace_point_emit_u16(nb_rx_desc);
> +       rte_trace_point_emit_u32(socket_id);
> +       rte_trace_point_emit_ptr(rx_conf);
> +       rte_trace_point_emit_ptr(mb_pool);
> +)
> +

Can you please add 'rx_conf->rx_drop_en' & 'rx_conf->offloads'.

> +RTE_TRACE_POINT(
> +       rte_eth_trace_set_queue_rate_limit,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_idx,
> +               uint16_t tx_rate),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_u16(queue_idx);
> +       rte_trace_point_emit_u16(tx_rate);
> +)
> +
> +RTE_TRACE_POINT(
> +       rte_eth_trace_speed_bitflag,
> +       RTE_TRACE_POINT_ARGS(uint32_t speed, int duplex),
> +       rte_trace_point_emit_u32(speed);
> +       rte_trace_point_emit_int(duplex);
> +)
> +
> +RTE_TRACE_POINT(
> +       rte_eth_trace_stats_get,
> +       RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_stats *stats),
> +       rte_trace_point_emit_u16(port_id);
> +       rte_trace_point_emit_ptr(stats);
> +       rte_trace_point_emit_u64(stats->rx_nombuf);
> +)
> +

Not sure what to record here, I think all basic stat counters can be
recorded if there cost is effordable.