Interleave sending and receiving of packets to reduce rx latency by liamjm · Pull Request #515 · google/OpenSK

liamjm · 2022-07-19T10:14:08Z

As discussed in #502 and #514 packet processing latency is too slow when both interfaces are being used. Another cause for this is that when a reply is being sent, it does so in a tight loop (ie inner loop in the main function) without receiving any other packets. This can easily introduce hundreds of milliseconds delay.

This PR removes the innner loop in main and leaves just a single loop. Each iteration it will optionally send a packet and always attempt to receive an incoming packet (#514 ensures suitable routing of the oldest packet). For our use case, the processing of these packets will be interleaved across the FIDO and Vendor HID interfaces (if both are present).

Several changes are required for this:

SendOrRecvStatus enum now includes the UsbEndpoint in the Received variant.
Code that uses SendOrRecvStatus handles the UsbEndpoint.
The HidConnection::send_or_recv_with_timeout now allows any endpoint to be received, not just the one it sent one.
HidPacketIterator now supports peeking to see if there is any data.
main.rs has new structs to support the state tracking and large changes to the loop.
tock CtapUsbSyscallDriver now sends packet before receiving incoming packet. Without this the tx packet is lost.

…iving of packets

patches/tock/13-send-packet-before-receiving.patch

ia0 · 2022-07-19T14:03:31Z

src/ctap/mod.rs

                debug_ctap!(env, "Sent KEEPALIVE packet");
            }
