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

Added functions to control the cache eviction #31

Open
wants to merge 4 commits into
base: tuner
Choose a base branch
from

Conversation

gobinathsr
Copy link

@gobinathsr gobinathsr commented Nov 7, 2020

Added functions to control the cache eviction

Root Cause:
Assigned credit for a pacer period is utilized without considering the bytes in flight.
Hence bytes flight increases a lot above the allowed maximum threshold level and leading to the cache eviction & leading to cache miss.

Fix info:
Added the below functions to control the cache eviction

  1. is_credit_available() - When credit availably checked, also checking bytes in flight is with in the maximum allowed bytes
    Default enabled with the macro SPDK_NVMF_RDMA_IO_PACER_ALLOW_WITHIN_CREDIT_ONLY

  2. is_entry_size_within_limit() - When processing the request from the pacer check if the request size is within in the allowed bytes range
    Default disabled with the macro SPDK_NVMF_RDMA_IO_PACER_ALLOW_IO_WITHIN_AVAIL - Reason to allow processing a single request to consider the slow disk/other slowness in processing the request

Added the macros to configure the tuner type and the proposals. Hence with the change will send the parameters from the test script to configure dynamically.

[Features/Functional areas and/or component interfaces affected]
General SPDK

[Whether the change affects how hardware is programmed]
Yes - Configures request out of pacer queue to be processed

Local testing:
Tested with Tuner2 - Test14 test case and below is the test result

test_14
CPU mask: 0xF0 Num cores:4 IO pacer period:5600 Adjusted period:22400
./test_NoArg.sh: line 336: 708492 Terminated tail -f > rpc_pipe
| QD | BW | BW Max | WIRE BW | AVG LAT, us | BW STDDEV | L3 Hit Rate | Bufs in-flight (MiB) | Pacer period, us
| 256 | 185.6 | 191.0 | 188.9947 | 2891.1 | .6 | 99.3 | 29.6 (3.7) | 22.5

To show up logs per second
Assigned credit for a pacer period is utilized without considering the bytes in flight.
Hence bytes flight increases a lot above the allowed maximum threshold level and leading to the cache eviction & leading to cache miss.

Fix info:
Added the below functions to control the cache eviction
1. is_credit_available() - When credit availably checked, also checking bytes in flight is with in the maximum allowed bytes
   Default enabled with the macro SPDK_NVMF_RDMA_IO_PACER_ALLOW_WITHIN_CREDIT_ONLY

2. is_entry_size_within_limit() - When processing the request from the pacer check if the request size is within in the allowed bytes range
   Default disabled with the macro SPDK_NVMF_RDMA_IO_PACER_ALLOW_IO_WITHIN_AVAIL - Reason to allow processing a single request to consider the slow disk/other slowness in processing the request

Added the macros to configure the tuner type and the proposals. Hence with the change will send the parameters from the test script to configure dynamically.

[Features/Functional areas and/or component interfaces affected]
General SPDK

[Whether the change affects how hardware is programmed]
Yes - Configures request out of pacer queue to be processed

Local testing:
Tested with Tuner2 - Test14 test case and below is the test result

test_14
CPU mask: 0xF0 Num cores:4 IO pacer period:5600 Adjusted period:22400
./test_NoArg.sh: line 336: 708492 Terminated              tail -f > rpc_pipe
| QD         | BW         |  BW Max     | WIRE BW    | AVG LAT, us     | BW STDDEV  | L3 Hit Rate     | Bufs in-flight (MiB) | Pacer period, us
| 256        | 185.6      |  191.0      | 188.9947   | 2891.1          | .6         | 99.3            | 29.6 (3.7)           | 22.5
@gobinathsr gobinathsr changed the title Test with updating the comment info Added functions to control the cache eviction Nov 9, 2020
@gobinathsr gobinathsr requested a review from nv-shri November 9, 2020 01:56
@gobinathsr
Copy link
Author

Pls ignore the first 3 commit added for testing with pull request. On merging, will squash and merge as single commit.

@ldewangan
Copy link

It seems multiple changes are there in single patch.
Can you do the split?

  1. Fix the allowed bytes and bytes_in_flight issue
  2. Fix the pacer expiry and credit allocation method
  3. Add the tuner’s algorithms

@swx-snap
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants