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

Fix incorrect usage of TX_INFO signalling acked frames #12

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

Conversation

jprestwo
Copy link
Contributor

Before this change every CMD_FRAME coming from the kernel would be ack'ed locally through the TX_INFO command. This tells the client that the frame was acked in all cases, regardless if the server dropped it.

The kernel does expect a TX_INFO for each CMD_FRAME but the ACK flag should only be set if the peer received the frame. If the server drops it the ACK flag should be unset.

To fix this TX_INFO messages can be sent over the "air" just as frames are. Then clients can choose to include the ACK flag or not. This did require the server still sends all frames regardless if they were dropped, but a new boolean was added to signal to clients if the frame should be dropped. If so the client won't forward the frame but will still send a TX_INFO (with the ACK flag set appropriately).

Before this change every CMD_FRAME coming from the kernel would be
ack'ed locally through the TX_INFO command. This tells the client
that the frame was acked in all cases, regardless if the server
dropped it.

The kernel does expect a TX_INFO for each CMD_FRAME but the ACK
flag should only be set if the peer received the frame. If the
server drops it the ACK flag should be unset.

To fix this TX_INFO messages can be sent over the "air" just as
frames are. Then clients can choose to include the ACK flag or
not. This did require the server still sends all frames regardless
if they were dropped, but a new boolean was added to signal to
clients if the frame should be dropped. If so the client won't
forward the frame but will still send a TX_INFO (with the ACK
flag set appropriately).
@jprestwo
Copy link
Contributor Author

Please don't merge this, I need to run through more testing but I wanted to show what I was talking about in #11.

This does work as-is but there are some parts I'm not happy with. The main issue I see is its effectively doubling the amount of traffic to the server when really the TX_INFO only needs to go to a single client. This also puts more strain on the kernels for each client because they are sending TX_INFO's that will not match up cookies to frames and will be discarded. I suppose this is no different that forwarding frames that don't belong but its still not great. Maybe you have some ideas here.

@jprestwo
Copy link
Contributor Author

In thinking about this a bit I'm not sure there is an easy way of addressing any of my concerns with the extra traffic. The only way I can think to reduce the additional traffic would be to have the server track MAC addresses and have a "unicast" path for TX_INFO messages to travel directly to the intended receiver. Its not something I really want to dive into at the moment.

I still need to do more stress testing on this, but assuming things still work fine are you ok with this idea of sending the TX_INFO's as well as frames?

@Raizo62
Copy link
Owner

Raizo62 commented Oct 1, 2023

Sorry, I was busy with a technical issue at work.

I looked at your code. I may have missed a detail, but you do not send more frames than if packet loss was not activated ?

I still need to do more stress testing on this, but assuming things still work fine are you ok with this idea of sending the TX_INFO's as well as frames?

I don't understand your question : are you asking me if i agree with this method (sending the dropped trame to all clients) or a method that sends the TX_INFO to the intended receiver ?

@jprestwo
Copy link
Contributor Author

jprestwo commented Oct 2, 2023

I looked at your code. I may have missed a detail, but you do not send more frames than if packet loss was not activated ?

The additional packets I'm talking about is the TX_INFO, one is sent for each frame so that is 2x the traffic to the server.

I don't understand your question : are you asking me if i agree with this method (sending the dropped trame to all clients) or a method that sends the TX_INFO to the intended receiver ?

Both would be required for acks to behave correctly.

@Raizo62
Copy link
Owner

Raizo62 commented Oct 2, 2023

The additional packets I'm talking about is the TX_INFO, one is sent for each frame so that is 2x the traffic to the server.

Ok. I see it.

I suppose that the Client-A can't generate his own TxInfo from Client-B : Client-A need the values flags, signal, tx_attempts, cookie which are not predictable.
As you say, vwifi-server could track the MACs of each client. It is not easy to implement : one vwifi-client can have several wifi interfaces.

Both would be required for acks to behave correctly.

Are both necessary? Just 1 of the two methods is not enough ?

@jprestwo jprestwo force-pushed the fix-ack-handling branch 6 times, most recently from 104e6e5 to 67c0f98 Compare October 2, 2023 12:39
@jprestwo
Copy link
Contributor Author

jprestwo commented Oct 2, 2023

Are both necessary? Just 1 of the two methods is not enough ?

Thinking more about this I think I could cut down on the packets by having the server parse the message and send TX_INFO's back to individual clients. Dropped frames would be dropped as it is now and clients would not be responsible for sending TX_INFO's. This seems like a much better way of doing it. My concern initially with this was that the server doesn't have any idea what the clients real MAC address is, but since each client iterates its interfaces and sends the CMD_FRAME/TX_INFO it doesn't matter, the server can just set it to anything and the client will correct it.

Nevermind (sorry, its early), we can't do this because the server doesn't actually know what client the message is intended for. So it may drop for client A, but the message was for client B. Which brings up another issue, since the clients blindly forward frames its impossible to know if the frame was actually intended for that client... The kernel will know and send an ACK which is about all we can go off of, and at that point the frame cookie is long gone so we can't properly build a TX_INFO.

I'll need to think about this a bit more, not sure there is a simple way to solve this.

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.

2 participants