Re: [PATCH v8] mac80211: Optimize scans on current operating channel.

[Ben Greear <greearb@candelatech.com>]

On 02/02/2011 09:53 AM, Johannes Berg wrote:
> On Wed, 2011-02-02 at 09:43 -0800, Ben Greear wrote:
>
>>>> +	/* If this off-channel logic ever changes,  ieee80211_on_oper_channel
>>>> +	 * may need to change as well.
>>>> +	 */
>>>
>>> Would it make sense to roll this thing into one?
>>>
>>> Like "ieee80211_get_desired_channel(local,&chan,&chantype)"
>>>
>>> and then this code and on_oper_channel would use that?
>>
>> As you say, the patch is big and scary already.  I would like to
>> attempt this change as a small follow-on patch that does just
>> one thing.  To me, the open-coded logic is a bit more similar
>> to existing logic and thus easier to review.
>
> Yeah that's a good plan.
>
>>>> +	/* If we are scanning on-channel, pass the pkt on up the stack so that
>>>> +	 * mlme can make use of it
>>>> +	 */
>>>> +	if (ieee80211_cfg_on_oper_channel(sdata->local)
>>>> +	&&   (channel == sdata->local->oper_channel))
>>>> +		return RX_CONTINUE;
>>>
>>> Ah, neat trick, no need to duplicate the packet :-)
>>> But didn't you say this wasn't necessary since timers were stopped
>>> anyway during scan? So maybe the comment shouldn't refer to scanning,
>>> but other work?
>>
>> Timers are stopped only if we are off-channel for scanning, I think.
>> And, they are NOT stopped when we go off channel for work_work().
>> Even if the packets are not currently used by the rest of the
>> stack, it seems logically sound to pass them up in this case.
>
> Right. But could you change the comment to clarify?

Sure, will do.  I'm making the comment slightly more generic,
but let me know if you want it changed.

>>>> +++ b/net/mac80211/tx.c
>>>> @@ -257,7 +257,8 @@ ieee80211_tx_h_check_assoc(struct ieee80211_tx_data *tx)
>>>>    	if (unlikely(info->flags&   IEEE80211_TX_CTL_INJECTED))
>>>>    		return TX_CONTINUE;
>>>>
>>>> -	if (unlikely(test_bit(SCAN_OFF_CHANNEL,&tx->local->scanning))&&
>>>> +	if (unlikely(test_bit(SCAN_SW_SCANNING,&tx->local->scanning))&&
>>>> +	    test_bit(SDATA_STATE_OFFCHANNEL,&tx->sdata->state)&&
>>>
>>> Shouldn't that be || ? Or only the off-channel check I think?
>>
>> The old check was for scan-off-channel flag.  That is equiv
>> to sw-scanning AND state-offchannel.
>
> Oh, true. I was just thinking this should also kick in for work stuff
> off-channel.
>
> Come to think of it -- what if work isn't off-channel, but needs to
> disable powersave?

I don't know.  I tried to change the work logic as little as possible
while dealing with just on/off channel changes.  If work has power-save
requirements, then probably we'd need to add a new flag and explicitly
deal with power-save instead of relying on some subtle effect of going
on or off channel.

>> One thing:  If we are normally operating in HT40 mode, we have to
>> shift to NO_HT for scanning.  But, I think we could probably still
>> transmit packets even if we are temporarily in NO_HT, right?
>
> Oh, hmm, that might be tricky depending on the rate scale algorithm and
> the device. But do we really have to shift to NO_HT for scanning?

I have no idea.  The current code explicitly sets NO_HT
(ie, config_hw() when scan_chan is set).

If we can scan on other channel-types, then that would be another
thing to add in a new patch.  This would be of interest to
me since currently the 'scan-on-channel' doesn't really
save much work if we are running any type of HT mode...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com