Plugin for getting DPDK ports link status and keep alive events. #2157

krzysztofx · 2017-01-30T13:21:30Z

Dpdkevents plugin collects and reports following events from DPDK based
applications:

link status of network ports bound with DPDK
keep alive events related to DPDK logical cores

In order to get link status plugin forks child process that attaches itself to
DPDK application as secondary DPDK process and fetches link status data using
DPDK API. This is the same approach like for dpdkstat plugin, also the same
utils_dpdk.c helper is utilized.

For getting keep alive events plugin communicates with DPDK application
via shared memory area. This is possible only if keep alive feature
is implemented in monitored application.
More details can be found under link
http://dpdk.org/doc/guides/sample_app_ug/keep_alive.html

Minimal required version of DPDK is 16.07.

Change-Id: Id820407ea933b1ecc31a72b6e344a6ec186ec780
Signed-off-by: Krzysztof Matczak krzysztofx.matczak@intel.com

Dpdkevents plugin collects and reports following events from DPDK based applications: - link status of network ports bound with DPDK - keep alive events related to DPDK logical cores In order to get link status plugin forks child process that attaches itself to DPDK application as secondary DPDK process and fetches link status data using DPDK API. This is the same approach like for dpdkstat plugin, also the same utils_dpdk.c helper is utilized. For getting keep alive events plugin communicates with DPDK application via shared memory area. This is possible only if keep alive feature is implemented in monitored application. More details can be found under link http://dpdk.org/doc/guides/sample_app_ug/keep_alive.html Minimal required version of DPDK is 16.07. Change-Id: Id820407ea933b1ecc31a72b6e344a6ec186ec780 Signed-off-by: Krzysztof Matczak <krzysztofx.matczak@intel.com>

krzysztofx · 2017-02-17T08:40:59Z

@octo @rubenk
Please let me know if you'll find some time for reviewing proposed plugin.
Any comments appreciated.

Thanks
Krzysztof

rubenk · 2017-02-19T12:20:46Z

Makefile.am

+pkglib_LTLIBRARIES += dpdkevents.la
+dpdkevents_la_SOURCES = src/dpdkevents.c src/utils_dpdk.c src/utils_dpdk.h
+dpdkevents_la_CPPFLAGS = $(AM_CPPFLAGS) $(LIBDPDK_CPPFLAGS)
+#dpdkevents_la_CFLAGS = $(AM_CFLAGS) $(BUILD_WITH_DPDK_CFLAGS)


please remove this line.

rubenk · 2017-02-19T12:22:47Z

configure.ac

-		[with_libdpdk="yes"],
+		[
+      with_libdpdk="yes"
+      AC_RUN_IFELSE(


use AC_COMPILE_IFELSE here

see comment below

rubenk · 2017-02-19T12:22:59Z

configure.ac

+        [rte_version_major=0],
+        [rte_version_major=$?]
+      )
+      AC_RUN_IFELSE(


use AC_COMPILE_IFELSE here

AC_COMPILE_IFELSE does only compilation, it doesn't run the AC_LANG_PROGRAM snippet. I need AC_RUN_IFELSE to extract RTE_VER_YEAR and RTE_VER_MONTH values from rte_version.h in runtime.

rubenk · 2017-02-19T12:23:36Z

configure.ac

@@ -2709,7 +2711,37 @@ then
 	SAVE_CPPFLAGS="$CPPFLAGS"
 	CPPFLAGS="$LIBDPDK_CPPFLAGS $CPPFLAGS"
 	AC_CHECK_HEADERS([rte_config.h],
-		[with_libdpdk="yes"],
+		[
+      with_libdpdk="yes"


what's with the indentation?

I'll fix it

rubenk · 2017-02-19T12:27:16Z

configure.ac

@@ -6231,6 +6264,14 @@ fi

 if test "x$with_libdpdk" = "xyes"
 then
+  plugin_dpdkevents="no (DPDK version < 16.07)"


I really dislike version checks. Is there no symbol in DPDK that you can check for existence?

There's no single symbol that would determine if current DPDK instance supports keepalive feature, if that's what you mean. We have to check if version is >= 16.07 this way or other. It would be possible to simplify a bit this logic since DPDK header rte_version.h provides also compound macro RTE_VERSION with major and minor version numbers included, so I could apply it and related changes if you like it.

Ah yes. The thing is, these runtime tests are slow, so perhaps you could abuse the preprocessor and do a compile time test like:

#if RTE_VERSION < RTE_VERSION_NUM(16,7,0,0) #error "version too old" #endif

You mean applying it along with AC_COMPILE_IFELSE, right ? Seems reasonable, I'll do it. Thanks

Yes exactly. Thanks!

rubenk · 2017-02-19T12:30:34Z

src/dpdkevents.c

+#include "sys/mman.h"
+#include "utils_dpdk.h"
+#include "utils_time.h"
+#include "collectd.h"


this should be the first include, separated by a blank line from the others.

rubenk · 2017-02-19T12:37:17Z

src/dpdkevents.c

+  if (nb_ports == 0) {
+    DPDK_CHILD_LOG("dpdkevent-helper: No DPDK ports available. "
+                   "Check bound devices to DPDK driver.\n");
+    return -ENODEV;


I'd just return -1, this is the only way this function can fail.

But looking further I see you never check the return value, so please fix that or make this return void.

rubenk · 2017-02-19T12:40:19Z

src/dpdkevents.c

+}
+
+/* this function is called from helper context */
+int dpdk_helper_command_handler(dpdk_helper_ctx_t *phc, enum DPDK_CMD cmd) {


I don't see this handler being registered anywhere.

This is plugin specific implementation of handler interface declared in utils_dpdk.[ch]

You're right, I missed that, sorry.

rubenk · 2017-02-19T12:41:36Z

src/dpdkevents.c

+      ec->core_info[i].lcore_state = ec->config.keep_alive.shm->core_state[i];
+      ec->core_info[i].read_time = cdtime();
+
+#if RTE_VERSION >= RTE_VERSION_16_07


./configure already checks that you have dpdk >= 16.07, so this is unneeded.

rubenk · 2017-02-19T12:42:45Z

src/dpdkevents.c

+  return 0;
+}
+
+static int dpdk_events_keep_alive_dispatch(dpdk_helper_ctx_t *phc) {


you never check the return value and this function always return 0, so please make this void.

rubenk · 2017-02-19T12:45:40Z

src/dpdkevents.c

+
+  if (g_hc == NULL) {
+    ERROR(DPDK_EVENTS_PLUGIN ": plugin not initialized.");
+    return -EINVAL;


Just return -1.

rubenk · 2017-02-19T12:47:13Z

src/dpdkevents.c

+
+static int dpdk_events_shutdown(void) {
+  DPDK_EVENTS_TRACE();
+  int ret = 0;


no need to initialize ret here.

rubenk · 2017-02-19T12:48:07Z

I left some comments inline, I'll see if I can do a more thorough review soon.

krzysztofx · 2017-02-20T09:24:29Z

@rubenk

Thanks for your comments. I'll address them as soon as possible.

RTE version check changed to compile time. Adressing PR review comments. Minor format changes Change-Id: Ie345a6009a291490323f614183362dfce8e6a5e6

rubenk reviewed Feb 19, 2017

View reviewed changes

dpdkevents: PR comments addressed

13a93e2

RTE version check changed to compile time. Adressing PR review comments. Minor format changes Change-Id: Ie345a6009a291490323f614183362dfce8e6a5e6

rubenk merged commit 9fdcbb6 into collectd:master Feb 24, 2017

krzysztofx deleted the dpdkevents_upstream branch February 28, 2017 15:24

octo added this to the 5.8 m 8471 ilestone Oct 11, 2017

octo added the New plugin label Oct 11, 2017

maryamtahhan mentioned this pull request Oct 12, 2017

Next collectd release #2420

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Plugin for getting DPDK ports link status and keep alive events. #2157

Plugin for getting DPDK ports link status and keep alive events. #2157

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Plugin for getting DPDK ports link status and keep alive events. #2157

Plugin for getting DPDK ports link status and keep alive events. #2157

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!