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.

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

issue: 2247939 Add option VMA_VMAD_ENABLED #907

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pasis
Copy link
Member

@pasis pasis commented Jul 27, 2020

This option allows to disable vmad functionality in VMA. Running vmad process
can lead to performance degradation in case of multiple TCP socket state
changes.

@swx-jenkins2
Copy link

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/accl-libvma-pr/3437/ for details (Mellanox internal link).

Copy link
Collaborator

@igor-ivanov igor-ivanov left a comment

Choose a reason for hiding this comment

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

Please consider checking for g_p_agent == NULL instead of safe_mce_sys().vmad_enabled.
It looks as more universal way that independent from approach if VMA has special env variable or not.
In this case it is enough to handle enable/disable just in one place in main().
Probably additional check is redundant for ring_tap because warning should be appeared once on initialization when g_p_agent is initialized on not (see possible warning at output_fatal())

This option allows to disable vmad functionality in VMA. Running vmad process
can lead to performance degradation in case of multiple TCP socket state
changes.

Signed-off-by: Dmytro Podgornyi <[email protected]>
@swx-jenkins2
Copy link

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/accl-libvma-pr/3440/ for details (Mellanox internal link).

@@ -371,6 +371,9 @@ int ring_tap::prepare_flow_message(vma_msg_flow& data, msg_flow_t flow_action,
{
int rc = 0;

if (unlikely(g_p_agent == NULL))
return 0;

Copy link
Collaborator

@igor-ivanov igor-ivanov Nov 2, 2020

Choose a reason for hiding this comment

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

it is needless and incorrect because it conflicts with

	/* create egress rule (redirect traffic from tap device to physical interface) */
	rc = prepare_flow_message(data, VMA_MSG_FLOW_ADD);
	if (rc != 0) {
		ring_logwarn("Add TC rule failed with error=%d", rc);
	}

ring_tap() does not work correctly w/o vma daemon.
So preferable way is to report error once ring_tap() is called. It can be before call or inside constructor.

@@ -400,6 +403,9 @@ int ring_tap::prepare_flow_message(vma_msg_flow& data, msg_flow_t flow_action)
{
int rc = 0;

if (unlikely(g_p_agent == NULL))
return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

the same as above

Copy link
Collaborator

@igor-ivanov igor-ivanov left a comment

Choose a reason for hiding this comment

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

see my comments

@swx-jenkins5
Copy link

Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants