Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from Oct 25, 2017
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf291ad
Correction of tracing from child process
jiriproX Oct 24, 2017
23fadd6
Format correction.
jiriproX Oct 24, 2017
5af4545
Incorporation of review commnets
jiriproX Oct 25, 2017
a90e8ac
One more review comment
jiriproX Oct 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 29 additions & 5 deletions src/utils_dpdk.c
View file
Open in desktop
Expand Up @@ -44,8 +44,9 @@
#include "utils_dpdk.h"

#define DPDK_DEFAULT_RTE_CONFIG "/var/run/.rte_config"
#define DPDK_EAL_ARGC 5
#define DPDK_MAX_BUFFER_SIZE (4096 * 4)
#define DPDK_EAL_ARGC 10
// Complete trace should fit into 1024 chars
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated comment.

#define DPDK_MAX_BUFFER_SIZE 896
#define DPDK_CDM_DEFAULT_TIMEOUT 10000

enum DPDK_HELPER_STATUS {
Expand Down Expand Up @@ -182,9 +183,19 @@ int dpdk_helper_eal_config_parse(dpdk_helper_ctx_t *phc, oconfig_item_t *ci) {
status = cf_util_get_string_buffer(child, prefix, sizeof(prefix));
if (status == 0) {
snprintf(phc->eal_config.file_prefix, DATA_MAX_NAME_LEN,
"/var/run/.%s_config", prefix);
"/var/run/.%s_config", prefix);
DEBUG("dpdk_common: EAL:File prefix %s", phc->eal_config.file_prefix);
}
} else if (strcasecmp("LogLevel", child->key) == 0) {
status = cf_util_get_string_buffer(child, phc->eal_config.log_level,
sizeof(phc->eal_config.log_level));
DEBUG("dpdk_common: EAL:LogLevel %s", phc->eal_config.log_level);
} else if (strcasecmp("RteDriverLibPath", child->key) == 0) {
status = cf_util_get_string_buffer(
child, phc->eal_config.rte_driver_lib_path,
sizeof(phc->eal_config.rte_driver_lib_path));
DEBUG("dpdk_common: EAL:RteDriverLibPath %s",
phc->eal_config.rte_driver_lib_path);
} else {
ERROR("dpdk_common: Invalid '%s' configuration option", child->key);
status = -EINVAL;
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

argp[argc++] = "--log-level";
argp[argc++] = phc->eal_config.log_level;
}
if (strcasecmp(phc->eal_config.rte_driver_lib_path, "") != 0) {
argp[argc++] = "-d";
argp[argc++] = phc->eal_config.rte_driver_lib_path;
}

assert(argc <= (DPDK_EAL_ARGC * 2 + 1));

int ret = rte_eal_init(argc, argp);
Expand Down Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

data_avail);
if (data_avail < 0) {
if (errno != EINTR || errno != EAGAIN) {
char errbuf[ERR_BUF_SIZE];
Expand All @@ -705,10 +727,12 @@ static void dpdk_helper_check_pipe(dpdk_helper_ctx_t *phc) {
}
}
while (data_avail) {
int nbytes = read(phc->pipes[0], buf, sizeof(buf));
int nbytes = read(phc->pipes[0], buf, (sizeof(buf) - 1));
DEBUG("%s:dpdk_helper_check_pipe: read nbytes=%d", phc->shm_name, nbytes);
if (nbytes <= 0)
break;
sstrncpy(out, buf, nbytes);
buf[nbytes] = '\n';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sstrncpy(out, buf, (nbytes + 1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

DEBUG("%s: helper process:\n%s", phc->shm_name, out);
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/utils_dpdk.h
View file
Open in desktop
Expand Up @@ -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];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

};
typedef struct dpdk_eal_config_s dpdk_eal_config_t;

Expand Down