-            Ok(SendOrRecvStatus::Received) => {
+            Ok(SendOrRecvStatus::Received(_)) => {


What if we receive on a different endpoint than the one being kept alive? Does it mean we discard it?

Good find.

At present we discard it. I've added a TODO to address this (for a later PR), but I can address it here is you'd prefer.

I guess it's not really a regression (but I might be wrong). So I would say it's ok to do it later.

I think it is a regression if we receive, but the channel ID is by chance the same. Then a different transport would interact with this channel.

Yes, good point.
I can add a check for this.

However, I am wondering about a bigger change. If the code ignores packets on the other interface, then those packets are lost. So perhaps the better approach is to send() but not send_and_recv() at this time. That way the incoming packets are not serviced and will sit there. Looking at the code, it looks challenging to connect incoming packets to the other interface without massive refactoring.
Thoughts?

I generally agree that a send / receive interface would be cleaner. The current solution is there for practical reasons. USB Request Blocks (URB) that need to be answered quickly. And if you only send and have the syscall roundtrip delay from that, you can't answer OUT packets in time.

If you try to go for a send / receive interface, please test for everything that didn't work last time.
@gendx @jmichelp are the original authors who made that decision. Maybe they can help describe a few test cases to look out for?

OK.

What I've done in the short term is ignore those packet on the other interface. This stops the regression and leaves room for the above discussed improvements at a later date.
I've add tests to ./tools/vendor_hid_tests.py to verify this behavior.

src/lib.rs

src/main.rs

ia0

Looks good on my side. I'll be OOO until Aug 3rd so don't wait for me if there are me updates.

src/main.rs

kaczmarczyck · 2022-07-22T20:31:22Z

src/ctap/mod.rs

                debug_ctap!(env, "Sent KEEPALIVE packet");
            }
-            Ok(SendOrRecvStatus::Received) => {
+            Ok(SendOrRecvStatus::Received(_)) => {


I think it is a regression if we receive, but the channel ID is by chance the same. Then a different transport would interact with this channel.

src/ctap/mod.rs

src/lib.rs

src/main.rs

coveralls · 2022-07-25T00:22:21Z

Coverage decreased (-1.3%) to 88.573% when pulling 31cc379 on liamjm:send_or_recv into 6276904 on google:develop.

kaczmarczyck

2 high level comments:

The new main is different enough (i.e. might discard replies) that we should test OpenSK a bit with it.
The test script entangles raw HID and libfido. I don't think there is an elegant solution for this, but I thought it's interesting to mention that there is a way to make libfido talk to the second usage page. You monkey patch it's usage page:

import fido2
fido2.hid.base.FIDO_USAGE_PAGE = 0xFF00

You have to be careful to do that after GetInfo though, or we patch OpenSK to allow GetInfo on the second usage page. Not saying this approach is better or you should do it, it's just a different tradeoff I think.

liamjm · 2022-08-04T10:29:28Z

2 high level comments:

The new main is different enough (i.e. might discard replies) that we should test OpenSK a bit with it.

Testing with https://github.com/google/CTAP2-test-tool shows no differences.

The test script entangles raw HID and libfido. I don't think there is an elegant solution for this, but I thought it's interesting to mention that there is a way to make libfido talk to the second usage page. You monkey patch it's usage page:
import fido2
fido2.hid.base.FIDO_USAGE_PAGE = 0xFF00
You have to be careful to do that after GetInfo though, or we patch OpenSK to allow GetInfo on the second usage page. Not saying this approach is better or you should do it, it's just a different tradeoff I think.

Good approach. I've used this idea.
The value is patched only for the period of enumerating the devices for the Vendor HID device, so should be OK.

kaczmarczyck · 2022-08-04T11:12:31Z

If everything works, then I'm happy with it!
@jmichelp can always complain later if something is wrong that I didn't account for :)
I'll merge #523 first.

liamjm added 2 commits July 19, 2022 19:57

Interleave sending and receiving of packets to reduce latency in rece…

5dba6a2

…iving of packets

Add patch to CtapUsbSyscallDriver

2ad6a48

ia0 reviewed Jul 19, 2022

View reviewed changes

liamjm added 6 commits July 21, 2022 10:58

Merge branch 'develop' into send_or_recv

7b444d0

Minor tweaks from review

1a3bb2e

Log when overwritting an existing reply

c739c0c

Only log when 'debug_ctap' is enabled

a43b32d

Make ctap mod public, as per review

59737e6

Rename send_or_recv to send_and_maybe_recv

a0e4c20

liamjm force-pushed the send_or_recv branch from 38aa433 to a0e4c20 Compare July 22, 2022 04:40

ia0 previously approved these changes Jul 22, 2022

View reviewed changes

kaczmarczyck reviewed Jul 22, 2022

View reviewed changes

liamjm added 3 commits July 25, 2022 09:52

Merge branch 'develop' into send_or_recv

42da89d

fix typo

6e01b1b

Don't process packets on other transport while doing keepalive

2bd96ba

liamjm dismissed ia0’s stale review via 2bd96ba July 25, 2022 00:14

liamjm added 8 commits July 26, 2022 20:12

Don't process packets on other transport while doing keepalive

b949320

More accurately determine if reply has finished

c1ba03c

Move comment closer to appropriate location

5dee4be

Add tests for canceling keepalive packets

567a121

Added a TODO for kaczmarczyck re ctap module being public

e2e628b

remove the unnecessary sleep()s

79e8b91

undo messed up commit

361b7f3

address pylint warnings

250b356

kaczmarczyck reviewed Jul 26, 2022

View reviewed changes

liamjm added 4 commits July 29, 2022 15:05

Merge remote-tracking branch 'origin/develop' into send_or_recv

27255b9

Merge remote-tracking branch 'origin/develop' into send_or_recv

0f647e8

Fix merge mess up, and patch fido2 Usage Page

f4998d1

Fix up completely borked merge

8ba656d

Remove patch to FIDO usage, after google#523.

84ed15e

remove obsolete aspects to diff

31cc379

kaczmarczyck approved these changes Aug 4, 2022

View reviewed changes

Merge branch 'develop' into send_or_recv

4b63e02

kaczmarczyck merged commit 4a2217f into google:develop Aug 4, 2022

liamjm mentioned this pull request Aug 4, 2022

Increase CTAP timeouts while debugging enabled #502

Closed

liamjm deleted the send_or_recv branch August 4, 2022 23:16

Conversation

liamjm commented Jul 19, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ia0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaczmarczyck left a comment

Choose a reason for hiding this comment

Uh oh!

liamjm commented Aug 4, 2022

Uh oh!

kaczmarczyck commented Aug 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jul 25, 2022 •

edited

Loading