OCPBUGS-18662:rps: trigger udev even per queue #816

Tal-or · 2023-09-27T15:06:26Z

Instead of trigger udev event per physical device creation,
triggers it per queue creation so in case of queue resize
(i.e. more queues are added) it applies the correct rps mask on the
new created queues as well.

Instead of trigger udev event per physical device creation, triggers it per queue creation so in case of queue resize (i.e. more queues are added) it applies the correct rps mask on the new created queues as well. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

With the new udev rule, the full path passes to the systemd unit. Hence, we don't need jump through loops in order to figure out the physical device location. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

openshift-ci-robot · 2023-09-27T15:06:31Z

@Tal-or: This pull request references Jira Issue OCPBUGS-18662, which is invalid:

expected the bug to target the "4.15.0" version, but it targets "4.14.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Instead of trigger udev event per physical device creation,
triggers it per queue creation so in case of queue resize
(i.e. more queues are added) it applies the correct rps mask on the
new created queues as well.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Tal-or · 2023-09-27T15:07:10Z

/jira refresh

openshift-ci-robot · 2023-09-27T15:07:13Z

@Tal-or: This pull request references Jira Issue OCPBUGS-18662, which is invalid:

expected the bug to target only the "4.15.0" version, but multiple target versions were set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Tal-or · 2023-09-27T15:07:27Z

/jira refresh

openshift-ci-robot · 2023-09-27T15:07:35Z

@Tal-or: This pull request references Jira Issue OCPBUGS-18662, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (mniranja@redhat.com), skipping review request.

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Tal-or · 2023-09-27T15:12:10Z

/hold
concerns were raised about performance degradation in case this script would against many queues in short period of time (especially at boot time)
/cc @yanirq @MarSik @pabeni

openshift-ci · 2023-09-27T15:12:16Z

@Tal-or: GitHub didn't allow me to request PR reviews from the following users: pabeni.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/hold
concerns were raised about performance degradation in case this script would against many queues in short period of time (especially at boot time)
/cc @yanirq @MarSik @pabeni

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pabeni · 2023-09-29T07:05:38Z

assets/performanceprofile/configs/99-netdev-physical-rps.rules

@@ -1 +1 @@
-SUBSYSTEM=="net", ACTION=="add", ENV{DEVPATH}!="/devices/virtual/net/*", TAG+="systemd", ENV{SYSTEMD_WANTS}="update-rps@%k.service"
+SUBSYSTEM=="queues", ACTION=="add", ENV{DEVPATH}=="/devices/pci*/queues/rx*", TAG+="systemd", ENV{SYSTEMD_WANTS}="update-rps@%p.service"


Here the devpath for patching events is almost full sysfs queue directory - just missing the '/sys' prefix...

AFAIK the path should start from /devices and not from /sys this is how it worked so far.

@Tal-or: I was not clear in the sentence above, I'm sorry. What I mean is that since the devpath for patching events is almost full sysfs queue directory, we can exploit such fact to simplify the action, as hopefully better described in my comment on set-rps-mask.sh.
The above sentence and the comment there are supposed to be read one after another as part of a the same message

pabeni · 2023-09-29T07:23:46Z

assets/performanceprofile/scripts/set-rps-mask.sh

