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

Update Readme.md #1624

Closed
wants to merge 2 commits into from
Closed

Update Readme.md #1624

wants to merge 2 commits into from

Conversation

gkintc
Copy link

@gkintc gkintc commented Dec 14, 2023

Updated Readme.md with notes and Troubleshooting section for a clear picture.

Copy link
Contributor

@mythi mythi left a comment

Choose a reason for hiding this comment

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

thanks for the updates! there are important updates/clarifications here but I think we need to find a bit better places for them

dpdk-test-crypto-perf --no-telemetry -l 4,112,116 -a $QAT1 -- --ptest throughput --devtype crypto_qat --optype cipher-only --cipher-algo aes-cbc --cipher-op encrypt --cipher-key-sz 16 --total-ops 10000000 --burst-sz 32 --buffer-sz 64


# Error-3
Copy link
Contributor

Choose a reason for hiding this comment

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

where do you see this? users are not expected to install drivers themselves.

## Solution-1
modprobe vfio-pci

## Error-2 - invalid options dpdk-test-crypto-perf
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can drop this error and remove the manual testing command. It's a bit outdated anyways.

failed to set device ID 4941 for vfio-pci. Driver module not loaded?

## Solution-1
modprobe vfio-pci
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we are missing this step from "prerequisites". Perhaps it is better to just add "Have vfio-pci module loaded." in that section and we should be able to drop this too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll push a patch shortly that should help prevent this. The NFD rule will check for vfio-pci module to exist before labeling the node.

Comment on lines +261 to +265
## NOTES
1. The initcontainer deployment will split the qat resources into two: cy and dc.
2. cy stands for crypto and dc for compression. Depending on what your workload needs, you can then request the either for the container.


Copy link
Contributor

Choose a reason for hiding this comment

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

  1. is documented in "automatic provision" table. 1. is not completely true: the "split" is the default from the driver so the initcontainer has no role in that.

@@ -187,6 +187,19 @@ For example, `qat.intel.com/generic: <number of devices>` for a container reques

For a DPDK-based workload, you may need to add hugepage request and limit.

##### NOTES
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need a separate note for this. Can we just add in parenthesis on line 186 something like: "(in case for 4th Gen Xeon and later, use cy or dc instead of generic for crypto and compression functionality, respectively)".

Comment on lines +199 to +200
Also, you can check by describing the workr node and verify the Allocatable resources.
![Alt text](worker-node-describe.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

add this as as text output in "verify plugin registration" below the existing sample.

@mythi
Copy link
Contributor

mythi commented Mar 8, 2024

@gkintc do you plan to continue with this PR?

@mythi
Copy link
Contributor

mythi commented Apr 11, 2024

we will rework this as part of #1687. closing due to no activity

@mythi mythi closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants