app/procinfo: add device registers dump
Checks
Commit Message
From: Chengchang Tang <tangchengchang@huawei.com>
This patch add support for dump the device registers from a running
application. It can help developers locate the problem.
Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
app/proc-info/main.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 91 insertions(+), 1 deletion(-)
Comments
Ping for review.
It looks too late for 21.05, moved as Deferred in patchwork.
25/04/2021 15:02, Min Hu (Connor):
> From: Chengchang Tang <tangchengchang@huawei.com>
>
> This patch add support for dump the device registers from a running
> application. It can help developers locate the problem.
>
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
<snip>
> + ret = rte_eth_dev_get_reg_info(i, ®_info);
> + if (ret) {
> + printf("Error getting device reg info: %d\n", ret);
> + continue;
> + }
> +
> + buf_size = reg_info.length * reg_info.width;
If it is to get the regs length, you can directly call "rte_ethtool_get_regs_len(uint16_t port_id)" API , instead of again writing the above logic.
And use the returned length in below malloc.
> + fp_regs = fopen(file_name, "wb");
> + if (fp_regs == NULL) {
> + printf("Error during opening '%s' for writing\n",
> + file_name);
Better to print error string from fopen() errno on failure , to indicate the exact error.
> + } else {
> + if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=
Better have "((int)fwrite(buf_data, 1, buf_size, fp_regs)" In separate line and use the returned value inside if check.
> + buf_size)
> + printf("Error during writing %s\n",
> + file_prefix);
Better to print error string from fwrite errno on failure , to indicate the exact error.
> + else
> + printf("dump device (%s) regs successfully, "
Reframe the sente to "Device regs dumped successfully"
On 2021/6/4 23:04, Pattan, Reshma wrote:
>
>
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
>
> <snip>
>
>> + ret = rte_eth_dev_get_reg_info(i, ®_info);
>> + if (ret) {
>> + printf("Error getting device reg info: %d\n", ret);
>> + continue;
>> + }
>> +
>> + buf_size = reg_info.length * reg_info.width;
>
>
> If it is to get the regs length, you can directly call "rte_ethtool_get_regs_len(uint16_t port_id)" API , instead of again writing the above logic.
> And use the returned length in below malloc.
This logic is indeed identical to the logic of the "rte_ethtool_get_regs_len" API of Ethtool,
but the method of using the "rte_eth_dev_get_reg_info" API is the case. All users will have
similar code logic when using this API.
And use "rte_ethtool_get_regs_len" API here only reduces "buf_size = reg_info.length * reg_info.width;"
this logic. But at the same time, it introduces the dependence of example/ethtool in procinfo. The code in
example is not compiled by default, which seems not appropriate to import it in app/procinfo.
So, I think it is fine to keep this.
>
>
>> + fp_regs = fopen(file_name, "wb");
>> + if (fp_regs == NULL) {
>> + printf("Error during opening '%s' for writing\n",
>> + file_name);
>
> Better to print error string from fopen() errno on failure , to indicate the exact error.
Agree， I will fix it in the next version.
>
>> + } else {
>> + if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=
>
> Better have "((int)fwrite(buf_data, 1, buf_size, fp_regs)" In separate line and use the returned value inside if check.
Agree， I will fix it in the next version.
>
>> + buf_size)
>> + printf("Error during writing %s\n",
>> + file_prefix);
>
> Better to print error string from fwrite errno on failure , to indicate the exact error.
>
>> + else
>> + printf("dump device (%s) regs successfully, "
>
> Reframe the sente to "Device regs dumped successfully"
>
Agree， I will fix it in the next version.
> .
>
> -----Original Message-----
> From: Chengchang Tang <tangchengchang@huawei.com>
> >> + ret = rte_eth_dev_get_reg_info(i, ®_info);
> >> + if (ret) {
> >> + printf("Error getting device reg info: %d\n", ret);
> >> + continue;
> >> + }
> >> +
> >> + buf_size = reg_info.length * reg_info.width;
> >
> >
> > If it is to get the regs length, you can directly call
> "rte_ethtool_get_regs_len(uint16_t port_id)" API , instead of again writing the
> above logic.
> > And use the returned length in below malloc.
>
> This logic is indeed identical to the logic of the "rte_ethtool_get_regs_len" API
> of Ethtool, but the method of using the "rte_eth_dev_get_reg_info" API is the
> case. All users will have similar code logic when using this API.
>
Oh yes, my bad, I overlooked it. It makes complete sense now.
On Sun, 25 Apr 2021 21:02:22 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:
> +
> + memset(®_info, 0, sizeof(reg_info));
> + memset(&dev_info, 0, sizeof(dev_info));
This memset is redundant, rte_eth_dev_info_get already has the same memset.
For the registers, ethdev should be fixed to do it.
> +
> + ret = rte_eth_dev_info_get(i, &dev_info);
> + if (ret) {
> + printf("Error getting device info: %d\n", ret);
> + continue;
> + }
> +
> + ret = rte_eth_dev_get_reg_info(i, ®_info);
> + if (ret) {
> + printf("Error getting device reg info: %d\n", ret);
> + continue;
> + }
On Sun, 25 Apr 2021 21:02:22 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:
> snprintf(file_name, MAX_FILE_NAME_SZ, "%s-port%u",
> + file_prefix, i);
> + fp_regs = fopen(file_name, "wb");
> + if (fp_regs == NULL) {
> + printf("Error during opening '%s' for writing\n",
> + file_name);
> + } else {
> + if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=
> + buf_size)
> + printf("Error during writing %s\n",
> + file_prefix);
> + else
> + printf("dump device (%s) regs successfully, "
> + "driver:%s version:0X%08X\n",
> + dev_info.device->name,
> + dev_info.driver_name, reg_info.version);
> +
> + fclose(fp_regs);
> +
I don't like applications opening and writing an arbitrary file in the
current directory. Any file should be an argument to the application
and optional.
Also, since you are writing binary data, there is no point in using
stdio here.
On 2021/7/18 1:51, Stephen Hemminger wrote:
> On Sun, 25 Apr 2021 21:02:22 +0800
> "Min Hu (Connor)" <humin29@huawei.com> wrote:
>
>> +
>> + memset(®_info, 0, sizeof(reg_info));
>> + memset(&dev_info, 0, sizeof(dev_info));
>
> This memset is redundant, rte_eth_dev_info_get already has the same memset.
> For the registers, ethdev should be fixed to do it.
>
The memset for dev_info is indeed redundant. But the memset for reg_info seems
reasonable, because there are some input parameters in reg_info.
E.g. If info->data is NULL the get_reg_info() fills in the width and length
fields. If non-NULL the registers are put into the buffer pointed at by the data
field.
So, I will remove the redundant memset for dev_info, and keep the memset for reg_info
in the next version.
>> +
>> + ret = rte_eth_dev_info_get(i, &dev_info);
>> + if (ret) {
>> + printf("Error getting device info: %d\n", ret);
>> + continue;
>> + }
>> +
>> + ret = rte_eth_dev_get_reg_info(i, ®_info);
>> + if (ret) {
>> + printf("Error getting device reg info: %d\n", ret);
>> + continue;
>> + }
> .
>
On 2021/7/18 1:53, Stephen Hemminger wrote:
> On Sun, 25 Apr 2021 21:02:22 +0800
> "Min Hu (Connor)" <humin29@huawei.com> wrote:
>
>> snprintf(file_name, MAX_FILE_NAME_SZ, "%s-port%u",
>> + file_prefix, i);
>> + fp_regs = fopen(file_name, "wb");
>> + if (fp_regs == NULL) {
>> + printf("Error during opening '%s' for writing\n",
>> + file_name);
>> + } else {
>> + if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=
>> + buf_size)
>> + printf("Error during writing %s\n",
>> + file_prefix);
>> + else
>> + printf("dump device (%s) regs successfully, "
>> + "driver:%s version:0X%08X\n",
>> + dev_info.device->name,
>> + dev_info.driver_name, reg_info.version);
>> +
>> + fclose(fp_regs);
>> +
>
> I don't like applications opening and writing an arbitrary file in the
> current directory. Any file should be an argument to the application
> and optional.
I think the use of the file here does not seem to be exactly the same as
the behavior you object to. The directories of the file and its name prefix
is passed by user. some suffixes has been added based on user's input.
This is mainly considering the scenario where the user does not specify a
device. If no device is specified, this command will dump regs for all
devices, and their dumped files are distinguished by the suffixes. If the
user is required to give a name to the dumped file of each device, the ease
of use of this command may decreases
>
> Also, since you are writing binary data, there is no point in using
> stdio here.
> .
>
@@ -94,6 +94,9 @@ static char *mempool_name;
/**< Enable iter mempool. */
static uint32_t enable_iter_mempool;
static char *mempool_iter_name;
+/**< Enable dump regs. */
+static uint32_t enable_dump_regs;
+static char *dump_regs_file_prefix;
/**< display usage */
static void
@@ -119,7 +122,8 @@ proc_info_usage(const char *prgname)
" --show-crypto: to display crypto information\n"
" --show-ring[=name]: to display ring information\n"
" --show-mempool[=name]: to display mempool information\n"
- " --iter-mempool=name: iterate mempool elements to display content\n",
+ " --iter-mempool=name: iterate mempool elements to display content\n"
+ " --dump-regs=file-prefix: dump reg to file with the file-prefix\n",
prgname);
}
@@ -226,6 +230,7 @@ proc_info_parse_args(int argc, char **argv)
{"show-ring", optional_argument, NULL, 0},
{"show-mempool", optional_argument, NULL, 0},
{"iter-mempool", required_argument, NULL, 0},
+ {"dump-regs", required_argument, NULL, 0},
{NULL, 0, 0, 0}
};
@@ -288,6 +293,10 @@ proc_info_parse_args(int argc, char **argv)
"iter-mempool", MAX_LONG_OPT_SZ)) {
enable_iter_mempool = 1;
mempool_iter_name = optarg;
+ } else if (!strncmp(long_option[option_index].name,
+ "dump-regs", MAX_LONG_OPT_SZ)) {
+ enable_dump_regs = 1;
+ dump_regs_file_prefix = optarg;
}
break;
case 1:
@@ -1349,6 +1358,85 @@ iter_mempool(char *name)
}
}
+static void
+dump_regs(char *file_prefix)
+{
+#define MAX_FILE_NAME_SZ (MAX_LONG_OPT_SZ + 10)
+ char file_name[MAX_FILE_NAME_SZ];
+ struct rte_dev_reg_info reg_info;
+ struct rte_eth_dev_info dev_info;
+ unsigned char *buf_data;
+ FILE *fp_regs;
+ int buf_size;
+ uint16_t i;
+ int ret;
+
+ snprintf(bdr_str, MAX_STRING_LEN, " dump - Port REG");
+ STATS_BDR_STR(10, bdr_str);
+
+ RTE_ETH_FOREACH_DEV(i) {
+ /* Skip if port is not in mask */
+ if ((enabled_port_mask & (1ul << i)) == 0)
+ continue;
+
+ snprintf(bdr_str, MAX_STRING_LEN, " Port (%u)", i);
+ STATS_BDR_STR(5, bdr_str);
+
+ memset(®_info, 0, sizeof(reg_info));
+ memset(&dev_info, 0, sizeof(dev_info));
+
+ ret = rte_eth_dev_info_get(i, &dev_info);
+ if (ret) {
+ printf("Error getting device info: %d\n", ret);
+ continue;
+ }
+
+ ret = rte_eth_dev_get_reg_info(i, ®_info);
+ if (ret) {
+ printf("Error getting device reg info: %d\n", ret);
+ continue;
+ }
+
+ buf_size = reg_info.length * reg_info.width;
+ buf_data = malloc(buf_size);
+ if (buf_data == NULL) {
+ printf("Error allocating %d bytes buffer\n", buf_size);
+ continue;
+ }
+
+ reg_info.data = buf_data;
+ reg_info.length = 0;
+ ret = rte_eth_dev_get_reg_info(i, ®_info);
+ if (ret) {
+ printf("Error getting regs from device: %d\n", ret);
+ free(buf_data);
+ continue;
+ }
+
+ snprintf(file_name, MAX_FILE_NAME_SZ, "%s-port%u",
+ file_prefix, i);
+ fp_regs = fopen(file_name, "wb");
+ if (fp_regs == NULL) {
+ printf("Error during opening '%s' for writing\n",
+ file_name);
+ } else {
+ if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=
+ buf_size)
+ printf("Error during writing %s\n",
+ file_prefix);
+ else
+ printf("dump device (%s) regs successfully, "
+ "driver:%s version:0X%08X\n",
+ dev_info.device->name,
+ dev_info.driver_name, reg_info.version);
+
+ fclose(fp_regs);
+ }
+
+ free(buf_data);
+ }
+}
+
int
main(int argc, char **argv)
{
@@ -1454,6 +1542,8 @@ main(int argc, char **argv)
show_mempool(mempool_name);
if (enable_iter_mempool)
iter_mempool(mempool_iter_name);
+ if (enable_dump_regs)
+ dump_regs(dump_regs_file_prefix);
RTE_ETH_FOREACH_DEV(i)
rte_eth_dev_close(i);