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

vsock: Move iter outside of while loop in process_rx_queue #411

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

vsock: Move iter outside of while loop in process_rx_queue #411

wants to merge 2 commits into from

Conversation

cutelizebin
Copy link
Contributor

@cutelizebin cutelizebin commented Jul 29, 2023 •
edited
Loading

the iter() function is used for produce a queue iterator to iterate over the descriptors.

But usually, it shouldn't be in the while loop, which might brings more unnecessary overhead.

So move iter outside of the while loop.

And the process_tx_queue has the same problem, maybe we can fix it, too.

Summary of the PR

Please summarize here why the changes in this PR are needed.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@cutelizebin cutelizebin force-pushed the lizebin/move-iter-out branch 4 times, most recently from 21620a4 to 8ea683a Compare July 29, 2023 16:04
@stefano-garzarella
Copy link
Member

And the process_tx_queue has the same problem, maybe we can fix it, too.

Can we fix also that in this PR? (separate commit)

@cutelizebin
Copy link
Contributor Author

And the process_tx_queue has the same problem, maybe we can fix it, too.

Can we fix also that in this PR? (separate commit)

Sure. And it is done now.

@cutelizebin cutelizebin force-pushed the lizebin/move-iter-out branch 2 times, most recently from 6ed15e5 to 6f390a9 Compare July 31, 2023 16:18
@stefano-garzarella
Copy link
Member

@cutelizebin LGTM but clippy now is suggesting to use a for loop.

Curiosity: can creating the iterator only at the beginning result in performance losses, since we might not see descriptors added while we are processing others?

Please also do a rebase since main was modified.

@cutelizebin
Copy link
Contributor Author

cutelizebin commented Aug 2, 2023 •
edited
Loading

@cutelizebin LGTM but clippy now is suggesting to use a for loop.

Curiosity: can creating the iterator only at the beginning result in performance losses, since we might not see descriptors added while we are processing others?

Please also do a rebase since main was modified.

  1. the performance loss issue you mention seems a design trade-off. Moving iter() outside can speed up the handling of desctiptors placed in avail ring before the iter is called. But it might delay the descriptors placed after iter() is called. But for the overall thput, I think it may benefit to move iter outside, cause it involves less iter() callings. And FYI, the reason why I notice this issue is that, we are using vhost-user-vsock in our product, while our guest memory is in the remote side. It means that the memory access to our guest memory is much more expensive than local mem access. And here, the iter() function involves a avail idx reading from guest memory, which brings us a little bit more cost. And it does bring us some speed-up in our product to move iter() outside the loop.
  2. will change the while let into for loop to fit in the cllipy demand.
  3. will do the rebase

@cutelizebin cutelizebin force-pushed the lizebin/move-iter-out branch 3 times, most recently from 771d498 to 698cd1c Compare August 2, 2023 17:43
@stefano-garzarella
Copy link
Member 

1. the performance loss issue you mention seems a design trade-off. Moving iter() outside can speed up the handling of desctiptors placed in avail ring before the iter is called. But it might delay the descriptors placed after iter() is called. But for the overall thput, I think it may  benefit to move iter outside, cause it involves less iter() callings.  And FYI, the reason why I notice this issue is that, we are using vhost-user-vsock in our product, while our guest memory is in the remote side. It means that the memory access to our guest memory is much more expensive than local mem access. And here, the iter() function involves a  avail idx reading from guest memory, which brings us a little bit more cost. And it does bring us some speed-up in our product to move iter() outside the loop.

So, at this point, what about adding an external loop where we create the iterator to get the best of both approaches?

2. will change the while let into for loop to fit in the cllipy demand.

Please include that changes in the 2 commits, the warning seems related to that changes, and we prefer to avoid changing code introduced in the same PR.

@cutelizebin
Copy link
Contributor Author 

1. the performance loss issue you mention seems a design trade-off. Moving iter() outside can speed up the handling of desctiptors placed in avail ring before the iter is called. But it might delay the descriptors placed after iter() is called. But for the overall thput, I think it may  benefit to move iter outside, cause it involves less iter() callings.  And FYI, the reason why I notice this issue is that, we are using vhost-user-vsock in our product, while our guest memory is in the remote side. It means that the memory access to our guest memory is much more expensive than local mem access. And here, the iter() function involves a  avail idx reading from guest memory, which brings us a little bit more cost. And it does bring us some speed-up in our product to move iter() outside the loop.