-function find_dev_dir {
-  systemd_devs=$(systemctl list-units -t device | grep sys-subsystem-net-devices | cut -d' ' -f1)
+# get the path for the queues
+queues_path=${de_escape_path%/rx*}


... so here you should be better off not trimming the trailing part and avoiding the loop below, with something alike:

queue_path=${de_escape_path//queues/rx//queues/rx-}
echo "${mask}" > "${queue_path}"

Side question: why is this script invoked via a systemd unit and not directly by the udev rule?

Additionally, is devpath escaping really needed here? Otherwise you can replace '%i' with '%I' in the unit file and avoid the de-escaping above.

why is this script invoked via a systemd unit and not directly by the udev rule?

I think the reason is because then we can use it as oneshot service but maybe @MarSik knows better

The loop was just to make sure we're on the safe side and nothing get missed, but yes removing the loop should be suffice as well

pabeni · 2023-09-29T07:30:44Z

test/e2e/performanceprofile/testdata/render-expected-output/manual_machineconfig.yaml

+          verification: {}
+        group: {}
+        mode: 448
+        path: /usr/local/bin/set-rps-mask.sh


out of sheer ignorance on my side...

the following file:

test/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yaml

contains the 'old' (prior to this patchset) version of set-rps-mask.sh. Should such file be updated, too?

Yea, we have a make target named redner-sync for that, but it got break.
#824 should fix it, but this PR not depends on it since I fixed it manually here.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

pabeni · 2023-10-13T10:49:29Z

assets/performanceprofile/scripts/set-rps-mask.sh

-queues_path=${de_escape_path%/rx*}
+queue_num=${path#*queues/}
+# replace '/' with '-'
+queue_num="${queue_num/\//-}"


minor nit: is this substitution required? If I read correctly $path is not escaped anymore and this is basically de-escaping the queue nr part

Another minor note: you could possibly mention in the commit message that this change additionally avoid looping on all the rx queue, just for better future memory ;)

both nits are not intended to block the MR, just to be considered if a new version should be needed for any reason

minor nit: is this substitution required? If I read correctly $path is not escaped anymore and this is basically de-escaping the queue nr part

when the $path is not escaped the queue number looks like rx\n and not rx-n as I would expect.
this is why this substitution is necessary.

pabeni · 2023-10-13T10:57:28Z

LGTM! some minor comments on patch 3, but really not blocking

Tal-or · 2023-10-15T12:11:53Z

/hold cancel

yanirq · 2023-10-15T12:25:16Z

/approve
@MarSik PTAL for a final LGTM

MarSik

/lgtm

openshift-ci · 2023-10-16T13:26:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MarSik, Tal-or, yanirq

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [MarSik,yanirq]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-10-16T15:37:45Z

/retest-required

Remaining retests: 0 against base HEAD e5181f4 and 2 for PR HEAD 73ac2ab in total

openshift-ci · 2023-10-16T17:19:18Z

@Tal-or: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2023-10-16T17:27:46Z

@Tal-or: Jira Issue OCPBUGS-18662: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-18662 has been moved to the MODIFIED state.

In response to this:

Instead of trigger udev event per physical device creation,
triggers it per queue creation so in case of queue resize
(i.e. more queues are added) it applies the correct rps mask on the
new created queues as well.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-merge-robot · 2023-10-17T13:39:47Z

Fix included in accepted release 4.15.0-0.nightly-2023-10-17-065657

Tal-or · 2023-10-17T14:04:34Z

/cherrypick release-4.14

yanirq · 2023-10-17T21:59:24Z

/cherry-pick release-4.14

openshift-cherrypick-robot · 2023-10-17T22:01:59Z

@yanirq: new pull request created: #832

In response to this:

/cherry-pick release-4.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Tal-or added 2 commits September 27, 2023 17:58

rps: simplifiy set-rps-mask.sh script

9b24a46

With the new udev rule, the full path passes to the systemd unit. Hence, we don't need jump through loops in order to figure out the physical device location. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Sep 27, 2023

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Sep 27, 2023

openshift-ci bot requested review from dagrayvid and MarSik September 27, 2023 15:08

openshift-ci bot requested a review from yanirq September 27, 2023 15:12

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2023

pabeni reviewed Sep 29, 2023

View reviewed changes

Tal-or added 2 commits October 12, 2023 19:25

rps: do no escape instance name

0bda6e2

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

render-sync

73ac2ab

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

Tal-or force-pushed the udev_per_queue branch from e6bf8ff to 73ac2ab Compare October 12, 2023 16:50

pabeni reviewed Oct 13, 2023

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2023

MarSik approved these changes Oct 16, 2023

View reviewed changes

openshift-ci bot assigned MarSik Oct 16, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2023

openshift-ci bot merged commit ee568b8 into openshift:master Oct 16, 2023

openshift-cherrypick-robot mentioned this pull request Oct 17, 2023

[release-4.14] OCPBUGS-21845:rps: trigger udev event per queue #832 #832

Merged

Tal-or deleted the udev_per_queue branch June 10, 2024 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-18662:rps: trigger udev even per queue #816

OCPBUGS-18662:rps: trigger udev even per queue #816

Tal-or commented Sep 27, 2023

openshift-ci-robot commented Sep 27, 2023

Tal-or commented Sep 27, 2023

openshift-ci-robot commented Sep 27, 2023

Tal-or commented Sep 27, 2023

openshift-ci-robot commented Sep 27, 2023

Tal-or commented Sep 27, 2023

openshift-ci bot commented Sep 27, 2023

pabeni Sep 29, 2023

Tal-or Oct 12, 2023

pabeni Oct 12, 2023

pabeni Sep 29, 2023

pabeni Sep 29, 2023

Tal-or Oct 12, 2023

Tal-or Oct 12, 2023

pabeni Sep 29, 2023

Tal-or Oct 12, 2023

pabeni Oct 13, 2023 •

edited

Loading

Tal-or Oct 15, 2023

pabeni commented Oct 13, 2023

Tal-or commented Oct 15, 2023

yanirq commented Oct 15, 2023

MarSik left a comment

openshift-ci bot commented Oct 16, 2023

openshift-ci-robot commented Oct 16, 2023

openshift-ci bot commented Oct 16, 2023

openshift-ci-robot commented Oct 16, 2023

openshift-merge-robot commented Oct 17, 2023

Tal-or commented Oct 17, 2023

yanirq commented Oct 17, 2023

openshift-cherrypick-robot commented Oct 17, 2023

		@@ -1 +1 @@
		SUBSYSTEM=="net", ACTION=="add", ENV{DEVPATH}!="/devices/virtual/net/*", TAG+="systemd", ENV{SYSTEMD_WANTS}="update-rps@%k.service"
		SUBSYSTEM=="queues", ACTION=="add", ENV{DEVPATH}=="/devices/pci/queues/rx", TAG+="systemd", ENV{SYSTEMD_WANTS}="update-rps@%p.service"

OCPBUGS-18662:rps: trigger udev even per queue #816

OCPBUGS-18662:rps: trigger udev even per queue #816

Conversation

Tal-or commented Sep 27, 2023

openshift-ci-robot commented Sep 27, 2023

Tal-or commented Sep 27, 2023

openshift-ci-robot commented Sep 27, 2023

Tal-or commented Sep 27, 2023

openshift-ci-robot commented Sep 27, 2023

Tal-or commented Sep 27, 2023

openshift-ci bot commented Sep 27, 2023

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

pabeni Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabeni commented Oct 13, 2023

Tal-or commented Oct 15, 2023

yanirq commented Oct 15, 2023

MarSik left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 16, 2023

openshift-ci-robot commented Oct 16, 2023

openshift-ci bot commented Oct 16, 2023

openshift-ci-robot commented Oct 16, 2023

openshift-merge-robot commented Oct 17, 2023

Tal-or commented Oct 17, 2023

yanirq commented Oct 17, 2023

openshift-cherrypick-robot commented Oct 17, 2023

pabeni Oct 13, 2023 •

edited

Loading