Correction of tracing from child process and new EAL params support #2505

jiriproX · 2017-10-24T13:54:39Z

Correction of tracing from child process

There was read 16kb from pipe but only less then 1kb was traced.
Rest of trace data was lost.
Added support for two EAL parameters:
LogLevel - optional EAL paramenter which enable debug traces from rte libs
If parameter is not used default trace level = 7 (INFO) is used
LogLevel "8" - (DEBUG) can be set to collectd.conf into dpdkstat
EAL section
RteDriverLibPath - optional EAL parameter which enable loading of shared
pmd driver libs. Param value can be full path to single
pmd diriver lib or directory where pmd driver libs are
located. E.g.: "/usr/lib/dpdk-pmd/librte_pmd_i40e.so"
or "/usr/lib/dpdk-pmd"

Signed-off-by: Jiri Prokes jirix.x.prokes@intel.com

- There was read 16kb from pipe but only less then 1kb was traced. Rest of trace data was lost. Added support for two EAL parameters: LogLevel - optional EAL paramenter which enable debug traces from rte libs If parameter is not used default trace level = 7 (INFO) is used LogLevel "8" - (DEBUG) can be set to collectd.conf into dpdkstat EAL section RteDriverLibPath - optional EAL parameter which enable loading of shared pmd driver libs. Param value can be full path to single pmd diriver lib or directory where pmd driver libs are located. E.g.: "/usr/lib/dpdk-pmd/librte_pmd_i40e.so" or "/usr/lib/dpdk-pmd" Signed-off-by: Jiri Prokes <jirix.x.prokes@intel.com>

Signed-off-by: Jiri Prokes <jirix.x.prokes@intel.com>

maryamtahhan

make sure to also update collectd.conf.in and collectd.conf.pod

octo

Hi @jiriproX,

thank you very much for your pull request! I've left some review comments inline.

Best regards,
—octo

octo · 2017-10-24T17:58:14Z

src/utils_dpdk.c

-#define DPDK_EAL_ARGC 5
-#define DPDK_MAX_BUFFER_SIZE (4096 * 4)
+#define DPDK_EAL_ARGC 10
+// Complete trace should fit into 1024 chars


I don't understand this comment: the trace should fit into 1024 bytes, that's why we set DPDK_MAX_BUFFER_SIZE to 896?

I've updated comment.

octo · 2017-10-24T17:59:06Z

src/utils_dpdk.c