So, at this point, what about adding an external loop where we create the iterator to get the best of both approaches?

2. will change the while let into for loop to fit in the cllipy demand.

Please include that changes in the 2 commits, the warning seems related to that changes, and we prefer to avoid changing code introduced in the same PR.

They all look good. I'll do the both.

@cutelizebin cutelizebin force-pushed the lizebin/move-iter-out branch 3 times, most recently from c7b1df0 to dc7148c Compare September 7, 2023 16:36
@cutelizebin
vsock: Move iter outside of loop in process_rx_queue
be8a7ac 
the iter() function is used for produce a queue iterator to iterate over
the descriptors.

But usually, it shouldn't be in the while loop, which might brings more
unnecessary overhead.

So move `iter` outside of the while loop.

And the process_tx_queue has the same problem, maybe we can fix it, too.

Signed-off-by: Li Zebin <cutelizebin@gmail.com>
@cutelizebin
vsock: Move iter outside of loop in process_tx_queue.
ae31c18 
the iter() function is used for produce a queue iterator to iterate over the descriptors.

But usually, it shouldn't be in the while loop, which might brings more unnecessary overhead.

So move iter outside of the while loop.

Signed-off-by: Li Zebin <cutelizebin@gmail.com>
stefano-garzarella
stefano-garzarella reviewed Sep 13, 2023
View reviewed changes
Copy link
Member

@stefano-garzarella stefano-garzarella 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 update!
I left some comments.

I'd also suggest updating the commit descriptions, since we now have 2 nested loops and also tx loop covered.

crates/vsock/src/vhu_vsock_thread.rs
} else {
queue.iter(mem).unwrap().go_to_previous_position();
break;
let mut iter_has_elemnt = true;
Copy link
Member

Choose a reason for hiding this comment

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

iter_has_element is less cryptic IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for replying. I'll fix these.

crates/vsock/src/vhu_vsock_thread.rs
PKT_HEADER_SIZE + pkt.len() as usize
} else {
queue.iter(mem).unwrap().go_to_previous_position();
break;
Copy link
Member

Choose a reason for hiding this comment

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

Now that we have 2 loops nested, IIUC this should exit both loops, so we may need to replace it with a return Ok(used_any), or set iter_has_elemnt to false.

If you prefer the last one, maybe then better to change the name of that variable to something like processing_rx.

crates/vsock/src/vhu_vsock_thread.rs
match rx_queue_type {
RxQueueType::Standard => {
if !self.thread_backend.pending_rx() {
break;
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I think the original idea was to stop processing new descriptors, since we don't have any packet to copy in it.

crates/vsock/src/vhu_vsock_thread.rs
break;
RxQueueType::RawPkts => {
if !self.thread_backend.pending_raw_pkts() {
break;
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

crates/vsock/src/vhu_vsock_thread.rs
.iter(mem)
.unwrap()
.go_to_previous_position();
break;
Copy link
Member

Choose a reason for hiding this comment

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

And maybe also here.

@stefano-garzarella
Copy link
Member

@cutelizebin it's been a long time since this PR has been open. Do you have time to complete it? We'd like to merge it.

@cutelizebin
Copy link
Contributor Author

cutelizebin commented Sep 16, 2024 •
edited
Loading

@stefano-garzarella Apologies for being away for so long. I'll address the issues you pointed out next week and ensure I'm caught up with the main branch. And thank you for your patience!

@stefano-garzarella
Copy link
Member

@cutelizebin don't worry :-) I'm glad you're back to work on this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefano-garzarella stefano-garzarella stefano-garzarella left review comments

@mathieupoirier mathieupoirier Awaiting requested review from mathieupoirier

@stsquad stsquad Awaiting requested review from stsquad stsquad is a code owner

@vireshk vireshk Awaiting requested review from vireshk vireshk is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cutelizebin @stefano-garzarella