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

cutelizebin · 2023-07-29T05:31:31Z

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.

stefano-garzarella · 2023-07-31T09:57:10Z

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 · 2023-07-31T16:08:09Z

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.

stefano-garzarella · 2023-08-01T16:13:27Z

@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 · 2023-08-02T17:28:00Z

@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.

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.
will change the while let into for loop to fit in the cllipy demand.
will do the rebase

stefano-garzarella · 2023-08-03T08:07:58Z

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 · 2023-08-09T01:13:17Z

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.

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 <[email protected]>

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 <[email protected]>

stefano-garzarella

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.

stefano-garzarella · 2023-09-13T08:04:30Z