What about VLAN tags?
Best Regards
Sebastian
> On Oct 17, 2019, at 12:24, Toke Høiland-Jørgensen <[email protected]> wrote:
>
> Sebastian Moeller <[email protected]> writes:
>
>>> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen <[email protected]> wrote:
>>>
>>> Kan Yan <[email protected]> writes:
>>>
>>>> Hi Toke,
>>>>
>>>> Thanks for getting this done! I will give it a try in the next few
>>>> days. A few comments:
>>>>
>>>>> The estimated airtime for each skb is stored in the tx_info, so we can
>>>>> subtract the same amount from the running total when the skb is freed or
>>>>> recycled.
>>>>
>>>> Looks like ath10k driver zero out the info->status before calling
>>>> ieee80211_tx_status(...):
>>>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>>> const struct htt_tx_done *tx_done)
>>>> {
>>>> ...
>>>> info = IEEE80211_SKB_CB(msdu);
>>>> memset(&info->status, 0, sizeof(info->status));
>>>> ...
>>>> ieee80211_tx_status(htt->ar->hw, msdu);
>>>> }
>>>
>>> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
>>> it's only ath10k and iwl that do this, though, so it's probably
>>> manageable to just fix this. I think the simplest solution is just to
>>> restore the field after clearing, no?
>>>
>>>> We need either restore the info->status.tx_time_est or calling
>>>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
>>>> get erased.
>>>>
>>>>> + if (local->airtime_flags & AIRTIME_USE_AQL) {
>>>>> + airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>>>>> + skb->len + 38);
>>>>
>>>> I think it is better to put the "+ 38" that takes care of the header
>>>> overhead inside ieee80211_calc_expected_tx_airtime().
>>>
>>> Hmm, no strong opinion about this; but yeah, since we have a dedicated
>>> function for this use I guess there's no harm in adding it there :)
>>>
>>
>> Silly question, is this Overhead guaranteed to be 38 Bytes for all
>> eternity? Otherwise a variable or a preprocessor constant might be
>> more future proof?
>
> Well, yeah, as long as we're sending Ethernet packets. Which is kinda
> baked into the WiFi standard :)
>
> -Toke
I don't think it is hard to take care of extra header size for frames
with VLAN tags, but the question is how far are we willing to go down
this rabbit hole. There are other factors like retries and aggregation
that are difficult to be taken into account. However, I doubt that
matters, a ballpark estimate of airtime is sufficient for the purpose
of implementing a queue limit.
On Thu, Oct 17, 2019 at 3:25 AM Sebastian Moeller <[email protected]> wrote:
>
> What about VLAN tags?
>
> Best Regards
> Sebastian
>
> > On Oct 17, 2019, at 12:24, Toke Høiland-Jørgensen <[email protected]> wrote:
> >
> > Sebastian Moeller <[email protected]> writes:
> >
> >>> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen <[email protected]> wrote:
> >>>
> >>> Kan Yan <[email protected]> writes:
> >>>
> >>>> Hi Toke,
> >>>>
> >>>> Thanks for getting this done! I will give it a try in the next few
> >>>> days. A few comments:
> >>>>
> >>>>> The estimated airtime for each skb is stored in the tx_info, so we can
> >>>>> subtract the same amount from the running total when the skb is freed or
> >>>>> recycled.
> >>>>
> >>>> Looks like ath10k driver zero out the info->status before calling
> >>>> ieee80211_tx_status(...):
> >>>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
> >>>> const struct htt_tx_done *tx_done)
> >>>> {
> >>>> ...
> >>>> info = IEEE80211_SKB_CB(msdu);
> >>>> memset(&info->status, 0, sizeof(info->status));
> >>>> ...
> >>>> ieee80211_tx_status(htt->ar->hw, msdu);
> >>>> }
> >>>
> >>> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
> >>> it's only ath10k and iwl that do this, though, so it's probably
> >>> manageable to just fix this. I think the simplest solution is just to
> >>> restore the field after clearing, no?
> >>>
> >>>> We need either restore the info->status.tx_time_est or calling
> >>>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
> >>>> get erased.
> >>>>
> >>>>> + if (local->airtime_flags & AIRTIME_USE_AQL) {
> >>>>> + airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
> >>>>> + skb->len + 38);
> >>>>
> >>>> I think it is better to put the "+ 38" that takes care of the header
> >>>> overhead inside ieee80211_calc_expected_tx_airtime().
> >>>
> >>> Hmm, no strong opinion about this; but yeah, since we have a dedicated
> >>> function for this use I guess there's no harm in adding it there :)
> >>>
> >>
> >> Silly question, is this Overhead guaranteed to be 38 Bytes for all
> >> eternity? Otherwise a variable or a preprocessor constant might be
> >> more future proof?
> >
> > Well, yeah, as long as we're sending Ethernet packets. Which is kinda
> > baked into the WiFi standard :)
> >
> > -Toke
>
On Thu, 2019-10-17 at 18:11 -0700, Kan Yan wrote:
> I don't think it is hard to take care of extra header size for frames
> with VLAN tags
VLAN tags are payload as far as wifi is concerned, so no need to take
that into account ...
johannes