@@ -493,6 +504,15 @@ static int dpdk_helper_eal_init(dpdk_helper_ctx_t *phc) {
  argp[argc++] = "--proc-type";
  argp[argc++] = "secondary";

+  if (strcasecmp(phc->eal_config.log_level, "") != 0) {


Nit, at your option: You don't need the case-insensitive variant when comparing with an empty string. Here and below.

You are right. Nevertheless I just reused what was used for --socket-mem. I would let it there.

octo · 2017-10-24T18:02:29Z

src/utils_dpdk.c

@@ -697,6 +717,8 @@ static void dpdk_helper_check_pipe(dpdk_helper_ctx_t *phc) {
      .fd = phc->pipes[0], .events = POLLIN,
  };
  int data_avail = poll(&fds, 1, 0);
+  DEBUG("%s:dpdk_helper_check_pipe: poll data_avail=%d", phc->shm_name,


Ideally this should have a "dpdk_common:" prefix like the other debug messages.

I don't think so. It is consistent with traces which were already present there.

octo · 2017-10-24T18:03:40Z

src/utils_dpdk.h

@@ -51,6 +51,8 @@ struct dpdk_eal_config_s {
  char memory_channels[DATA_MAX_NAME_LEN];
  char socket_memory[DATA_MAX_NAME_LEN];
  char file_prefix[DATA_MAX_NAME_LEN];
+  char log_level[DATA_MAX_NAME_LEN];
+  char rte_driver_lib_path[DATA_MAX_NAME_LEN];


I'd use PATH_MAX as the size of this buffer – the define is better suited for this purpose.

octo · 2017-10-24T18:05:50Z

src/utils_dpdk.c

    if (nbytes <= 0)
      break;
-    sstrncpy(out, buf, nbytes);
+    buf[nbytes] = '\n';


Why \n? Did you intend to nil-terminate the string? In that case use buf[nbytes] = 0; instead.

It is just defined char. sstrncpy ignore char at that position anyway. Nevertheless you are right '\0' will be better here.

octo · 2017-10-24T18:07:13Z

src/utils_dpdk.c

    if (nbytes <= 0)
      break;
-    sstrncpy(out, buf, nbytes);
+    buf[nbytes] = '\n';
+    sstrncpy(out, buf, (nbytes + 1));


Use sizeof(out) as the third parameter (after making sure that buf is nil-terminated, see above).

sstrncpy does not check nil in string, so it will copy complete buffer. In our case only real data are copied.

sstrncpy() expects a nil-terminated string as second argument and copies it into the memory pointed to by the first argument. It will, however, not copy more than the third argument.

While your variant also works, it is uncommon and therefore confusing. Please use sizeof(out) instead of nbytes + 1.

I've changed that as you requested. Nevertheless if I understood documentation right then it is less efficient than original one. It always write complete buffer even if read nbytes is only few chars. Rest of buffer is filled with zeros.

Signed-off-by: Jiri Prokes <jirix.x.prokes@intel.com>

octo

Hi @jiriproX,

thank you very much for your update! Once the ssstrncpy() call is fixed this is good to go.

Best regards,
—octo

octo · 2017-10-25T14:44:34Z

src/utils_dpdk.c

    if (nbytes <= 0)
      break;
-    sstrncpy(out, buf, nbytes);
+    buf[nbytes] = '\n';
+    sstrncpy(out, buf, (nbytes + 1));


sstrncpy() expects a nil-terminated string as second argument and copies it into the memory pointed to by the first argument. It will, however, not copy more than the third argument.

While your variant also works, it is uncommon and therefore confusing. Please use sizeof(out) instead of nbytes + 1.

Signed-off-by: Jiri Prokes <jirix.x.prokes@intel.com>

octo

LGTM, thanks @jiriproX!

jiriproX added 2 commits October 24, 2017 06:17

Format correction.

23fadd6

Signed-off-by: Jiri Prokes <jirix.x.prokes@intel.com>

maryamtahhan reviewed Oct 24, 2017

View reviewed changes

octo added the Feature label Oct 24, 2017

octo reviewed Oct 24, 2017

View reviewed changes

Incorporation of review commnets

5af4545

Signed-off-by: Jiri Prokes <jirix.x.prokes@intel.com>

octo requested changes Oct 25, 2017

View reviewed changes

One more review comment

a90e8ac

Signed-off-by: Jiri Prokes <jirix.x.prokes@intel.com>

octo approved these changes Oct 25, 2017

View reviewed changes

octo added the Automerge Labels PRs to be merged by a bot once approved label Oct 25, 2017

collectd-bot merged commit b7a8e04 into collectd:master Oct 25, 2017

maryamtahhan mentioned this pull request Nov 22, 2017

dpdkstat and dpdkevent runtime error message "Cannot init tail queues for objects?" #2240

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correction of tracing from child process and new EAL params support #2505

Correction of tracing from child process and new EAL params support #2505

jiriproX commented Oct 24, 2017

maryamtahhan left a comment

octo left a comment

octo Oct 24, 2017

jiriproX Oct 25, 2017

octo Oct 24, 2017

jiriproX Oct 25, 2017

octo Oct 24, 2017

jiriproX Oct 25, 2017

octo Oct 24, 2017

jiriproX Oct 25, 2017

octo Oct 24, 2017

jiriproX Oct 25, 2017

octo Oct 24, 2017

jiriproX Oct 25, 2017

octo Oct 25, 2017

jiriproX Oct 25, 2017

octo left a comment

octo Oct 25, 2017

octo left a comment

Correction of tracing from child process and new EAL params support #2505

Correction of tracing from child process and new EAL params support #2505

Conversation

jiriproX commented Oct 24, 2017

maryamtahhan left a comment

Choose a reason for hiding this comment

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octo left a comment

Choose a reason for hiding this comment