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 mira_node statistics in driver #466

Merged
merged 2 commits into from
Dec 26, 2020

Conversation

pigworlds
Copy link
Contributor

The itlwm driver uses MiRA algorithm to control the tx rate (upload). There is a bit disconnect between what the MiRA rate adaption statistics wants and what the iwm/iwn driver passed. Note, the iwx driver uses firmware based RA and do not use MiRA.

In the mira formula, txfail mean the whole single frame or the whole AMPDU frame get lost. The driver deduce the number of txfail from the number of missing ACKs in BA. This looks wrong to me that a temporary sub-frame lost in the ACK would count as whole frame lost. Counting on the number of retries is sufficient to calculate in the mira formula.

Also in mira formula, it calculate ampdu_size * agglen to count the number of bytes transmitted. The ampdu_size should be the average of each sub-frame ampdu size, rather than the total ampdu size of all sub-frames.

In the AMPDU code for iwm, there was an early return on iwm_rx_tx_cmd when frame count more than 1. This seems unnecessary and would affect mira statistics collection.

@zxystd
Copy link
Collaborator

zxystd commented Dec 25, 2020

Maybe this will take effect, but I am not sure because the Mira node designed as collecting sub-frames status when it is in aggregation mode, and now you change it as whole A-MPDU status. correct me if I am wrong, I will take sometime to look into Mira code implementation.

struct ieee80211_mira_node {
	/*
	 * Fields set by drivers before calling ieee80211_mira_choose().
	 */
	uint32_t frames;	/* Increment per (sub-)frame transmitted. */
	uint32_t retries;	/* Increment per Tx retry (frame not ACKed). */
	uint32_t txfail;	/* Increment per Tx failure (also not ACKed). */
	uint32_t ampdu_size;	/* Length of last (aggregated) frame sent. */
	uint32_t agglen;	/* Number of subframes in last frame (1-64). */

@pigworlds
Copy link
Contributor Author

This MiRA need to be reviewed carefully.

  • In the original paper, SFER (Sub-Frame Error Rate) is defined as . It is calculated as
    sfer = mn->frames * mn->txfail + mn->retries;
    sfer /= (mn->txfail + 1) * mn->frames;

The mn->txfail means the whole AMPDU frame get retried. The code calculate the mn->txfail from the missing ACKs. For eg, a frame contains 3 subframe and 2 missing ACK. It counts the 2 whole frames lost and retransmitted. This doesn't seems right to me.

  • On the goodput calculation, The paper define goodput as where Ar(t) is the measured aggregation level (in terms of frames) for the current probing frame, where DATA is the payload size of a MAC-layer frame. It is calculated as
    toverhead = MIRA_FP_MUL(toverhead, rate);
    g->measured = MIRA_FP_DIV(MIRA_FP_1 - sfer, MIRA_FP_1 +
                              MIRA_FP_DIV(toverhead, MIRA_FP_MUL(ampdu_size, g->average_agg)));
    g->measured = MIRA_FP_MUL(g->measured, rate);

The ampdu_size is the individual payload size. The counted as the whole AMPDU size. When multiply with averagg_agg (derived from agglen), it effectively calculate the ampdu_size*agglen^2, which makes the measured goodput too good.

These are the two issues obviously wrong to me.

There are still a few issues in the algorithm. It is sensitive to failures, when single frame retries happen, it count towards 100% loss and triggering downwards rate probing. When probing upwards (after the timer fix #465), it appear raising the rate very slow. Itself doesn't support calculate channel wider than 20MHz. It only support 11n rates and not 11ac rates. My guess this rate adaption is at least 80% comparable to iwlwifi with the fixes above, the next 20% needs a lot of fixes.

@zxystd zxystd merged commit eeebc4c into OpenIntelWireless:master Dec 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants