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

[Ankur Dwivedi <adwivedi@marvell.com>]

Hi Ferruh,

Thanks a lot for reviewing this patch. The generic questions have been answered by Jerin in earlier email. I have replied inline to the remaining comments.
>
>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.
I will split this patch into two.
>
>>
>> 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.
The tx API is also added.
+RTE_TRACE_POINT_REGISTER(rte_eth_trace_tx_burst_mode_get,
+	lib.ethdev.tx_burst_mode_get)
+
>
>> +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.
Will remove this. This is not required as rte_ethdev_trace_rxq_setup is already added.

>
>And Tx counterpart of this API is missing, these are important APIs, we should
>have them.
rte_ethdev_trace_txq_setup is already added.
>
><...>
>
>> @@ -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.
Ok. Will add trace for the "there is no next port" case.
>
><...>
>
>> @@ -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.
The trace was added for the success case only. For the failure case there is no port_id value.
>
>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.
Will add return value. The following changes will be required:
ret = eth_err(port_id, dev->dev_ops->rx_queue_start(dev, rx_queue_id)); 
rte_ethdev_trace_rx_queue_start(port_id, rx_queue_id, ret);
return ret;
>
>>         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.
Ok.
>
>>         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.
Ok.
>
>>         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.
Ok.
>
>>         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.
Will drop the tracing for this function.
>
><...>
>
>> @@ -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.
Ok.
>
>>         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.
Ok.
>
><...>
>
>> @@ -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.
For this the following changes will capture return value and diag:
 ret = eth_err(port_id, diag);
rte_eth_trace_promiscuous_enable(port_id, dev->data->promiscuous, diag, ret);
return ret;
>
>>         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.
Similar changes as rte_eth_trace_promiscuous_enable.
>
><...>
>
>> @@ -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.
Here the return value is not required, because if the control reached here the return is 0.
>
><...>
>
>> @@ -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);
If the control reached this point the return is 0. I think the return value is not required here.
>>
>>         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.
Ok.
>
><...>
>
>> @@ -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.
Ok.
>
><...>
>
>> @@ -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.
Ok.
>
><...>
>
>> @@ -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.
Ok.
>
>>         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.
Ok.
>
>>         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.
Ok.
>
>>         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.
Ok.
>
><...>
>
>> @@ -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.
Ok.
>
><...>
>
>> @@ -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.
Ok.
>
>>         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.
Ok.
>
><...>
>
>> @@ -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.
I think its fine to move the trace to the bottom. But I was thinking of capturing all the input arguments to the function like port_id, event, cb_fn, cb_arg also.
>
>'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?
Initially my thought was to not change the existing code of library functions, and capture the trace with input args and in some cases capture return value where possible.
But now I think small modifications are fine if return values can be captured.
>
>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.
Just that it makes the function(and as a result the file) longer. But i agree adding the empty lines improve readability. 
>
>>         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?
Yes this iterates on queues records the return (rc) for qid 0 to nb_rx_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;
>```
In this trace the return value rc is being captured, so I was thinking changing the API is not required.
>
>>         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.
Ok.
>
>>         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.
Ok.
>
><...>
>
>> @@ -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.
Ok.
>
><...>
>
>> @@ -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.
Yes. Will remove nb_rx_desc and nb_tx_desc.
>
><...>
>
>> @@ -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.
Ok.
>
><...>
>
>> +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?
The value of fc_conf is updated after this line.
return eth_err(port_id, (*dev->dev_ops->flow_ctrl_get)(dev, fc_conf));

So to capture fc_conf values the return of eth_err needs to be stored in a local variable and also the return needs to be checked for success(if condition). Then only fc_conf will have valid values. Is this fine?

>
>> +
>> +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?
Yes string can be displayed in trace by rte_trace_point_emit_string() function. But there is a max string length of 32 which can be displayed. Will display 'fw_version' as string.
>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'
If fw_size is greater than 32, displaying fw_size will present some additional information, as only 32 bytes of fw_version will be displayed.
>
>> +
>> +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).
Yes will capture the array also. Same changes required as fc_conf above.
>
>> +
>> +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.
Ok. Same changes required as fc_conf above.
>
>> +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.
Ok. Same as fc_conf above.
>
><...>
>
>> +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.
Yes info can be displayed. The rte_eth_dev_get_reg_info needs to be changed such that trace is called after eth_err() function call. Same as fc_conf above.
>
>> +
>> +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_eth_dev_get_sec_ctx() needs to be changed. The rte_eth_devices[port_id].security_ctx needs to be stored in a local variable.
>
>> +
>> +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.
Ok.
>
>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'.
Ok.
>
>> +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.
Some changes needs to be made as the rte_ethdev_is_valid_port function return 0 or 1.
>
>> +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.
Right now the function to display array is not there in trace. I have added a new function(not yet sent upstream) to display mac addresses.
>
>> +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.
Ok.
>
>> +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.
Ok. Same as fc_conf above.
>
>> +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.
Ok.
>
>> +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.
In rte_eth_dev_rss_reta_query, if return of eth_err is captured and success then reta conf can be displayed.
>
>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?
In my new series(not yet sent upstream), I have added a function to display uint8_t arrays like mac addresses. Will have to check how much change is required to display other array type.
>
><...>
>
>> +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'.
Ok.
>
><...>
>
>> +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?
mac_addr can be displayed as arrays.
>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.
Ok.
>
><...>
>
>> +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.
Ok.
>
>> +
>> +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
mode->flags can be displayed. Not sure about info array.
>
>> +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'.
Ok.
>
>
>> +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'.
Ok.
>
>> +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.
Ok. Same changes required as fc_conf above.