Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Tue, 24 Sep 2019 11:19:33 +0800
From:   Yibo Zhao <yiboz@codeaurora.org>
To:     =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
Cc:     linux-wireless@vger.kernel.org, ath10k@lists.infradead.org,
        linux-wireless-owner@vger.kernel.org
Subject: Re: [PATCH V3 4/4] mac80211: Sync airtime weight sum with per AC
 synced sta airtime weight together
In-Reply-To: <87blvb5lb9.fsf@toke.dk>
References: <1569223201-1490-1-git-send-email-yiboz@codeaurora.org>
 <1569223201-1490-5-git-send-email-yiboz@codeaurora.org>
 <87blvb5lb9.fsf@toke.dk>
Message-ID: <ffe79f35d4dcb50aa31dc04035855f79@codeaurora.org>
User-Agent: Roundcube Webmail/1.2.5
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 2019-09-23 19:00, Toke Høiland-Jørgensen wrote:

>> -	if (params->airtime_weight)
>> -		sta->airtime_weight = params->airtime_weight;
>> +	if (params->airtime_weight &&
>> +	    params->airtime_weight != sta->airtime_weight) {
> 
> This check doesn't work I think? You're not using the array-based
> sta->airtime_weight[], and there are locking issues by just checking
> like this; so maybe just keep the if() on params->airtime_weight, and 
> do
> the checking inside the loop while holding the lock?

It should be array-based sta->airtime_weight[] and I am missing that 
part during the porting. But you are right about we should check it 
inside the loop with the lock.

> 
> Or could we just turn the weights into atomics to avoid the locking
> entirely?

Actually, we still need the active txq locking to make sure the txq is 
on the rbtree. Otherwise, no need to change airtime weight sum.

> 
>> +		for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
>> +			spin_lock_bh(&local->active_txq_lock[ac]);
>> +			for (tid = 0; tid < IEEE80211_NUM_TIDS + 1; tid++) {
>> +				if (!sta->sta.txq[tid] ||
>> +				    ac != ieee80211_ac_from_tid(tid))
>> +					continue;
>> +
>> +				pre_weight = sta->airtime_weight[ac];
>> +				sta->airtime_weight[ac] =
>> +						params->airtime_weight;
>> +
>> +				txqi = to_txq_info(sta->sta.txq[tid]);
>> +				if (RB_EMPTY_NODE(&txqi->schedule_order))
>> +					continue;
>> +
>> +				local->airtime_weight_sum[ac] = local->airtime_weight_sum[ac] +
>> +								params->airtime_weight -
>> +								pre_weight;
>> +			}
>> +			spin_unlock_bh(&local->active_txq_lock[ac]);
>> +		}
>> +	}
>> 


-- 
Yibo