Re: [PATCH 1/4] wifi: mac80211: factor out part of ieee80211_calc_expected_tx_airtime

[Felix Fietkau <nbd@nbd.name>]

On 25.03.26 12:21, Pablo MARTIN-GOMEZ wrote:
> On 25/03/2026 04:58, Felix Fietkau wrote:
>> On 23.03.26 17:00, Pablo MARTIN-GOMEZ wrote:
>>> Hello,
>>>
> [...]
>>> I know this patch is just a refactoring, but I think this moved code is
>>> bugged. If (and it's a big if) I understood correctly the chain of
>>> macros and the comments, `ieee80211_get_rate_duration` return the
>>> `duration` in 1024 µs of an average packet (which would imply
>>> 1f38b8c564b8 is wrong) and the (PHY) `overhead` in µs for a (average)
>>> packet. So I believe the code should be:
>>> ```c
>>>     duration = ieee80211_get_rate_duration(hw, &stat, &overhead);
>>>     duration *= 1024;  /* now duration is in µs */
>>>     /* the agg_shift calculation has to be fixed */
>>>     duration += (overhead >> agg_shift);  /* for one packet, we
>>> "assign" a
>>> fraction of the overhead */
>>>     duration *= len/AVG_PKT_SIZE;  /* we multiply by the number of
>>> packets */
>>>     duration /= 1024;  /* we go back to a duration in 1024 µs*/
>>>
>>>     return duration;
>>> ```
>> 
>> The overhead (preamble, signal field, etc.) is a fixed per-frame PHY
>> cost that doesn't depend on how many data bytes are in the frame. In the
>> aggregated case, agg_shift amortizes that fixed cost across the
>> estimated number of subframes in the aggregate. So the correct order is:
>> scale the data duration to the actual packet size, then add the
>> amortized overhead once.
> My bad, I didn't understand that `len` was the byte size of a MPDU.
> 
> So I was wrong on where I put the overhead, but (a priori) not on the
> rest of the calculation *if* my understanding of the units is correct.
> If 1f38b8c564b8 is correct and so `duration` is in ns and `overhead` is
> in µs, then your code is correct, but the commit message is wrong
> because `ieee80211_rate_expected_tx_airtime` is returning a value in ns.
> 
> My snippet fixed if `duration` is in 1024 µs:
> ```c
>       duration = ieee80211_get_rate_duration(hw, &stat, &overhead);  /*
> duration of an average MPDU in 1024 µs */
>       duration *= 1024;  /* duration in µs */
>       duration /= AVG_PKT_SIZE;  /* duration in µs for a byte */
>       duration *= len;  /* duration in µs for the actual MPDU */
>       duration += (overhead >> agg_shift);  /* duration in µs for an
> approximate PPDU aka airtime */
>       duration /= 1024;  /* airtime duration in 1024 µs*/
> 
>       return duration;
> ```
> [`ieee80211_calc_expected_tx_airtime` has to be fixed too]
> 
> The current patch:
> ```c
> 	duration·=·ieee80211_get_rate_duration(hw,·&stat,·&overhead);  /*
> duration of an average MPDU in ns */
> 	duration·*=·len;
> 	duration·/=·AVG_PKT_SIZE;  /* duration in ns for the actual MPDU */
> 	duration·+=·(overhead·*·1024·>>·agg_shift);  /* adding the overhead in
> µs to a duration in ns to get PPDU duration: overhead [µs] == overhead *
> 1024 [ns] */
> 	return·duration;  /* airtime duration in ns */
> ```

The formatting of your snippets is really weird. Are you using some kind 
of LLM?

There is definitely an issue in the commit message, which I will fix in 
v2. It says the unit is 1024 us, when in fact it should say that the 
unit is 1/1024 us (approximately ns). Maybe that's what got you 
confused. Either way, your 'fixed' snippet seems wrong to me and the 
code should be fine as-is.

- Felix